Skip to content

Commit d65c52d

Browse files
MathiasVPMathiasVP
committed
Merge branch 'master' into ir-flow-fields
2 parents 52b179a + 42e9d14 commit d65c52d

File tree

12 files changed

+243
-33
lines changed

12 files changed

+243
-33
lines changed

‎change-notes/1.24/analysis-cpp.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
2424
| No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
2525
| Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) | | This query is no longer run on LGTM. |
2626
| No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings. This approach produces fewer, more accurate results. |
27+
| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | Cases where the tainted allocation size is range checked are now more reliably excluded. |
2728
| Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |
2829
| Unsafe array for days of the year (`cpp/leap-year/unsafe-array-for-days-of-the-year`) | | This query is no longer run on LGTM. |
2930
| Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | More correct results | This query now also looks for comparisons of the form `0 <= x`. |
@@ -53,3 +54,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
5354
* The library now models data flow through formatting functions such as `sprintf`.
5455
* The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) uses a new intermediate representation. This provides a more precise analysis of pointers to stack variables and flow through parameters, improving the results of many security queries.
5556
* The global value numbering library (`semmle.code.cpp.valuenumbering.GlobalValueNumbering`) uses a new intermediate representation to provide a more precise analysis of heap allocated memory and pointers to stack variables.
57+
* `freeCall` in `semmle.code.cpp.commons.Alloc` has been deprecated. The`Allocation` and `Deallocation` models in `semmle.code.cpp.models.interfaces` should be used instead.

‎cpp/ql/src/Critical/MemoryFreed.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ private predicate freed(Expr e) {
44
e = any(DeallocationExpr de).getFreedExpr()
55
or
66
exists(ExprCall c |
7-
// cautiously assume that any ExprCall could be a freeCall.
7+
// cautiously assume that any `ExprCall` could be a deallocation expression.
88
c.getAnArgument() = e
99
)
1010
}

‎cpp/ql/src/Critical/NewDelete.qll

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,34 @@
55
import cpp
66
import semmle.code.cpp.controlflow.SSA
77
import semmle.code.cpp.dataflow.DataFlow
8+
import semmle.code.cpp.models.implementations.Allocation
9+
import semmle.code.cpp.models.implementations.Deallocation
810

911
/**
1012
* Holds if `alloc` is a use of `malloc` or `new`. `kind` is
1113
* a string describing the type of the allocation.
1214
*/
1315
predicate allocExpr(Expr alloc, string kind) {
14-
isAllocationExpr(alloc) and
15-
not alloc.isFromUninstantiatedTemplate(_) and
1616
(
17-
alloc instanceof FunctionCall and
18-
kind = "malloc"
17+
exists(Function target |
18+
alloc.(AllocationExpr).(FunctionCall).getTarget() = target and
19+
(
20+
target.getName() = "operator new" and
21+
kind = "new" and
22+
// exclude placement new and custom overloads as they
23+
// may not conform to assumptions
24+
not target.getNumberOfParameters() > 1
25+
or
26+
target.getName() = "operator new[]" and
27+
kind = "new[]" and
28+
// exclude placement new and custom overloads as they
29+
// may not conform to assumptions
30+
not target.getNumberOfParameters() > 1
31+
or
32+
not target instanceof OperatorNewAllocationFunction and
33+
kind = "malloc"
34+
)
35+
)
1936
or
2037
alloc instanceof NewExpr and
2138
kind = "new" and
@@ -28,7 +45,8 @@ predicate allocExpr(Expr alloc, string kind) {
2845
// exclude placement new and custom overloads as they
2946
// may not conform to assumptions
3047
not alloc.(NewArrayExpr).getAllocatorCall().getTarget().getNumberOfParameters() > 1
31-
)
48+
) and
49+
not alloc.isFromUninstantiatedTemplate(_)
3250
}
3351

