diff --git a/change-notes/1.23/analysis-cpp.md b/change-notes/1.23/analysis-cpp.md index 680ee1259ad0..ce317236f93d 100644 --- a/change-notes/1.23/analysis-cpp.md +++ b/change-notes/1.23/analysis-cpp.md @@ -28,6 +28,10 @@ The following changes in version 1.23 affect C/C++ analysis in all applications. picture of the partial flow paths from a given source. The feature is disabled by default and can be enabled for individual configurations by overriding `int explorationLimit()`. +* The `DataFlow::DefinitionByReferenceNode` class now considers `f(x)` to be a + definition of `x` when `x` is a variable of pointer type. It no longer + considers deep paths such as `f(&x.myField)` to be definitions of `x`. These + changes are in line with the user expectations we've observed. * There is now a `DataFlow::localExprFlow` predicate and a `TaintTracking::localExprTaint` predicate to make it easy to use the most common case of local data flow and taint: from one `Expr` to another. diff --git a/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll b/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll index d84e69e8bb3c..74e96d8d0633 100644 --- a/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll +++ b/cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll @@ -105,7 +105,7 @@ private module PartialDefinitions { ) } or TExplicitCallQualifier(Expr qualifier, Call call) { qualifier = call.getQualifier() } or - TReferenceArgument(Expr arg, VariableAccess va) { definitionByReference(va, arg) } + TReferenceArgument(Expr arg, VariableAccess va) { referenceArgument(va, arg) } private predicate isInstanceFieldWrite(FieldAccess fa, ControlFlowNode node) { not fa.getTarget().isStatic() and @@ -154,18 +154,48 @@ private module PartialDefinitions { } /** - * A partial definition that's a definition by reference (in the sense of the - * `definitionByReference` predicate). + * A partial definition that's a definition by reference. */ class DefinitionByReference extends PartialDefinition, TReferenceArgument { VariableAccess va; - DefinitionByReference() { definitionByReference(va, definedExpr) } + DefinitionByReference() { + // `this` is not restricted in this charpred. That's because the full + // extent of this class includes the charpred of the superclass, which + // relates `this` to `definedExpr`, and `va` is functionally determined + // by `definedExpr`. + referenceArgument(va, definedExpr) + } VariableAccess getVariableAccess() { result = va } override predicate partiallyDefines(Variable v) { va = v.getAnAccess() } } + + private predicate referenceArgument(VariableAccess va, Expr argument) { + argument = any(Call c).getAnArgument() and + exists(Type argumentType | + argumentType = argument.getFullyConverted().getType().stripTopLevelSpecifiers() + | + argumentType instanceof ReferenceType and + not argumentType.(ReferenceType).getBaseType().isConst() and + va = argument + or + argumentType instanceof PointerType and + not argumentType.(PointerType).getBaseType().isConst() and + ( + // f(variable) + va = argument + or + // f(&variable) + va = argument.(AddressOfExpr).getOperand() + or + // f(&array[0]) + va.getType().getUnspecifiedType() instanceof ArrayType and + va = argument.(AddressOfExpr).getOperand().(ArrayExpr).getArrayBase() + ) + ) + } } import PartialDefinitions private import FlowVar_internal @@ -236,7 +266,7 @@ module FlowVar_internal { not v instanceof Field and // Fields are interprocedural data flow, not local reachable(sbb) and ( - initializer(sbb.getANode(), v, _) + initializer(v, sbb.getANode()) or assignmentLikeOperation(sbb, v, _, _) or @@ -353,7 +383,12 @@ module FlowVar_internal { assignmentLikeOperation(node, v, _, e) and node = sbb or - initializer(node, v, e) and + // We pick the defining `ControlFlowNode` of an `Initializer` to be its + // expression rather than the `Initializer` itself. That's because the + // `Initializer` of a `ConditionDeclExpr` is for historical reasons not + // part of the CFG and therefore ends up in the wrong basic block. + initializer(v, e) and + node = e and node = sbb.getANode() } @@ -711,13 +746,11 @@ module FlowVar_internal { } /** - * Holds if `v` is initialized by `init` to have value `assignedExpr`. + * Holds if `v` is initialized to have value `assignedExpr`. */ - predicate initializer( - Initializer init, LocalVariable v, Expr assignedExpr) + predicate initializer(LocalVariable v, Expr assignedExpr) { - v = init.getDeclaration() and - assignedExpr = init.getExpr() + assignedExpr = v.getInitializer().getExpr() } /** diff --git a/cpp/ql/test/library-tests/dataflow/fields/A.cpp b/cpp/ql/test/library-tests/dataflow/fields/A.cpp index 7668db1fbbcc..fd49e1e85ee2 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/A.cpp +++ b/cpp/ql/test/library-tests/dataflow/fields/A.cpp @@ -129,7 +129,7 @@ class A { B *b = new B(); f7(b); - sink(b->c); // flow [NOT DETECTED] + sink(b->c); // flow } class D @@ -151,7 +151,7 @@ class A D *d = new D(b, r()); sink(d->b); // flow x2 sink(d->b->c); // flow - sink(b->c); // flow [NOT DETECTED] + sink(b->c); // flow } void f10() diff --git a/cpp/ql/test/library-tests/dataflow/fields/flow.expected b/cpp/ql/test/library-tests/dataflow/fields/flow.expected index 4a90cccf0903..d0cc1ee05ebc 100644 --- a/cpp/ql/test/library-tests/dataflow/fields/flow.expected +++ b/cpp/ql/test/library-tests/dataflow/fields/flow.expected @@ -24,7 +24,12 @@ edges | A.cpp:103:14:103:14 | c [a, ... (1)] | A.cpp:120:12:120:13 | c1 [a, ... (1)] | | A.cpp:107:12:107:13 | c1 [a, ... (1)] | A.cpp:107:16:107:16 | a | | A.cpp:120:12:120:13 | c1 [a, ... (1)] | A.cpp:120:16:120:16 | a | +| A.cpp:126:5:126:5 | b [post update] [c, ... (1)] | A.cpp:131:8:131:8 | ref arg b [c, ... (1)] | +| A.cpp:126:12:126:18 | new [void] | A.cpp:126:5:126:5 | b [post update] [c, ... (1)] | +| A.cpp:131:8:131:8 | ref arg b [c, ... (1)] | A.cpp:132:10:132:10 | b [c, ... (1)] | +| A.cpp:132:10:132:10 | b [c, ... (1)] | A.cpp:132:13:132:13 | c | | A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | A.cpp:143:7:143:31 | ... = ... [c, ... (1)] | +| A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | A.cpp:151:18:151:18 | ref arg b [c, ... (1)] | | A.cpp:142:7:142:20 | ... = ... [void] | A.cpp:142:7:142:7 | b [post update] [c, ... (1)] | | A.cpp:142:14:142:20 | new [void] | A.cpp:142:7:142:20 | ... = ... [void] | | A.cpp:143:7:143:10 | this [post update] [b, ... (1)] | A.cpp:151:12:151:24 | call to D [b, ... (1)] | @@ -36,9 +41,11 @@ edges | A.cpp:151:12:151:24 | call to D [b, ... (1)] | A.cpp:152:10:152:10 | d [b, ... (1)] | | A.cpp:151:12:151:24 | call to D [b, ... (2)] | A.cpp:153:10:153:10 | d [b, ... (2)] | | A.cpp:151:18:151:18 | b [void] | A.cpp:151:12:151:24 | call to D [b, ... (1)] | +| A.cpp:151:18:151:18 | ref arg b [c, ... (1)] | A.cpp:154:10:154:10 | b [c, ... (1)] | | A.cpp:152:10:152:10 | d [b, ... (1)] | A.cpp:152:13:152:13 | b | | A.cpp:153:10:153:10 | d [b, ... (2)] | A.cpp:153:13:153:13 | b [c, ... (1)] | | A.cpp:153:13:153:13 | b [c, ... (1)] | A.cpp:153:16:153:16 | c | +| A.cpp:154:10:154:10 | b [c, ... (1)] | A.cpp:154:13:154:13 | c | | A.cpp:159:12:159:18 | new [void] | A.cpp:160:29:160:29 | b [void] | | A.cpp:160:18:160:60 | call to MyList [head, ... (1)] | A.cpp:161:38:161:39 | l1 [head, ... (1)] | | A.cpp:160:29:160:29 | b [void] | A.cpp:160:18:160:60 | call to MyList [head, ... (1)] | @@ -178,9 +185,11 @@ edges | 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] | | 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] | | 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] | +| 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] | | 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] | | 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] | | 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] | +| 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] | | 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] | | 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] | | 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] | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index fddfb2a8a49d..4895cf711126 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -133,6 +133,7 @@ | taint.cpp:165:22:165:25 | {...} | taint.cpp:172:10:172:15 | buffer | | | taint.cpp:165:22:165:25 | {...} | taint.cpp:173:8:173:13 | buffer | | | taint.cpp:165:24:165:24 | 0 | taint.cpp:165:22:165:25 | {...} | TAINT | +| taint.cpp:168:8:168:14 | ref arg tainted | taint.cpp:172:18:172:24 | tainted | | | taint.cpp:170:10:170:15 | buffer | taint.cpp:170:3:170:8 | call to strcpy | | | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:170:3:170:8 | call to strcpy | | | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:171:8:171:13 | buffer | |