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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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 4 commits
Commits
Show all changes
5 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
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
22 changes: 21 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,29 @@
FreeCall() { this.getTarget().hasGlobalName("free") }
}

predicate isAffectedByMacro(FreeCall fc, BasicBlock bb) {
fc.isInMacroExpansion()
or
exists(PreprocessorBranch ppb, Location bbLoc, Location ppbLoc |
bbLoc = bb.(Stmt).getLocation() and ppbLoc = ppb.getLocation()
|
bbLoc.getStartLine() < ppbLoc.getStartLine() and
ppbLoc.getEndLine() < bbLoc.getEndLine()
)
Fixed Show fixed Hide fixed
}

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
(
bb = fc.getEnclosingStmt()
or
strictcount(bb.(BlockStmt).getAStmt()) = 1
) and
strictcount(BasicBlock bb2 | gc.ensuresEq(_, 0, bb2, _) | bb2) = 1 and
not isAffectedByMacro(fc, 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);
}