Skip to content

Commit ff62d60

Browse files
zlaski-semmlezlaski-semmle
committed
[CPP-435] Initial IR-based version of query.
1 parent 81add37 commit ff62d60

4 files changed

Lines changed: 40 additions & 68 deletions

File tree

‎cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.ql‎

Lines changed: 14 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -17,50 +17,23 @@ import semmle.code.cpp.ir.ValueNumbering
1717
import semmle.code.cpp.models.implementations.Memset
1818

1919
class MemsetCallInstruction extends CallInstruction {
20-
MemsetCallInstruction() {
21-
this.getValue() = "0" and
22-
this.getResultType().getUnspecifiedType() instanceof PointerType
23-
}
20+
MemsetCallInstruction() { this.getStaticCallTarget() instanceof MemsetFunction }
2421
}
2522

26-
predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) {
27-
bool = any(CompareInstruction cmp |
28-
exists(NullInstruction null |
29-
cmp.getLeft() = null and cmp.getRight() = checked
30-
or
31-
cmp.getLeft() = checked and cmp.getRight() = null
32-
|
33-
cmp instanceof CompareEQInstruction
34-
or
35-
cmp instanceof CompareNEInstruction
36-
)
37-
)
38-
or
39-
bool = any(ConvertInstruction convert |
40-
checked = convert.getUnary() and
41-
convert.getResultType() instanceof BoolType and
42-
checked.getResultType() instanceof PointerType
43-
)
23+
private Instruction insnSuccessor(Instruction i) {
24+
exists(GotoEdge edge | result = i.getSuccessor(edge))
4425
}
4526

46-
pragma[noinline]
47-
predicate candidateResult(LoadInstruction checked, ValueNumber value, IRBlock dominator) {
48-
explicitNullTestOfInstruction(checked, _) and
49-
not checked.getAST().isInMacroExpansion() and
50-
value.getAnInstruction() = checked and
51-
dominator.dominates(checked.getBlock())
27+
private predicate insnDominates(Instruction i1, Instruction i2) {
28+
i1.getBlock().dominates(i2.getBlock())
29+
or
30+
i1.getBlock() = i2.getBlock() and insnSuccessor+(i1) = i2
5231
}
5332

54-
from LoadInstruction checked, LoadInstruction deref, ValueNumber sourceValue, IRBlock dominator
55-
where
56-
candidateResult(checked, sourceValue, dominator) and
57-
sourceValue.getAnInstruction() = deref.getSourceAddress() and
58-
// This also holds if the blocks are equal, meaning that the check could come
59-
// before the deref. That's still not okay because when they're in the same
60-
// basic block then the deref is unavoidable even if the check concluded that
61-
// the pointer was null. To follow this idea to its full generality, we
62-
// should also give an alert when `check` post-dominates `deref`.
63-
deref.getBlock() = dominator
64-
select checked, "This null check is redundant because the value is $@ in any case", deref,
65-
"dereferenced here"
66-
select fc, "Call to " + fc.getTarget().getName() + " may be deleted by the compiler."
33+
//insnDominates(memset, deref) and
34+
//vn.getAnInstruction() = memset.getAnArgument() and
35+
//vn.getAnInstruction() = deref.getSourceAddress()
36+
from MemsetCallInstruction memset
37+
where not exists(LoadInstruction deref | memset.getBlock().dominates(deref.getBlock())) // insnDominates(memset, deref))
38+
select memset,
39+
"Call to " + memset.getStaticCallTarget().getName() + " may be deleted by the compiler."

‎cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.c‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ int func1a(void) {
3636
char *func1b(void) {
3737
char pw1b[PW_SIZE];
3838
use_pw(pw1b);
39-
memset(pw1b, 0, PW_SIZE); // BAD [NOT DETECTED]
39+
memset(pw1b, 0, PW_SIZE); // BAD
4040
pw1b[0] = pw1b[3] = 'a';
4141
return 0;
4242
}
@@ -50,13 +50,13 @@ int func1c(char pw1c[PW_SIZE]) {
5050
return 0;
5151
}
5252

53-
// x86-64 gcc 9.2: deleted
54-
// x86-64 clang 9.0.0: deleted
55-
// x64 msvc v19.14 (WINE): deleted
53+
// x86-64 gcc 9.2: not deleted
54+
// x86-64 clang 9.0.0: not deleted
55+
// x64 msvc v19.14 (WINE): not deleted
5656
char *func2(void) {
5757
char pw2[PW_SIZE];
5858
use_pw(pw2);
59-
memset(pw2, 1, PW_SIZE); // BAD [NOT DETECTED]
59+
memset(pw2, 1, PW_SIZE); // BAD
6060
return pw2;
6161
}
6262

@@ -96,7 +96,7 @@ int func6(void) {
9696
int func5(void) {
9797
char pw1a[PW_SIZE];
9898
use_pw(pw1a);
99-
__builtin_memset(pw1a + 3, 0, PW_SIZE - 4); // BAD
99+
__builtin_memset(pw1a + 3, 0, PW_SIZE - 4); // BAD [NOT DETECTED]
100100
return pw1a[4];
101101
}
102102

@@ -116,20 +116,16 @@ int func7(void) {
116116
int func8(void) {
117117
char pw1a[PW_SIZE];
118118
use_pw(pw1a);
119-
__builtin_memset(pw1a + pw1a[3], 0, PW_SIZE - 4); // GOOD [FALSE POSITIVE]
119+
__builtin_memset(pw1a + pw1a[3], 0, PW_SIZE - 4); // GOOD
120120
return pw1a[4];
121121
}
122122

123+
// x86-64 gcc 9.2: not deleted
124+
// x86-64 clang 9.0.0: not deleted
125+
// x64 msvc v19.14 (WINE): not deleted
123126
char *func9(void) {
124127
char pw1[PW_SIZE];
125128
use_pw(pw1);
126129
memset(pw1, 0, PW_SIZE); // BAD
127130
return 0;
128131
}
129-
130-
char *func10(void) {
131-
char pw1[PW_SIZE];
132-
use_pw(pw1);
133-
memset(pw1, 0, PW_SIZE); // GOOD
134-
return pw1;
135-
}

‎cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ void func3(unsigned long long sz) {
4848
// x64 msvc v19.22: deleted
4949
void func4(unsigned long long sz) {
5050
char buff[128];
51-
memset(buff, 0, PW_SIZE); // BAD [NOT DETECTED]
51+
memset(buff, 0, PW_SIZE); // BAD
5252
strcpy(buff, "Hello");
5353
}
5454

@@ -76,7 +76,7 @@ void func6(unsigned long long sz) {
7676
// x64 msvc v19.22: deleted
7777
void func7(unsigned long long sz) {
7878
struct mem m;
79-
memset(&m, 0, PW_SIZE); // BAD [NOT DETECTED]
79+
memset(&m, 0, PW_SIZE); // BAD
8080
m.a = 15;
8181
}
8282

@@ -85,7 +85,7 @@ void func7(unsigned long long sz) {
8585
// x64 msvc v19.22: not deleted
8686
void func8(unsigned long long sz) {
8787
struct mem *m = (struct mem *)malloc(sizeof(struct mem));
88-
memset(m, 0, PW_SIZE); // BAD
88+
memset(m, 0, PW_SIZE); // BAD [NOT DETECTED]
8989
}
9090

9191
// x86-64 gcc 9.2: deleted
@@ -112,7 +112,7 @@ void func10(unsigned long long sz) {
112112
// x64 msvc v19.22: not deleted
113113
void func11(unsigned long long sz) {
114114
struct mem *m = (struct mem *)malloc(sizeof(struct mem));
115-
::memset(m, 0, PW_SIZE); // BAD [ NOT DETECTED]
115+
::memset(m, 0, PW_SIZE); // BAD [NOT DETECTED]
116116
if (sz > 5) {
117117
strcpy(m->b, "Hello");
118118
}

‎cpp/ql/test/query-tests/Likely Bugs/Memory Management/MemsetMayBeDeleted/MemsetMayBeDeleted.expected‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
1-
| MemsetMayBeDeleted.c:19:2:19:7 | call to memset | Call to memset may be deleted by the compiler. |
2-
| MemsetMayBeDeleted.c:29:2:29:17 | call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. |
3-
| MemsetMayBeDeleted.c:79:2:79:17 | call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. |
4-
| MemsetMayBeDeleted.c:99:2:99:17 | call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. |
5-
| MemsetMayBeDeleted.c:109:2:109:17 | call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. |
6-
| MemsetMayBeDeleted.c:119:2:119:17 | call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. |
7-
| MemsetMayBeDeleted.cpp:43:5:43:10 | call to memset | Call to memset may be deleted by the compiler. |
8-
| MemsetMayBeDeleted.cpp:71:5:71:10 | call to memset | Call to memset may be deleted by the compiler. |
9-
| MemsetMayBeDeleted.cpp:88:5:88:10 | call to memset | Call to memset may be deleted by the compiler. |
1+
WARNING: Unused predicate insnDominates (/mnt/c/code/ql/cpp/ql/src/Likely Bugs/Memory Management/MemsetMayBeDeleted.ql:27,19-32)
2+
| MemsetMayBeDeleted.c:19:2:19:7 | Call: call to memset | Call to memset may be deleted by the compiler. |
3+
| MemsetMayBeDeleted.c:29:2:29:17 | Call: call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. |
4+
| MemsetMayBeDeleted.c:39:2:39:7 | Call: call to memset | Call to memset may be deleted by the compiler. |
5+
| MemsetMayBeDeleted.c:59:2:59:7 | Call: call to memset | Call to memset may be deleted by the compiler. |
6+
| MemsetMayBeDeleted.c:79:2:79:17 | Call: call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. |
7+
| MemsetMayBeDeleted.c:109:2:109:17 | Call: call to __builtin_memset | Call to __builtin_memset may be deleted by the compiler. |
8+
| MemsetMayBeDeleted.c:129:2:129:7 | Call: call to memset | Call to memset may be deleted by the compiler. |
9+
| MemsetMayBeDeleted.cpp:43:5:43:10 | Call: call to memset | Call to memset may be deleted by the compiler. |
10+
| MemsetMayBeDeleted.cpp:51:5:51:10 | Call: call to memset | Call to memset may be deleted by the compiler. |
11+
| MemsetMayBeDeleted.cpp:71:5:71:10 | Call: call to memset | Call to memset may be deleted by the compiler. |
12+
| MemsetMayBeDeleted.cpp:79:5:79:10 | Call: call to memset | Call to memset may be deleted by the compiler. |

0 commit comments

Comments
 (0)