diff --git a/change-notes/1.24/analysis-cpp.md b/change-notes/1.24/analysis-cpp.md index 2b53e6d56aa7..73d58a57404e 100644 --- a/change-notes/1.24/analysis-cpp.md +++ b/change-notes/1.24/analysis-cpp.md @@ -53,3 +53,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications. * The library now models data flow through formatting functions such as `sprintf`. * The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) uses a new intermediate representation. This provides a more precise analysis of pointers to stack variables and flow through parameters, improving the results of many security queries. * The global value numbering library (`semmle.code.cpp.valuenumbering.GlobalValueNumbering`) uses a new intermediate representation to provide a more precise analysis of heap allocated memory and pointers to stack variables. +* `freeCall` in `semmle.code.cpp.commons.Alloc` has been deprecated. The`Allocation` and `Deallocation` models in `semmle.code.cpp.models.interfaces` should be used instead. diff --git a/cpp/ql/src/Critical/MemoryFreed.qll b/cpp/ql/src/Critical/MemoryFreed.qll index d548aca26e7c..880199e54c9f 100644 --- a/cpp/ql/src/Critical/MemoryFreed.qll +++ b/cpp/ql/src/Critical/MemoryFreed.qll @@ -4,7 +4,7 @@ private predicate freed(Expr e) { e = any(DeallocationExpr de).getFreedExpr() or exists(ExprCall c | - // cautiously assume that any ExprCall could be a freeCall. + // cautiously assume that any `ExprCall` could be a deallocation expression. c.getAnArgument() = e ) } diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll index 9dd55525b59a..30b9f9ad94ad 100644 --- a/cpp/ql/src/Critical/NewDelete.qll +++ b/cpp/ql/src/Critical/NewDelete.qll @@ -5,17 +5,34 @@ import cpp import semmle.code.cpp.controlflow.SSA import semmle.code.cpp.dataflow.DataFlow +import semmle.code.cpp.models.implementations.Allocation +import semmle.code.cpp.models.implementations.Deallocation /** * Holds if `alloc` is a use of `malloc` or `new`. `kind` is * a string describing the type of the allocation. */ predicate allocExpr(Expr alloc, string kind) { - isAllocationExpr(alloc) and - not alloc.isFromUninstantiatedTemplate(_) and ( - alloc instanceof FunctionCall and - kind = "malloc" + exists(Function target | + alloc.(AllocationExpr).(FunctionCall).getTarget() = target and + ( + target.getName() = "operator new" and + kind = "new" and + // exclude placement new and custom overloads as they + // may not conform to assumptions + not target.getNumberOfParameters() > 1 + or + target.getName() = "operator new[]" and + kind = "new[]" and + // exclude placement new and custom overloads as they + // may not conform to assumptions + not target.getNumberOfParameters() > 1 + or + not target instanceof OperatorNewAllocationFunction and + kind = "malloc" + ) + ) or alloc instanceof NewExpr and kind = "new" and @@ -28,7 +45,8 @@ predicate allocExpr(Expr alloc, string kind) { // exclude placement new and custom overloads as they // may not conform to assumptions not alloc.(NewArrayExpr).getAllocatorCall().getTarget().getNumberOfParameters() > 1 - ) + ) and + not alloc.isFromUninstantiatedTemplate(_) } /** @@ -110,8 +128,20 @@ predicate allocReaches(Expr e, Expr alloc, string kind) { * describing the type of that free or delete. */ predicate freeExpr(Expr free, Expr freed, string kind) { - freeCall(free, freed) and - kind = "free" + exists(Function target | + freed = free.(DeallocationExpr).getFreedExpr() and + free.(FunctionCall).getTarget() = target and + ( + target.getName() = "operator delete" and + kind = "delete" + or + target.getName() = "operator delete[]" and + kind = "delete[]" + or + not target instanceof OperatorDeleteDeallocationFunction and + kind = "free" + ) + ) or free.(DeleteExpr).getExpr() = freed and kind = "delete" diff --git a/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll b/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll index a508c929d9ae..a9597fc72b5a 100644 --- a/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll +++ b/cpp/ql/src/semmle/code/cpp/commons/Alloc.qll @@ -23,6 +23,8 @@ predicate freeFunction(Function f, int argNum) { argNum = f.(DeallocationFunctio /** * A call to a library routine that frees memory. + * + * DEPRECATED: Use `DeallocationExpr` instead (this also includes `delete` expressions). */ predicate freeCall(FunctionCall fc, Expr arg) { arg = fc.(DeallocationExpr).getFreedExpr() } diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll index c6766983889c..f6f7ab279a68 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll @@ -1,3 +1,9 @@ +/** + * Provides implementation classes modelling various methods of allocation + * (`malloc`, `new` etc). See `semmle.code.cpp.models.interfaces.Allocation` + * for usage information. + */ + import semmle.code.cpp.models.interfaces.Allocation /** diff --git a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll index d2e4951e436a..980645df0312 100644 --- a/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll +++ b/cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll @@ -1,4 +1,10 @@ -import semmle.code.cpp.models.interfaces.Allocation +/** + * Provides implementation classes modelling various methods of deallocation + * (`free`, `delete` etc). See `semmle.code.cpp.models.interfaces.Deallocation` + * for usage information. + */ + +import semmle.code.cpp.models.interfaces.Deallocation /** * A deallocation function such as `free`. diff --git a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected index 7350749ce962..45f88d426c98 100644 --- a/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected +++ b/cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected @@ -1,5 +1,9 @@ | test2.cpp:19:3:19:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:18:12:18:18 | new | new | | test2.cpp:26:3:26:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:25:7:25:13 | new | new | +| test2.cpp:51:2:51:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:45:18:45:24 | new | new | +| test2.cpp:55:2:55:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:46:20:46:33 | call to operator new | new | +| test2.cpp:57:2:57:18 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc | +| test2.cpp:58:2:58:18 | call to operator delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc | | test.cpp:36:2:36:17 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:27:18:27:23 | call to malloc | malloc | | test.cpp:41:2:41:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test.cpp:26:7:26:17 | new | new | | test.cpp:68:3:68:11 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:64:28:64:33 | call to malloc | malloc | diff --git a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp index 9101758d85b3..43a286f6f97f 100644 --- a/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp +++ b/cpp/ql/test/query-tests/Critical/NewFree/test2.cpp @@ -34,3 +34,27 @@ class MyTest2Class }; MyTest2Class mt2c_i; + +// --- + +void* operator new(size_t); +void operator delete(void*); + +void test_operator_new() +{ + void *ptr_new = new int; + void *ptr_opnew = ::operator new(sizeof(int)); + void *ptr_malloc = malloc(sizeof(int)); + + delete ptr_new; // GOOD + ::operator delete(ptr_new); // GOOD + free(ptr_new); // BAD + + delete ptr_opnew; // GOOD + ::operator delete(ptr_opnew); // GOOD + free(ptr_opnew); // BAD + + delete ptr_malloc; // BAD + ::operator delete(ptr_malloc); // BAD + free(ptr_malloc); // GOOD +}