C++: Add flow through arrays

This works by adding data-flow edges to skip over array expressions when
reading from arrays. On the post-update side, there was already code to
skip over array expressions when storing to arrays. That happens in
`valueToUpdate` in `AddressFlow.qll`, which needed just a small tweak to
support assignments with non-field expressions at the top-level LHS,
like `*a = ...` or `a[0] = ...`.

The new code in `AddressFlow.qll` is copy-pasted from `EscapesTree.qll`,
and there is already a note in these files saying that they share a lot
of code and must be maintained in sync.

Author: jbj

cpp/ql/src/semmle/code/cpp/dataflow/internal/AddressFlow.qll

@@ -29,7 +29,7 @@ private predicate stdIdentityFunction(Function f) { f.hasQualifiedName("std", ["
  */
 private predicate stdAddressOf(Function f) { f.hasQualifiedName("std", "addressof") }
 
-private predicate lvalueToLvalueStep(Expr lvalueIn, Expr lvalueOut) {
+private predicate lvalueToLvalueStepPure(Expr lvalueIn, Expr lvalueOut) {
   lvalueIn.getConversion() = lvalueOut.(ParenthesisExpr)
   or
   // When an object is implicitly converted to a reference to one of its base
@@ -42,6 +42,10 @@ private predicate lvalueToLvalueStep(Expr lvalueIn, Expr lvalueOut) {
   // such casts.
   lvalueIn.getConversion() = lvalueOut and
   lvalueOut.(CStyleCast).isImplicit()
+}
+
+private predicate lvalueToLvalueStep(Expr lvalueIn, Expr lvalueOut) {
+  lvalueToLvalueStepPure(lvalueIn, lvalueOut)
   or
   // C++ only
   lvalueIn = lvalueOut.(PrefixCrementOperation).getOperand().getFullyConverted()
@@ -214,6 +218,69 @@ private predicate referenceToUpdate(Expr reference, Expr outer, ControlFlowNode
   )
 }
 
+private predicate lvalueFromVariableAccess(VariableAccess va, Expr lvalue) {
+  // Base case for non-reference types.
+  lvalue = va and
+  not va.getConversion() instanceof ReferenceDereferenceExpr
+  or
+  // Base case for reference types where we pretend that they are
+  // non-reference types. The type of the target of `va` can be `ReferenceType`
+  // or `FunctionReferenceType`.
+  lvalue = va.getConversion().(ReferenceDereferenceExpr)
+  or
+  // lvalue -> lvalue
+  exists(Expr prev |
+    lvalueFromVariableAccess(va, prev) and
+    lvalueToLvalueStep(prev, lvalue)
+  )
+  or
+  // pointer -> lvalue
+  exists(Expr prev |
+    pointerFromVariableAccess(va, prev) and
+    pointerToLvalueStep(prev, lvalue)
+  )
+  or
+  // reference -> lvalue
+  exists(Expr prev |
+    referenceFromVariableAccess(va, prev) and
+    referenceToLvalueStep(prev, lvalue)
+  )
+}
+
+private predicate pointerFromVariableAccess(VariableAccess va, Expr pointer) {
+  // pointer -> pointer
+  exists(Expr prev |
+    pointerFromVariableAccess(va, prev) and
+    pointerToPointerStep(prev, pointer)
+  )
+  or
+  // reference -> pointer
+  exists(Expr prev |
+    referenceFromVariableAccess(va, prev) and
+    referenceToPointerStep(prev, pointer)
+  )
+  or
+  // lvalue -> pointer
+  exists(Expr prev |
+    lvalueFromVariableAccess(va, prev) and
+    lvalueToPointerStep(prev, pointer)
+  )
+}
+
+private predicate referenceFromVariableAccess(VariableAccess va, Expr reference) {
+  // reference -> reference
+  exists(Expr prev |
+    referenceFromVariableAccess(va, prev) and
+    referenceToReferenceStep(prev, reference)
+  )
+  or
+  // lvalue -> reference
+  exists(Expr prev |
+    lvalueFromVariableAccess(va, prev) and
+    lvalueToReferenceStep(prev, reference)
+  )
+}
+
 /**
  * Holds if `node` is a control-flow node that may modify `inner` (or what it
  * points to) through `outer`. The two expressions may be `Conversion`s. Plain
@@ -236,7 +303,7 @@ predicate valueToUpdate(Expr inner, Expr outer, ControlFlowNode node) {
   (
     inner instanceof VariableAccess and
     // Don't track non-field assignments
-    (assignmentTo(outer, _) implies inner instanceof FieldAccess)
+    not (assignmentTo(outer, _) and outer.(VariableAccess).getTarget() instanceof StackVariable)
     or
     inner instanceof ThisExpr
     or
@@ -245,3 +312,27 @@ predicate valueToUpdate(Expr inner, Expr outer, ControlFlowNode node) {
     // can't do anything useful with those at the moment.
   )
 }
+
+/**
+ * Holds if `e` is a fully-converted expression that evaluates to an lvalue
+ * derived from `va` and is used for reading from or assigning to. This is in
+ * contrast with a variable access that is used for taking an address (`&x`)
+ * or simply discarding its value (`x;`).
+ *
+ * This analysis does not propagate across assignments or calls, and unlike
+ * `variableAccessedAsValue` in `semmle.code.cpp.dataflow.EscapesTree` it
+ * propagates through array accesses but not field accesses. The analysis is
+ * also not concerned with whether the lvalue `e` is converted to an rvalue --
+ * to examine that, use the relevant member predicates on `Expr`.
+ *
+ * If `va` has reference type, the analysis concerns the value pointed to by
+ * the reference rather than the reference itself. The expression `e` may be a
+ * `Conversion`.
+ */
+predicate variablePartiallyAccessed(VariableAccess va, Expr e) {
+  lvalueFromVariableAccess(va, e) and
+  not lvalueToLvalueStepPure(e, _) and
+  not lvalueToPointerStep(e, _) and
+  not lvalueToReferenceStep(e, _) and
+  not e = any(ExprInVoidContext eivc | e = eivc.getConversion*())
+}

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

@@ -6,6 +6,7 @@ private import cpp
 private import semmle.code.cpp.dataflow.internal.FlowVar
 private import semmle.code.cpp.models.interfaces.DataFlow
 private import semmle.code.cpp.controlflow.Guards
+private import semmle.code.cpp.dataflow.internal.AddressFlow
 
 cached
 private newtype TNode =
@@ -610,6 +611,15 @@ private predicate exprToExprStep_nocfg(Expr fromExpr, Expr toExpr) {
   or
   toExpr.(AddressOfExpr).getOperand() = fromExpr
   or
+  // This rule enables flow from an array to its elements. Example: `a` to
+  // `a[i]` or `*a`, where `a` is an array type. It does not enable flow from a
+  // pointer to its indirection as in `p[i]` where `p` is a pointer type.
+  exists(Expr toConverted |
+    variablePartiallyAccessed(fromExpr, toConverted) and
+    toExpr = toConverted.getUnconverted() and
+    not toExpr = fromExpr
+  )
+  or
   toExpr.(BuiltInOperationBuiltInAddressOf).getOperand() = fromExpr
   or
   // The following case is needed to track the qualifier object for flow

cpp/ql/test/library-tests/dataflow/dataflow-tests/clang.cpp

@@ -48,6 +48,6 @@ void following_pointers(
 
   int stackArray[2] = { source(), source() };
   stackArray[0] = source();
-  sink(stackArray); // no flow
+  sink(stackArray); // flow
 }
 

cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected

@@ -13,6 +13,7 @@
 | example.c:24:13:24:18 | coords [post update] | example.c:26:19:26:24 | coords |
 | example.c:24:13:24:30 | ... = ... | example.c:24:2:24:30 | ... = ... |
 | example.c:24:13:24:30 | ... = ... | example.c:24:20:24:20 | y [post update] |
+| example.c:24:20:24:20 | y | example.c:24:13:24:30 | ... = ... |
 | example.c:24:24:24:30 | ... + ... | example.c:24:13:24:30 | ... = ... |
 | example.c:26:2:26:25 | ... = ... | example.c:26:9:26:9 | x [post update] |
 | example.c:26:13:26:16 | call to getX | example.c:26:2:26:25 | ... = ... |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp

@@ -428,7 +428,7 @@ void intPointerSourceCaller2() {
   int local[1];
   intPointerSource(local);
   sink(local); // tainted
-  sink(*local); // clean
+  sink(*local); // tainted
 }
 
 void intArraySourceCaller() {
@@ -441,7 +441,7 @@ void intArraySourceCaller2() {
   int local[2];
   intArraySource(local, 2);
   sink(local); // tainted
-  sink(*local); // clean
+  sink(*local); // tainted
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -468,5 +468,5 @@ void intOutparamSource(int *p) {
 void viaOutparam() {
   int x = 0;
   intOutparamSource(&x);
-  sink(x); // tainted [FALSE NEGATIVE]
+  sink(x); // tainted
 }

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected

@@ -16,6 +16,7 @@
 | clang.cpp:30:27:30:34 | call to getFirst | clang.cpp:28:27:28:32 | call to source |
 | clang.cpp:37:10:37:11 | m2 | clang.cpp:34:32:34:37 | call to source |
 | clang.cpp:45:17:45:18 | m2 | clang.cpp:43:35:43:40 | call to source |
+| clang.cpp:51:8:51:17 | stackArray | clang.cpp:50:19:50:24 | call to source |
 | dispatch.cpp:11:38:11:38 | x | dispatch.cpp:37:19:37:24 | call to source |
 | dispatch.cpp:11:38:11:38 | x | dispatch.cpp:45:18:45:23 | call to source |
 | dispatch.cpp:35:16:35:25 | call to notSource1 | dispatch.cpp:9:37:9:42 | call to source |
@@ -79,12 +80,17 @@
 | test.cpp:424:8:424:12 | local | test.cpp:423:20:423:25 | ref arg & ... |
 | test.cpp:430:8:430:12 | local | test.cpp:428:7:428:11 | local |
 | test.cpp:430:8:430:12 | local | test.cpp:429:20:429:24 | ref arg local |
+| test.cpp:431:8:431:13 | * ... | test.cpp:428:7:428:11 | local |
+| test.cpp:431:8:431:13 | * ... | test.cpp:429:20:429:24 | ref arg local |
 | test.cpp:437:8:437:12 | local | test.cpp:435:7:435:11 | local |
 | test.cpp:437:8:437:12 | local | test.cpp:436:18:436:23 | ref arg & ... |
 | test.cpp:443:8:443:12 | local | test.cpp:441:7:441:11 | local |
 | test.cpp:443:8:443:12 | local | test.cpp:442:18:442:22 | ref arg local |
+| test.cpp:444:8:444:13 | * ... | test.cpp:441:7:441:11 | local |
+| test.cpp:444:8:444:13 | * ... | test.cpp:442:18:442:22 | ref arg local |
 | test.cpp:450:9:450:22 | (statement expression) | test.cpp:449:26:449:32 | source1 |
 | test.cpp:461:8:461:12 | local | test.cpp:449:26:449:32 | source1 |
+| test.cpp:471:8:471:8 | x | test.cpp:465:8:465:13 | call to source |
 | true_upon_entry.cpp:21:8:21:8 | x | true_upon_entry.cpp:17:11:17:16 | call to source |
 | true_upon_entry.cpp:29:8:29:8 | x | true_upon_entry.cpp:27:9:27:14 | call to source |
 | true_upon_entry.cpp:39:8:39:8 | x | true_upon_entry.cpp:33:11:33:16 | call to source |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected

@@ -2,6 +2,7 @@
 | BarrierGuard.cpp:60:11:60:16 | BarrierGuard.cpp:62:14:62:14 | AST only |
 | clang.cpp:12:9:12:20 | clang.cpp:22:8:22:20 | AST only |
 | clang.cpp:39:42:39:47 | clang.cpp:41:18:41:19 | IR only |
+| clang.cpp:50:19:50:24 | clang.cpp:51:8:51:17 | AST only |
 | dispatch.cpp:16:37:16:42 | dispatch.cpp:32:16:32:24 | IR only |
 | dispatch.cpp:16:37:16:42 | dispatch.cpp:40:15:40:23 | IR only |
 | dispatch.cpp:22:37:22:42 | dispatch.cpp:31:16:31:24 | IR only |
@@ -41,11 +42,16 @@
 | test.cpp:422:7:422:11 | test.cpp:424:8:424:12 | AST only |
 | test.cpp:423:20:423:25 | test.cpp:424:8:424:12 | AST only |
 | test.cpp:428:7:428:11 | test.cpp:430:8:430:12 | AST only |
+| test.cpp:428:7:428:11 | test.cpp:431:8:431:13 | AST only |
 | test.cpp:429:20:429:24 | test.cpp:430:8:430:12 | AST only |
+| test.cpp:429:20:429:24 | test.cpp:431:8:431:13 | AST only |
 | test.cpp:435:7:435:11 | test.cpp:437:8:437:12 | AST only |
 | test.cpp:436:18:436:23 | test.cpp:437:8:437:12 | AST only |
 | test.cpp:441:7:441:11 | test.cpp:443:8:443:12 | AST only |
+| test.cpp:441:7:441:11 | test.cpp:444:8:444:13 | AST only |
 | test.cpp:442:18:442:22 | test.cpp:443:8:443:12 | AST only |
+| test.cpp:442:18:442:22 | test.cpp:444:8:444:13 | AST only |
+| test.cpp:465:8:465:13 | test.cpp:471:8:471:8 | AST only |
 | true_upon_entry.cpp:9:11:9:16 | true_upon_entry.cpp:13:8:13:8 | IR only |
 | true_upon_entry.cpp:62:11:62:16 | true_upon_entry.cpp:66:8:66:8 | IR only |
 | true_upon_entry.cpp:98:11:98:16 | true_upon_entry.cpp:105:8:105:8 | IR only |

cpp/ql/test/library-tests/dataflow/fields/arrays.cpp

@@ -4,17 +4,17 @@ void *user_input(void);
 void local_array() {
   void *arr[10] = { 0 };
   arr[0] = user_input();
-  sink(arr[0]); // $ir $f-:ast
-  sink(arr[1]);
-  sink(*arr); // $ir $f-:ast
-  sink(*&arr[0]); // $ir $f-:ast
+  sink(arr[0]); // $ast,ir
+  sink(arr[1]); // $f+:ast
+  sink(*arr); // $ast,ir
+  sink(*&arr[0]); // $ast,ir
 }
 
 void local_array_convoluted_assign() {
   void *arr[10] = { 0 };
   *&arr[0] = user_input();
-  sink(arr[0]); // $ir $f-:ast
-  sink(arr[1]);
+  sink(arr[0]); // $ast,ir
+  sink(arr[1]); // $f+:ast
 }
 
 struct inner {
@@ -34,14 +34,14 @@ struct outer {
 
 void nested_array_1(outer o) {
   o.nested.arr[1].data = user_input();
-  sink(o.nested.arr[1].data); // $ir $f-:ast
-  sink(o.nested.arr[0].data);
+  sink(o.nested.arr[1].data); // $ast,ir
+  sink(o.nested.arr[0].data); // $f+:ast
 }
 
 void nested_array_2(outer o) {
   o.indirect->arr[1].data = user_input();
-  sink(o.indirect->arr[1].data); // $f-:ast,ir
-  sink(o.indirect->arr[0].data);
+  sink(o.indirect->arr[1].data); // $ast $f-:ir
+  sink(o.indirect->arr[0].data); // $f+:ast
 }
 
 void nested_array_3(outer o) {

cpp/ql/test/library-tests/dataflow/fields/by_reference.cpp

@@ -109,11 +109,11 @@ void test_outer_with_ptr(Outer *pouter) {
 
   sink(outer.inner_nested.a); // $ast,ir
   sink(outer.inner_ptr->a); // $ast $f-:ir
-  sink(outer.a); // $f-:ast,ir
+  sink(outer.a); // $ast $f-:ir
 
   sink(pouter->inner_nested.a); // $ast,ir
   sink(pouter->inner_ptr->a); // $ast $f-:ir
-  sink(pouter->a); // $f-:ast,ir
+  sink(pouter->a); // $ast $f-:ir
 }
 
 void test_outer_with_ref(Outer *pouter) {

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

@@ -21,15 +21,17 @@
 | aliasing.cpp:79:11:79:20 | call to user_input | aliasing.cpp:80:12:80:13 | m1 | IR only |
 | aliasing.cpp:86:10:86:19 | call to user_input | aliasing.cpp:87:12:87:13 | m1 | IR only |
 | aliasing.cpp:98:10:98:19 | call to user_input | aliasing.cpp:102:8:102:10 | * ... | IR only |
-| arrays.cpp:6:12:6:21 | call to user_input | arrays.cpp:7:8:7:13 | access to array | IR only |
-| arrays.cpp:6:12:6:21 | call to user_input | arrays.cpp:9:8:9:11 | * ... | IR only |
-| arrays.cpp:6:12:6:21 | call to user_input | arrays.cpp:10:8:10:15 | * ... | IR only |
-| arrays.cpp:15:14:15:23 | call to user_input | arrays.cpp:16:8:16:13 | access to array | IR only |
-| arrays.cpp:36:26:36:35 | call to user_input | arrays.cpp:37:24:37:27 | data | IR only |
+| arrays.cpp:6:12:6:21 | call to user_input | arrays.cpp:8:8:8:13 | access to array | AST only |
+| arrays.cpp:15:14:15:23 | call to user_input | arrays.cpp:17:8:17:13 | access to array | AST only |
+| arrays.cpp:36:26:36:35 | call to user_input | arrays.cpp:38:24:38:27 | data | AST only |
+| arrays.cpp:42:29:42:38 | call to user_input | arrays.cpp:43:27:43:30 | data | AST only |
+| arrays.cpp:42:29:42:38 | call to user_input | arrays.cpp:44:27:44:30 | data | AST only |
 | by_reference.cpp:84:14:84:23 | call to user_input | by_reference.cpp:111:25:111:25 | a | AST only |
 | by_reference.cpp:84:14:84:23 | call to user_input | by_reference.cpp:115:27:115:27 | a | AST only |
 | by_reference.cpp:88:13:88:22 | call to user_input | by_reference.cpp:131:25:131:25 | a | AST only |
 | by_reference.cpp:88:13:88:22 | call to user_input | by_reference.cpp:135:27:135:27 | a | AST only |
+| by_reference.cpp:92:9:92:18 | call to user_input | by_reference.cpp:112:14:112:14 | a | AST only |
+| by_reference.cpp:92:9:92:18 | call to user_input | by_reference.cpp:116:16:116:16 | a | AST only |
 | by_reference.cpp:96:8:96:17 | call to user_input | by_reference.cpp:132:14:132:14 | a | AST only |
 | by_reference.cpp:96:8:96:17 | call to user_input | by_reference.cpp:136:16:136:16 | a | AST only |
 | complex.cpp:62:19:62:28 | call to user_input | complex.cpp:52:18:52:18 | call to b | AST only |