Skip to content

C++: Fix cpp/new-free-mismatch false positives #3206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 8, 2020
Merged
1 change: 1 addition & 0 deletions change-notes/1.24/analysis-cpp.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
2 changes: 1 addition & 1 deletion cpp/ql/src/Critical/MemoryFreed.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
}
Expand Down
44 changes: 37 additions & 7 deletions cpp/ql/src/Critical/NewDelete.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(_)
}

/**
Expand Down Expand Up @@ -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"
Expand Down
2 changes: 2 additions & 0 deletions cpp/ql/src/semmle/code/cpp/commons/Alloc.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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() }

Expand Down
Original file line number Diff line number Diff line change
@@ -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

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -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`.
Expand Down
Original file line number Diff line number Diff line change
@@ -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 |
Expand Down
24 changes: 24 additions & 0 deletions cpp/ql/test/query-tests/Critical/NewFree/test2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,27 @@ class MyTest2Class
};

MyTest2Class<int> 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
}