diff --git a/cpp/ql/src/Critical/NewDelete.qll b/cpp/ql/src/Critical/NewDelete.qll index 05c641c04c32..97848e3bbdf2 100644 --- a/cpp/ql/src/Critical/NewDelete.qll +++ b/cpp/ql/src/Critical/NewDelete.qll @@ -46,32 +46,20 @@ predicate allocExprOrIndirect(Expr alloc, string kind) { alloc.(FunctionCall).getTarget() = rtn.getEnclosingFunction() and ( allocExprOrIndirect(rtn.getExpr(), kind) or - allocReaches(rtn.getExpr(), _, kind) + allocReaches0(rtn.getExpr(), _, kind) ) ) } /** - * Holds if `v` is assigned value `e`, and `e` is not known to be `0`. + * Holds if `v` is a non-local variable which is assigned with allocations of + * type `kind`. */ -private predicate nonNullGlobalAssignment(Variable v, Expr e) { - not v instanceof LocalScopeVariable and - v.getAnAssignedValue() = e and - not e.getValue().toInt() = 0 -} - -/** - * Holds if `v` is a non-local variable which is assigned only with allocations of - * type `kind` (it may also be assigned with NULL). - */ -private predicate allocReachesVariable(Variable v, Expr alloc, string kind) { +private pragma[nomagic] predicate allocReachesVariable(Variable v, Expr alloc, string kind) { exists(Expr mid | - nonNullGlobalAssignment(v, mid) and - allocReaches(mid, alloc, kind) - ) and - forall(Expr mid | - nonNullGlobalAssignment(v, mid) | - allocReaches(mid, _, kind) + not v instanceof LocalScopeVariable and + v.getAnAssignedValue() = mid and + allocReaches0(mid, alloc, kind) ) } @@ -80,22 +68,35 @@ private predicate allocReachesVariable(Variable v, Expr alloc, string kind) { * result of a previous memory allocation `alloc`. `kind` is a * string describing the type of that allocation. */ -predicate allocReaches(Expr e, Expr alloc, string kind) { +private predicate allocReaches0(Expr e, Expr alloc, string kind) { ( // alloc allocExprOrIndirect(alloc, kind) and e = alloc ) or exists(SsaDefinition def, LocalScopeVariable v | // alloc via SSA - allocReaches(def.getAnUltimateDefiningValue(v), alloc, kind) and + allocReaches0(def.getAnUltimateDefiningValue(v), alloc, kind) and e = def.getAUse(v) ) or exists(Variable v | - // alloc via a singly assigned global + // alloc via a global allocReachesVariable(v, alloc, kind) and e.(VariableAccess).getTarget() = v ) } +/** + * Holds if `e` is an expression which may evaluate to the + * result of previous memory allocations `alloc` only of type + * `kind`. + */ +predicate allocReaches(Expr e, Expr alloc, string kind) { + allocReaches0(e, alloc, kind) and + not exists(string k2 | + allocReaches0(e, _, k2) and + kind != k2 + ) +} + /** * Holds if `free` is a use of free or delete. `freed` is the * expression that is freed / deleted and `kind` is a string diff --git a/cpp/ql/test/query-tests/Critical/NewFree/test.cpp b/cpp/ql/test/query-tests/Critical/NewFree/test.cpp index fd5485ca8766..4fe8559d7eb0 100644 --- a/cpp/ql/test/query-tests/Critical/NewFree/test.cpp +++ b/cpp/ql/test/query-tests/Critical/NewFree/test.cpp @@ -337,3 +337,37 @@ class Test11 char *data; }; + +// --- + +int *z; + +void test12(bool cond) +{ + int *x, *y; + + x = new int(); + delete x; // GOOD + x = (int *)malloc(sizeof(int)); + free(x); // GOOD + + if (cond) + { + y = new int(); + z = new int(); + } else { + y = (int *)malloc(sizeof(int)); + z = (int *)malloc(sizeof(int)); + } + + // ... + + if (cond) + { + delete y; // GOOD + delete z; // GOOD + } else { + free(y); // GOOD + free(z); // GOOD + } +}