3452
/**
@@ -110,8 +128,20 @@ predicate allocReaches(Expr e, Expr alloc, string kind) {
110128
* describing the type of that free or delete.
111129
*/
112130
predicate freeExpr(Expr free, Expr freed, string kind) {
113-
freeCall(free, freed) and
114-
kind = "free"
131+
exists(Function target |
132+
freed = free.(DeallocationExpr).getFreedExpr() and
133+
free.(FunctionCall).getTarget() = target and
134+
(
135+
target.getName() = "operator delete" and
136+
kind = "delete"
137+
or
138+
target.getName() = "operator delete[]" and
139+
kind = "delete[]"
140+
or
141+
not target instanceof OperatorDeleteDeallocationFunction and
142+
kind = "free"
143+
)
144+
)
115145
or
116146
free.(DeleteExpr).getExpr() = freed and
117147
kind = "delete"

‎cpp/ql/src/semmle/code/cpp/commons/Alloc.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ predicate freeFunction(Function f, int argNum) { argNum = f.(DeallocationFunctio
2323

2424
/**
2525
* A call to a library routine that frees memory.
26+
*
27+
* DEPRECATED: Use `DeallocationExpr` instead (this also includes `delete` expressions).
2628
*/
2729
predicate freeCall(FunctionCall fc, Expr arg) { arg = fc.(DeallocationExpr).getFreedExpr() }
2830

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,17 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
144144
}
145145

146146
/**
147-
* A variable that has any kind of upper-bound check anywhere in the program
147+
* A variable that has any kind of upper-bound check anywhere in the program. This is
148+
* biased towards being inclusive because there are a lot of valid ways of doing an
149+
* upper bounds checks if we don't consider where it occurs, for example:
150+
* ```
151+
* if (x < 10) { sink(x); }
152+
*
153+
* if (10 > y) { sink(y); }
154+
*
155+
* if (z > 10) { z = 10; }
156+
* sink(z);
157+
* ```
148158
*/
149159
// TODO: This coarse overapproximation, ported from the old taint tracking
150160
// library, could be replaced with an actual semantic check that a particular
@@ -153,10 +163,10 @@ private predicate writesVariable(StoreInstruction store, Variable var) {
153163
// previously suppressed by this predicate by coincidence.
154164
private predicate hasUpperBoundsCheck(Variable var) {
155165
exists(RelationalOperation oper, VariableAccess access |
156-
oper.getLeftOperand() = access and
166+
oper.getAnOperand() = access and
157167
access.getTarget() = var and
158168
// Comparing to 0 is not an upper bound check
159-
not oper.getRightOperand().getValue() = "0"
169+
not oper.getAnOperand().getValue() = "0"
160170
)
161171
}
162172

‎cpp/ql/src/semmle/code/cpp/models/implementations/Allocation.qll

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
/**
2+
* Provides implementation classes modelling various methods of allocation
3+
* (`malloc`, `new` etc). See `semmle.code.cpp.models.interfaces.Allocation`
4+
* for usage information.
5+
*/
6+
17
import semmle.code.cpp.models.interfaces.Allocation
28

39
/**

‎cpp/ql/src/semmle/code/cpp/models/implementations/Deallocation.qll

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
import semmle.code.cpp.models.interfaces.Allocation
1+
/**
2+
* Provides implementation classes modelling various methods of deallocation
3+
* (`free`, `delete` etc). See `semmle.code.cpp.models.interfaces.Deallocation`
4+
* for usage information.
5+
*/
6+
7+
import semmle.code.cpp.models.interfaces.Deallocation
28

39
/**
410
* A deallocation function such as `free`.

‎cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -328,14 +328,24 @@ GlobalOrNamespaceVariable globalVarFromId(string id) {
328328
}
329329

330330
/**
331-
* A variable that has any kind of upper-bound check anywhere in the program
331+
* A variable that has any kind of upper-bound check anywhere in the program. This is
332+
* biased towards being inclusive because there are a lot of valid ways of doing an
333+
* upper bounds checks if we don't consider where it occurs, for example:
334+
* ```
335+
* if (x < 10) { sink(x); }
336+
*
337+
* if (10 > y) { sink(y); }
338+
*
339+
* if (z > 10) { z = 10; }
340+
* sink(z);
341+
* ```
332342
*/
333343
private predicate hasUpperBoundsCheck(Variable var) {
334344
exists(RelationalOperation oper, VariableAccess access |
335-
oper.getLeftOperand() = access and
345+
oper.getAnOperand() = access and
336346
access.getTarget() = var and
337347
// Comparing to 0 is not an upper bound check
338-
not oper.getRightOperand().getValue() = "0"
348+
not oper.getAnOperand().getValue() = "0"
339349
)
340350
}
341351

‎cpp/ql/test/query-tests/Critical/NewFree/NewFreeMismatch.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
| test2.cpp:19:3:19:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:18:12:18:18 | new | new |
22
| test2.cpp:26:3:26:6 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:25:7:25:13 | new | new |
3+
| test2.cpp:51:2:51:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:45:18:45:24 | new | new |
4+
| test2.cpp:55:2:55:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test2.cpp:46:20:46:33 | call to operator new | new |
5+
| test2.cpp:57:2:57:18 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc |
6+
| test2.cpp:58:2:58:18 | call to operator delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test2.cpp:47:21:47:26 | call to malloc | malloc |
37
| test.cpp:36:2:36:17 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:27:18:27:23 | call to malloc | malloc |
48
| test.cpp:41:2:41:5 | call to free | There is a new/free mismatch between this free and the corresponding $@. | test.cpp:26:7:26:17 | new | new |
59
| test.cpp:68:3:68:11 | delete | There is a malloc/delete mismatch between this delete and the corresponding $@. | test.cpp:64:28:64:33 | call to malloc | malloc |

‎cpp/ql/test/query-tests/Critical/NewFree/test2.cpp

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,3 +34,27 @@ class MyTest2Class
3434
};
3535

3636
MyTest2Class<int> mt2c_i;
37+
38+
// ---
39+
40+
void* operator new(size_t);
41+
void operator delete(void*);
42+
43+
void test_operator_new()
44+
{
45+
void *ptr_new = new int;
46+
void *ptr_opnew = ::operator new(sizeof(int));
47+
void *ptr_malloc = malloc(sizeof(int));
48+
49+
delete ptr_new; // GOOD
50+
::operator delete(ptr_new); // GOOD
51+
free(ptr_new); // BAD
52+
53+
delete ptr_opnew; // GOOD
54+
::operator delete(ptr_opnew); // GOOD
55+
free(ptr_opnew); // BAD
56+
57+
delete ptr_malloc; // BAD
58+
::operator delete(ptr_malloc); // BAD
59+
free(ptr_malloc); // GOOD
60+
}

0 commit comments

Comments
 (0)