Merge pull request #1777 from jbj/ast-field-flow-defbyref

C++: Don't use definitionByReference for data flow

Author: Robert Marsh

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.

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()
   }
 
   /**

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()

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] |

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 |  |