C++: use(x) is no longer an array read.

Author: MathiasVP

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

@@ -357,37 +357,48 @@ private class ArrayToPointerConvertInstruction extends ConvertInstruction {
 
 private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
   a = TArrayContent() and
-  (
-    // In cases such as:
-    // ```cpp
-    // void f(int* pa) {
-    //   *pa = source();
-    // }
-    // ...
-    // int x;
-    // f(&x);
-    // use(x);
-    // ```
-    // the load on `x` in `use(x)` will exactly overlap with its definition (in this case the definition
-    // is a `BufferMayWriteSideEffect`).
-    exists(LoadInstruction load |
-      node1.asInstruction() = load.getSourceValue() and
-      load = node2.asInstruction()
-    )
-    or
-    // Explicit dereferences such as `*p` or `p[i]` where `p` is a pointer or array.
-    exists(LoadInstruction load |
-      node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
-      load = node2.asInstruction() and
-      (
-        load.getSourceAddress() instanceof LoadInstruction or
-        load.getSourceAddress() instanceof ArrayToPointerConvertInstruction or
-        load.getSourceAddress() instanceof PointerAddInstruction
-      )
+  // Explicit dereferences such as `*p` or `p[i]` where `p` is a pointer or array.
+  exists(LoadInstruction load |
+    load.getSourceValueOperand().isDefinitionInexact() and
+    node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
+    load = node2.asInstruction() and
+    (
+      load.getSourceAddress() instanceof LoadInstruction or
+      load.getSourceAddress() instanceof ArrayToPointerConvertInstruction or
+      load.getSourceAddress() instanceof PointerAddInstruction
     )
   )
 }
 
+/**
+ * In cases such as:
+ * ```cpp
+ * void f(int* pa) {
+ *   *pa = source();
+ * }
+ * ...
+ * int x;
+ * f(&x);
+ * use(x);
+ * ```
+ * the load on `x` in `use(x)` will exactly overlap with its definition (in this case the definition
+ * is a `BufferMayWriteSideEffect`). This predicate pops the `ArrayContent` (pushed by the store in `f`)
+ * from the access path.
+ */
+private predicate exactReadStep(Node node1, ArrayContent a, Node node2) {
+  a = TArrayContent() and
+  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi |
+    not chi.isResultConflated() and
+    chi.getPartial() = write and
+    node1.asInstruction() = write and
+    node2.asInstruction() = chi and
+    // To distinquish this case from the `arrayReadStep` case we require that the entire variable was
+    // overwritten by the `BufferMayWriteSideEffectInstruction` (i.e., there is a load that reads the
+    // entire variable).
+    exists(LoadInstruction load | load.getSourceValue() = chi)
+  )
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` via a read of `f`.
  * Thus, `node1` references an object with a field `f` whose value ends up in
@@ -396,6 +407,7 @@ private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
 predicate readStep(Node node1, Content f, Node node2) {
   fieldReadStep(node1, f, node2) or
   arrayReadStep(node1, f, node2) or
+  exactReadStep(node1, f, node2) or
   suppressArrayRead(node1, f, node2)
 }
 

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

@@ -93,6 +93,7 @@
 | defaulttainttracking.cpp:88:18:88:23 | call to getenv | shared.h:5:23:5:31 | sinkparam |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:91:42:91:44 | arg |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:12:92:14 | arg |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:96:11:96:12 | p2 |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:97:27:97:32 | call to getenv |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | (const char *)... |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 |

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

@@ -16,6 +16,7 @@
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:91:31:91:33 | ret | AST only |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:5:92:8 | * ... | AST only |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:92:6:92:8 | ret | AST only |
+| defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:96:11:96:12 | p2 | IR only |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | (const char *)... | IR only |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 | IR only |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | shared.h:5:23:5:31 | sinkparam | IR only |