Merge pull request #4302 from MathiasVP/fix-field-conflation-after-4230

C++: Fix field conflation after #4230

Author: jbj

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

@@ -211,10 +211,17 @@ private predicate fieldStoreStepNoChi(Node node1, FieldContent f, PostUpdateNode
   )
 }
 
+private FieldAddressInstruction getFieldInstruction(Instruction instr) {
+  result = instr or
+  result = instr.(CopyValueInstruction).getUnary()
+}
+
 pragma[noinline]
-private predicate getWrittenField(StoreInstruction store, Field f, Class c) {
+private predicate getWrittenField(Instruction instr, Field f, Class c) {
   exists(FieldAddressInstruction fa |
-    fa = store.getDestinationAddress() and
+    fa =
+      getFieldInstruction([instr.(StoreInstruction).getDestinationAddress(),
+            instr.(WriteSideEffectInstruction).getDestinationAddress()]) and
     f = fa.getField() and
     c = f.getDeclaringType()
   )
@@ -265,7 +272,23 @@ private predicate arrayStoreStepChi(Node node1, ArrayContent a, PostUpdateNode n
 predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
   fieldStoreStepNoChi(node1, f, node2) or
   fieldStoreStepChi(node1, f, node2) or
-  arrayStoreStepChi(node1, f, node2)
+  arrayStoreStepChi(node1, f, node2) or
+  fieldStoreStepAfterArraySuppression(node1, f, node2)
+}
+
+// This predicate pushes the correct `FieldContent` onto the access path when the
+// `suppressArrayRead` predicate has popped off an `ArrayContent`.
+private predicate fieldStoreStepAfterArraySuppression(
+  Node node1, FieldContent f, PostUpdateNode node2
+) {
+  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi, Class c |
+    not chi.isResultConflated() and
+    node1.asInstruction() = chi and
+    node2.asInstruction() = chi and
+    chi.getPartial() = write and
+    getWrittenField(write, f.getAField(), c) and
+    f.hasOffset(c, _, _)
+  )
 }
 
 bindingset[result, i]
@@ -302,11 +325,88 @@ private predicate fieldReadStep(Node node1, FieldContent f, Node node2) {
   )
 }
 
+/**
+ * When a store step happens in a function that looks like an array write such as:
+ * ```cpp
+ * void f(int* pa) {
+ *   pa = source();
+ * }
+ * ```
+ * it can be a write to an array, but it can also happen that `f` is called as `f(&a.x)`. If that is
+ * the case, the `ArrayContent` that was written by the call to `f` should be popped off the access
+ * path, and a `FieldContent` containing `x` should be pushed instead.
+ * So this case pops `ArrayContent` off the access path, and the `fieldStoreStepAfterArraySuppression`
+ * predicate in `storeStep` ensures that we push the right `FieldContent` onto the access path.
+ */
+predicate suppressArrayRead(Node node1, ArrayContent a, Node node2) {
+  a = TArrayContent() and
+  exists(BufferMayWriteSideEffectInstruction write, ChiInstruction chi |
+    node1.asInstruction() = write and
+    node2.asInstruction() = chi and
+    chi.getPartial() = write and
+    getWrittenField(write, _, _)
+  )
+}
+
+private class ArrayToPointerConvertInstruction extends ConvertInstruction {
+  ArrayToPointerConvertInstruction() {
+    this.getUnary().getResultType() instanceof ArrayType and
+    this.getResultType() instanceof PointerType
+  }
+}
+
+private Instruction skipOneCopyValueInstruction(Instruction instr) {
+  not instr instanceof CopyValueInstruction and result = instr
+  or
+  result = instr.(CopyValueInstruction).getUnary()
+}
+
+private Instruction skipCopyValueInstructions(Instruction instr) {
+  result = skipOneCopyValueInstruction*(instr) and not result instanceof CopyValueInstruction
+}
+
 private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
   a = TArrayContent() and
