Skip to content
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.

Sign up for GitHub

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

Jump to bottom

C++: Reduce number of FPs cpp/guarded-free and turn if(x) { free(x) } cases from FNs to TPs #17986

Merged
merged 8 commits into from
Nov 20, 2024
Merged

C++: Reduce number of FPs cpp/guarded-free and turn if(x) { free(x) } cases from FNs to TPs #17986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c86bbbb
C++: Fix `cpp/guarded-free` FPs when there are other blocks depending…
jketema Nov 14, 2024
a31e983
C++: Also allow single statement blocks in `cpp/guarded-free`
jketema Nov 14, 2024
176acab
C++: Ignore `free` calls that are macro defined or `#if`/`#ifdef` gua…
jketema Nov 14, 2024
b581723
C++: Ignore complex guards and the comma operator
jketema Nov 14, 2024
2068591
C++: Silence ql-for-ql warning
jketema Nov 14, 2024
88be4b8
C++: Address review comments
jketema Nov 18, 2024
08bf6d7
Merge branch 'main' into guarded-free2
jketema Nov 18, 2024
84f3e6a
Merge branch 'main' into guarded-free2
jketema Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion cpp/ql/src/experimental/Best Practices/GuardedFree.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved
)
}

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
geoffw0 marked this conversation as resolved.
Show resolved Hide resolved
) and
strictcount(BasicBlock bb2 | gc.ensuresEq(_, 0, bb2, _) | bb2) = 1 and
not fc.isInMacroExpansion() and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't particularly like this exclusion, but I can see that it circumvents some really difficult cases and helps get us a good set of results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

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"
11 changes: 3 additions & 8 deletions cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/GuardedFree.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 |
10 changes: 5 additions & 5 deletions cpp/ql/test/experimental/query-tests/Best Practices/GuardedFree/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ 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);
}
}

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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}