Skip to content

Commit d7681bf

Browse files
jbjjbj
committed
C++: Don't use definitionByReference for data flow
The data flow library conflates pointers and objects enough for the `definitionByReference` predicate to be too strict in some cases. It was too permissive in other cases that are now (or will be) handled better by field flow. See also the change note entry.
1 parent e4d59c3 commit d7681bf

File tree

5 files changed

+44
-16
lines changed

5 files changed

+44
-16
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,3 +25,7 @@ The following changes in version 1.23 affect C/C++ analysis in all applications.
2525
picture of the partial flow paths from a given source. The feature is
2626
disabled by default and can be enabled for individual configurations by
2727
overriding `int explorationLimit()`.
28+
* The `DataFlow::DefinitionByReferenceNode` class now considers `f(x)` to be a
29+
definition of `x` when `x` is a variable of pointer type. It no longer
30+
considers deep paths such as `f(&x.myField)` to be definitions of `x`. These
31+
changes are in line with the user expectations we've observed.

‎cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ private module PartialDefinitions {
100100
)
101101
} or
102102
TExplicitCallQualifier(Expr qualifier, Call call) { qualifier = call.getQualifier() } or
103-
TReferenceArgument(Expr arg, VariableAccess va) { definitionByReference(va, arg) }
103+
TReferenceArgument(Expr arg, VariableAccess va) { referenceArgument(va, arg) }
104104

105105
private predicate isInstanceFieldWrite(FieldAccess fa, ControlFlowNode node) {
106106
not fa.getTarget().isStatic() and
@@ -138,18 +138,42 @@ private module PartialDefinitions {
138138
}
139139

140140
/**
141-
* A partial definition that's a definition by reference (in the sense of the
142-
* `definitionByReference` predicate).
141+
* A partial definition that's a definition by reference.
143142
*/
144143
class DefinitionByReference extends PartialDefinition, TReferenceArgument {
145144
VariableAccess va;
146145

147-
DefinitionByReference() { definitionByReference(va, definedExpr) }
146+
DefinitionByReference() { referenceArgument(va, definedExpr) }
148147

149148
VariableAccess getVariableAccess() { result = va }
150149

151150
override predicate partiallyDefines(Variable v) { va = v.getAnAccess() }
152151
}
152+
153+
private predicate referenceArgument(VariableAccess va, Expr argument) {
154+
argument = any(Call c).getAnArgument() and
155+
exists(Type argumentType |
156+
argumentType = argument.getFullyConverted().getType().stripTopLevelSpecifiers()
157+
|
158+
argumentType instanceof ReferenceType and
159+
not argumentType.(ReferenceType).getBaseType().isConst() and
160+
va = argument
161+
or
162+
argumentType instanceof PointerType and
163+
not argumentType.(PointerType).getBaseType().isConst() and
164+
(
165+
// f(variable)
166+
va = argument
167+
or
168+
// f(&variable)
169+
va = argument.(AddressOfExpr).getOperand()
170+
or
171+
// f(&array[0])
172+
va.getType().getUnspecifiedType() instanceof ArrayType and
173+
va = argument.(AddressOfExpr).getOperand().(ArrayExpr).getArrayBase()
174+
)
175+
)
176+
}
153177
}
154178
import PartialDefinitions
155179
private import FlowVar_internal

‎cpp/ql/test/library-tests/dataflow/fields/A.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ class A
129129
{
130130
B *b = new B();
131131
f7(b);
132-
sink(b->c); // flow [NOT DETECTED]
132+
sink(b->c); // flow
133133
}
134134

135135
class D
@@ -151,7 +151,7 @@ class A
151151
D *d = new D(b, r());
152152
sink(d->b); // flow x2
153153
sink(d->b->c); // flow
154-
sink(b->c); // flow [NOT DETECTED]
154+
sink(b->c); // flow
155155
}
156156

157157
void f10()