-  exists(LoadInstruction load |
+  // Explicit dereferences such as `*p` or `p[i]` where `p` is a pointer or array.
+  exists(LoadInstruction load, Instruction address |
+    load.getSourceValueOperand().isDefinitionInexact() and
     node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
-    load = node2.asInstruction()
+    load = node2.asInstruction() and
+    address = skipCopyValueInstructions(load.getSourceAddress()) and
+    (
+      address instanceof LoadInstruction or
+      address instanceof ArrayToPointerConvertInstruction or
+      address instanceof PointerOffsetInstruction
+    )
+  )
+}
+
+/**
+ * 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)
   )
 }
 
@@ -317,7 +417,9 @@ 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)
+  arrayReadStep(node1, f, node2) or
+  exactReadStep(node1, f, node2) or
+  suppressArrayRead(node1, f, node2)
 }
 
 /**

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

@@ -389,6 +389,35 @@ private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNod
   }
 }
 
+private FieldAddressInstruction getFieldInstruction(Instruction instr) {
+  result = instr or
+  result = instr.(CopyValueInstruction).getUnary()
+}
+
+/**
+ * The target of a `fieldStoreStepAfterArraySuppression` store step, which is used to convert
+ * an `ArrayContent` to a `FieldContent` when the `BufferMayWriteSideEffect` instruction stores
+ * into a field. See the QLDoc for `suppressArrayRead` for an example of where such a conversion
+ * is inserted.
+ */
+private class BufferMayWriteSideEffectFieldStoreQualifierNode extends PartialDefinitionNode {
+  override ChiInstruction instr;
+  BufferMayWriteSideEffectInstruction write;
+  FieldAddressInstruction field;
+
+  BufferMayWriteSideEffectFieldStoreQualifierNode() {
+    not instr.isResultConflated() and
+    instr.getPartial() = write and
+    field = getFieldInstruction(write.getDestinationAddress())
+  }
+
+  override Node getPreUpdateNode() { result.asOperand() = instr.getTotalOperand() }
+
+  override Expr getDefinedExpr() {
+    result = field.getObjectAddress().getUnconvertedResultExpression()
+  }
+}
+
 /**
  * The `PostUpdateNode` that is the target of a `arrayStoreStepChi` store step. The overriden
  * `ChiInstruction` corresponds to the instruction represented by `node2` in `arrayStoreStepChi`.

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 |

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

@@ -109,5 +109,99 @@ void taint_a_ptr(int* pa) {
 void test_field_conflation_array_content() {
   S s;
   taint_a_ptr(&s.m1);
-  sink(s.m2); //$f+:ir
+  sink(s.m2);
+}
+
+struct S_with_pointer {
+  int m1, m2;
+  int* data;
+};
+
+void pointer_deref(int* xs) {
+  taint_a_ptr(xs);
+  sink(xs[0]); // $f-:ast $ir
+}
+
+void pointer_deref_sub(int* xs) {
+  taint_a_ptr(xs - 2);
+  sink(*(xs - 2)); // $f-:ast $ir
+}
+
+void pointer_many_addrof_and_deref(int* xs) {
+  taint_a_ptr(xs);
+  sink(*&*&*xs); // $f-:ast $ir
+}
+
+void pointer_unary_plus(int* xs) {
+  taint_a_ptr(+xs);
+  sink(*+xs); // $f-:ast $ir
+}
+
+void pointer_member_index(S_with_pointer s) {
+  taint_a_ptr(s.data);
+  // `s.data` is points to all-aliased-memory
+  sink(s.data[0]); // $f-:ast,ir
+}
+
+void member_array_different_field(S_with_pointer* s) {
+  taint_a_ptr(&s[0].m1);
+  sink(s[0].m2);
+}
+
+struct S_with_array {
+  int m1, m2;
+  int data[10];
+};
+
+void pointer_member_deref() {
+  S_with_array s;
+  taint_a_ptr(s.data);
+  sink(*s.data); // $ir,ast
+}
+
+void array_member_deref() {
+  S_with_array s;
+  taint_a_ptr(s.data);
+  sink(s.data[0]); // $ir,ast
+}
+
+struct S2 {
+  S s;
+  int m3;
+};
+
+void deep_member_field_dot() {
+  S2 s2;
+  taint_a_ptr(&s2.s.m1);
+  sink(s2.s.m1); // $ir,ast
+}
+
+void deep_member_field_dot_different_fields() {
+  S2 s2;
+  taint_a_ptr(&s2.s.m1);
+  sink(s2.s.m2);
+}
+
+void deep_member_field_dot_2() {
+  S2 s2;
+  taint_a_ptr(&s2.s.m1);
+  S2 s2_2 = s2;
+  sink(s2_2.s.m1); // $ir,ast
+}
+
+void deep_member_field_dot_different_fields_2() {
+  S2 s2;
+  taint_a_ptr(&s2.s.m1);
+  S2 s2_2 = s2;
+  sink(s2_2.s.m2);
+}
+
+void deep_member_field_arrow(S2 *ps2) {
+  taint_a_ptr(&ps2->s.m1);
+  sink(ps2->s.m1); // $ir,ast
+}
+
+void deep_member_field_arrow_different_fields(S2 *ps2) {
+  taint_a_ptr(&ps2->s.m1);
+  sink(ps2->s.m2);
 }

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

@@ -21,7 +21,10 @@
 | 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 |
-| aliasing.cpp:106:9:106:18 | call to user_input | aliasing.cpp:112:10:112:11 | m2 | IR only |
+| aliasing.cpp:106:9:106:18 | call to user_input | aliasing.cpp:122:8:122:12 | access to array | IR only |
+| aliasing.cpp:106:9:106:18 | call to user_input | aliasing.cpp:127:8:127:16 | * ... | IR only |
+| aliasing.cpp:106:9:106:18 | call to user_input | aliasing.cpp:132:8:132:14 | * ... | IR only |
+| aliasing.cpp:106:9:106:18 | call to user_input | aliasing.cpp:137:8:137:11 | * ... | 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 |