diff --git a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql index 2d504d9bc057..66c7a31111b5 100644 --- a/cpp/ql/src/experimental/Best Practices/GuardedFree.ql +++ b/cpp/ql/src/experimental/Best Practices/GuardedFree.ql @@ -18,9 +18,31 @@ class FreeCall extends FunctionCall { FreeCall() { this.getTarget().hasGlobalName("free") } } +predicate blockContainsPreprocessorBranches(BasicBlock bb) { + exists(PreprocessorBranch ppb, Location bbLoc, Location ppbLoc | + bbLoc = bb.(Stmt).getLocation() and ppbLoc = ppb.getLocation() + | + bbLoc.getFile() = ppb.getFile() and + bbLoc.getStartLine() < ppbLoc.getStartLine() and + ppbLoc.getEndLine() < bbLoc.getEndLine() + ) +} + from GuardCondition gc, FreeCall fc, Variable v, BasicBlock bb where gc.ensuresEq(v.getAnAccess(), 0, bb, false) and fc.getArgument(0) = v.getAnAccess() and - bb = fc.getEnclosingStmt() + bb = fc.getBasicBlock() and + ( + // No block statement: if (x) free(x); + bb = fc.getEnclosingStmt() + or + // Block statement with a single nested statement: if (x) { free(x); } + strictcount(bb.(BlockStmt).getAStmt()) = 1 + ) and + strictcount(BasicBlock bb2 | gc.ensuresEq(_, 0, bb2, _) | bb2) = 1 and + not fc.isInMacroExpansion() and + not blockContainsPreprocessorBranches(bb) and + not (gc instanceof BinaryOperation and not gc instanceof ComparisonOperation) and + not exists(CommaExpr c | c.getAChild*() = fc) select gc, "unnecessary NULL check before call to $@", fc, "free" diff --git a/cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/GuardedFree.expected b/cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/GuardedFree.expected index 209bae407b82..a9a189218c62 100644 --- a/cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/GuardedFree.expected +++ b/cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/GuardedFree.expected @@ -1,10 +1,5 @@ | test.cpp:5:7:5:7 | x | unnecessary NULL check before call to $@ | test.cpp:6:5:6:8 | call to free | free | -| test.cpp:23:7:23:7 | x | unnecessary NULL check before call to $@ | test.cpp:26:5:26:8 | call to free | free | -| test.cpp:31:7:31:8 | ! ... | unnecessary NULL check before call to $@ | test.cpp:35:3:35:6 | call to free | free | -| test.cpp:31:7:31:24 | ... \|\| ... | unnecessary NULL check before call to $@ | test.cpp:35:3:35:6 | call to free | free | -| test.cpp:31:8:31:8 | x | unnecessary NULL check before call to $@ | test.cpp:35:3:35:6 | call to free | free | -| test.cpp:94:12:94:12 | x | unnecessary NULL check before call to $@ | test.cpp:94:3:94:13 | call to free | free | -| test.cpp:98:7:98:8 | ! ... | unnecessary NULL check before call to $@ | test.cpp:101:3:101:6 | call to free | free | -| test.cpp:98:8:98:8 | x | unnecessary NULL check before call to $@ | test.cpp:101:3:101:6 | call to free | free | +| test.cpp:10:7:10:7 | x | unnecessary NULL check before call to $@ | test.cpp:11:5:11:8 | call to free | free | +| test.cpp:42:7:42:7 | x | unnecessary NULL check before call to $@ | test.cpp:43:5:43:8 | call to free | free | +| test.cpp:49:7:49:7 | x | unnecessary NULL check before call to $@ | test.cpp:50:5:50:8 | call to free | free | | test.cpp:106:7:106:18 | ... != ... | unnecessary NULL check before call to $@ | test.cpp:107:5:107:8 | call to free | free | -| test.cpp:113:7:113:18 | ... != ... | unnecessary NULL check before call to $@ | test.cpp:114:17:114:20 | call to free | free | diff --git a/cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/test.cpp b/cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/test.cpp index 12b1fb2364e7..d52bcef72d16 100644 --- a/cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/test.cpp +++ b/cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/test.cpp @@ -20,7 +20,7 @@ void test2(int *x) { } void test3(int *x, bool b) { - if (x) { // GOOD [FALSE POSITIVE]: x is being accessed in the body of the if + if (x) { // GOOD: x is being accessed in the body of the if if (b) *x = 42; free(x); @@ -28,7 +28,7 @@ void test3(int *x, bool b) { } bool test4(char *x, char *y) { - if (!x || strcmp(x, y)) { // GOOD [FALSE POSITIVE]: x is being accessed in the guard and return value depends on x + if (!x || strcmp(x, y)) { // GOOD: x is being accessed in the guard and return value depends on x free(x); return true; } @@ -91,11 +91,11 @@ void test10(char *x) { if (x) free(x); void test11(char *x) { - TRY_FREE(x) // BAD + TRY_FREE(x) // BAD [NOT DETECTED] } bool test12(char *x) { - if (!x) // GOOD [FALSE POSITIVE]: return value depends on x + if (!x) // GOOD: return value depends on x return false; free(x); @@ -110,6 +110,6 @@ void test13(char *x) { void inspect(char *x); void test14(char *x) { - if (x != nullptr) // GOOD [FALSE POSITIVE]: x might be accessed in the first operand of the comma operator + if (x != nullptr) // GOOD: x might be accessed in the first operand of the comma operator inspect(x), free(x); }