‎cpp/ql/test/library-tests/dataflow/fields/flow.expected‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,12 @@ edges
1616
| A.cpp:73:10:73:19 | call to setOnBWrap [c, ... (1)] | A.cpp:75:10:75:11 | b2 [c, ... (1)] |
1717
| A.cpp:73:25:73:32 | new [void] | A.cpp:73:10:73:19 | call to setOnBWrap [c, ... (1)] |
1818
| A.cpp:75:10:75:11 | b2 [c, ... (1)] | A.cpp:75:14:75:14 | c |
19-
| A.cpp:98:12:98:18 | new [void] | A.cpp:100:5:100:13 | ... = ... [void] |
20-
| A.cpp:100:5:100:6 | c1 [post update] [a, ... (1)] | A.cpp:101:8:101:9 | c1 [a, ... (1)] |
21-
| A.cpp:100:5:100:13 | ... = ... [void] | A.cpp:100:5:100:6 | c1 [post update] [a, ... (1)] |
22-
| A.cpp:101:8:101:9 | c1 [a, ... (1)] | A.cpp:103:14:103:14 | c [a, ... (1)] |
23-
| A.cpp:103:14:103:14 | c [a, ... (1)] | A.cpp:107:12:107:13 | c1 [a, ... (1)] |
24-
| A.cpp:103:14:103:14 | c [a, ... (1)] | A.cpp:120:12:120:13 | c1 [a, ... (1)] |
25-
| A.cpp:107:12:107:13 | c1 [a, ... (1)] | A.cpp:107:16:107:16 | a |
26-
| A.cpp:120:12:120:13 | c1 [a, ... (1)] | A.cpp:120:16:120:16 | a |
19+
| A.cpp:126:5:126:5 | b [post update] [c, ... (1)] | A.cpp:131:8:131:8 | ref arg b [c, ... (1)] |
20+
| A.cpp:126:12:126:18 | new [void] | A.cpp:126:5:126:5 | b [post update] [c, ... (1)] |
21+
| A.cpp:131:8:131:8 | ref arg b [c, ... (1)] | A.cpp:132:10:132:10 | b [c, ... (1)] |
22+
| A.cpp:132:10:132:10 | b [c, ... (1)] | A.cpp:132:13:132:13 | c |
2723
| A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | A.cpp:143:7:143:31 | ... = ... [c, ... (1)] |
24+
| A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | A.cpp:151:18:151:18 | ref arg b [c, ... (1)] |
2825
| A.cpp:142:7:142:20 | ... = ... [void] | A.cpp:142:7:142:7 | b [post update] [c, ... (1)] |
2926
| A.cpp:142:14:142:20 | new [void] | A.cpp:142:7:142:20 | ... = ... [void] |
3027
| A.cpp:143:7:143:10 | this [post update] [b, ... (1)] | A.cpp:151:12:151:24 | call to D [b, ... (1)] |
@@ -36,9 +33,11 @@ edges
3633
| A.cpp:151:12:151:24 | call to D [b, ... (1)] | A.cpp:152:10:152:10 | d [b, ... (1)] |
3734
| A.cpp:151:12:151:24 | call to D [b, ... (2)] | A.cpp:153:10:153:10 | d [b, ... (2)] |
3835
| A.cpp:151:18:151:18 | b [void] | A.cpp:151:12:151:24 | call to D [b, ... (1)] |
36+
| A.cpp:151:18:151:18 | ref arg b [c, ... (1)] | A.cpp:154:10:154:10 | b [c, ... (1)] |
3937
| A.cpp:152:10:152:10 | d [b, ... (1)] | A.cpp:152:13:152:13 | b |
4038
| A.cpp:153:10:153:10 | d [b, ... (2)] | A.cpp:153:13:153:13 | b [c, ... (1)] |
4139
| A.cpp:153:13:153:13 | b [c, ... (1)] | A.cpp:153:16:153:16 | c |
40+
| A.cpp:154:10:154:10 | b [c, ... (1)] | A.cpp:154:13:154:13 | c |
4241
| A.cpp:159:12:159:18 | new [void] | A.cpp:160:29:160:29 | b [void] |
4342
| A.cpp:160:18:160:60 | call to MyList [head, ... (1)] | A.cpp:161:38:161:39 | l1 [head, ... (1)] |
4443
| A.cpp:160:29:160:29 | b [void] | A.cpp:160:18:160:60 | call to MyList [head, ... (1)] |
@@ -170,11 +169,11 @@ edges
170169
| A.cpp:57:28:57:30 | call to get | A.cpp:57:17:57:23 | new [void] | A.cpp:57:28:57:30 | call to get | call to get flows from $@ | A.cpp:57:17:57:23 | new [void] | new [void] |
171170
| A.cpp:66:14:66:14 | c | A.cpp:64:21:64:28 | new [void] | A.cpp:66:14:66:14 | c | c flows from $@ | A.cpp:64:21:64:28 | new [void] | new [void] |
172171
| A.cpp:75:14:75:14 | c | A.cpp:73:25:73:32 | new [void] | A.cpp:75:14:75:14 | c | c flows from $@ | A.cpp:73:25:73:32 | new [void] | new [void] |
173-
| A.cpp:107:16:107:16 | a | A.cpp:98:12:98:18 | new [void] | A.cpp:107:16:107:16 | a | a flows from $@ | A.cpp:98:12:98:18 | new [void] | new [void] |
174-
| A.cpp:120:16:120:16 | a | A.cpp:98:12:98:18 | new [void] | A.cpp:120:16:120:16 | a | a flows from $@ | A.cpp:98:12:98:18 | new [void] | new [void] |
172+
| A.cpp:132:13:132:13 | c | A.cpp:126:12:126:18 | new [void] | A.cpp:132:13:132:13 | c | c flows from $@ | A.cpp:126:12:126:18 | new [void] | new [void] |
175173
| A.cpp:152:13:152:13 | b | A.cpp:143:25:143:31 | new [void] | A.cpp:152:13:152:13 | b | b flows from $@ | A.cpp:143:25:143:31 | new [void] | new [void] |
176174
| A.cpp:152:13:152:13 | b | A.cpp:150:12:150:18 | new [void] | A.cpp:152:13:152:13 | b | b flows from $@ | A.cpp:150:12:150:18 | new [void] | new [void] |
177175
| A.cpp:153:16:153:16 | c | A.cpp:142:14:142:20 | new [void] | A.cpp:153:16:153:16 | c | c flows from $@ | A.cpp:142:14:142:20 | new [void] | new [void] |
176+
| A.cpp:154:13:154:13 | c | A.cpp:142:14:142:20 | new [void] | A.cpp:154:13:154:13 | c | c flows from $@ | A.cpp:142:14:142:20 | new [void] | new [void] |
178177
| A.cpp:165:26:165:29 | head | A.cpp:159:12:159:18 | new [void] | A.cpp:165:26:165:29 | head | head flows from $@ | A.cpp:159:12:159:18 | new [void] | new [void] |
179178
| A.cpp:169:15:169:18 | head | A.cpp:159:12:159:18 | new [void] | A.cpp:169:15:169:18 | head | head flows from $@ | A.cpp:159:12:159:18 | new [void] | new [void] |
180179
| B.cpp:9:20:9:24 | elem1 | B.cpp:6:15:6:24 | new [void] | B.cpp:9:20:9:24 | elem1 | elem1 flows from $@ | B.cpp:6:15:6:24 | new [void] | new [void] |

‎cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@
133133
| taint.cpp:165:22:165:25 | {...} | taint.cpp:172:10:172:15 | buffer | |
134134
| taint.cpp:165:22:165:25 | {...} | taint.cpp:173:8:173:13 | buffer | |
135135
| taint.cpp:165:24:165:24 | 0 | taint.cpp:165:22:165:25 | {...} | TAINT |
136+
| taint.cpp:168:8:168:14 | ref arg tainted | taint.cpp:172:18:172:24 | tainted | |
136137
| taint.cpp:170:10:170:15 | buffer | taint.cpp:170:3:170:8 | call to strcpy | |
137138
| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:170:3:170:8 | call to strcpy | |
138139
| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:171:8:171:13 | buffer | |

0 commit comments

Comments
 (0)