C++: Remove single-field case from PostUpdateNode and accept tests

2025-12-24 04:36:35 +01:00 · 2020-04-07 12:03:28 +02:00
parent c577541850
commit 5719967a8e
5 changed files with 33 additions and 41 deletions
--- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll
@@ -131,12 +131,10 @@ private class ArrayContent extends Content, TArrayContent {
 * value of `node1`.
 */
 predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
-  exists(FieldAddressInstruction fa |
-    exists(StoreInstruction store |
-      node1.asInstruction() = store and
-      store.getDestinationAddress() = fa
-    ) and
-    node2.getPreUpdateNode().asInstruction() = fa.getObjectAddress() and
+  exists(FieldAddressInstruction fa, StoreInstruction store |
+    node1.asInstruction() = store and
+    store.getDestinationAddress() = fa and
+    node2.asInstruction().(ChiInstruction).getPartial() = store and
    f.(FieldContent).getField() = fa.getField()
  )
 }
--- a/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
@@ -264,22 +264,6 @@ private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
  override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() }
 }

-private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNode {
-  override StoreInstruction instr;
-  FieldAddressInstruction field;
-
-  ExplicitSingleFieldStoreQualifierNode() {
-    field = instr.getDestinationAddress() and
-    not exists(ChiInstruction chi | chi.getPartial() = instr)
-  }
-
-  // Since there is no Chi instruction with a total operand for us to use we let the pre update node
-  // be the address of the object containing the field.
-  // Note that, unlike in the case where a struct has multiple fields (and thus has a `Chi`
-  // instruction), the pre update node will be an instruction with a register result.
-  override Node getPreUpdateNode() { result.asInstruction() = field.getObjectAddress() }
-}
-
 /**
 * A node that represents the value of a variable after a function call that
 * may have changed the variable because it's passed by reference.
@@ -290,31 +274,31 @@ private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNod
 * returned. This node will have its `getArgument()` equal to `&x` and its
 * `getVariableAccess()` equal to `x`.
 */
-class DefinitionByReferenceNode extends PartialDefinitionNode {
-  override ChiInstruction instr;
-  WriteSideEffectInstruction write;
-  CallInstruction call;
-
-  DefinitionByReferenceNode() {
-    not instr.isResultConflated() and
-    instr.getPartial() = write and
-    call = write.getPrimaryInstruction()
-  }
-
-  // See the comment on ExplicitFieldStoreQualifierNode::getPreUpdateNode for comments on why
-  // this causes failures in DataFlowImplConsistency::Consistency.
-  override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() }
+class DefinitionByReferenceNode extends InstructionNode {
+  override WriteSideEffectInstruction instr;

  /** Gets the argument corresponding to this node. */
  Expr getArgument() {
-    result = call.getPositionalArgument(write.getIndex()).getUnconvertedResultExpression()
+    result =
+      instr
+          .getPrimaryInstruction()
+          .(CallInstruction)
+          .getPositionalArgument(instr.getIndex())
+          .getUnconvertedResultExpression()
    or
-    result = call.getThisArgument().getUnconvertedResultExpression() and
-    write.getIndex() = -1
+    result =
+      instr
+          .getPrimaryInstruction()
+          .(CallInstruction)
+          .getThisArgument()
+          .getUnconvertedResultExpression() and
+    instr.getIndex() = -1
  }

  /** Gets the parameter through which this value is assigned. */
-  Parameter getParameter() { result = call.getStaticCallTarget().getParameter(write.getIndex()) }
+  Parameter getParameter() {
+    exists(CallInstruction ci | result = ci.getStaticCallTarget().getParameter(instr.getIndex()))
+  }
 }

 /**
--- a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected
+++ b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected
@@ -31,7 +31,6 @@ postIsNotPre
 postHasUniquePre
 uniquePostUpdate
 | ref.cpp:83:5:83:17 | Chi | Node has multiple PostUpdateNodes. |
-| ref.cpp:100:34:100:36 | InitializeIndirection | Node has multiple PostUpdateNodes. |
 | ref.cpp:109:5:109:22 | Chi | Node has multiple PostUpdateNodes. |
 postIsInSameCallable
 reverseRead
--- a/cpp/ql/test/library-tests/dataflow/fields/ir-flow.expected
+++ b/cpp/ql/test/library-tests/dataflow/fields/ir-flow.expected
@@ -5,6 +5,10 @@ edges
 | aliasing.cpp:42:3:42:22 | Store : void | aliasing.cpp:43:13:43:14 | m1 |
 | aliasing.cpp:42:11:42:20 | call to user_input : void | aliasing.cpp:42:3:42:22 | Store : void |
 | aliasing.cpp:42:11:42:20 | call to user_input : void | aliasing.cpp:43:13:43:14 | m1 |
+| aliasing.cpp:60:3:60:22 | Chi [m1] : void | aliasing.cpp:61:13:61:14 | Store [m1] : void |
+| aliasing.cpp:60:3:60:22 | Store : void | aliasing.cpp:60:3:60:22 | Chi [m1] : void |
+| aliasing.cpp:60:11:60:20 | call to user_input : void | aliasing.cpp:60:3:60:22 | Store : void |
+| aliasing.cpp:61:13:61:14 | Store [m1] : void | aliasing.cpp:62:14:62:15 | m1 |
 | aliasing.cpp:79:3:79:22 | Store : void | aliasing.cpp:80:12:80:13 | m1 |
 | aliasing.cpp:79:11:79:20 | call to user_input : void | aliasing.cpp:79:3:79:22 | Store : void |
 | aliasing.cpp:79:11:79:20 | call to user_input : void | aliasing.cpp:80:12:80:13 | m1 |
@@ -27,6 +31,11 @@ nodes
 | aliasing.cpp:42:3:42:22 | Store : void | semmle.label | Store : void |
 | aliasing.cpp:42:11:42:20 | call to user_input : void | semmle.label | call to user_input : void |
 | aliasing.cpp:43:13:43:14 | m1 | semmle.label | m1 |
+| aliasing.cpp:60:3:60:22 | Chi [m1] : void | semmle.label | Chi [m1] : void |
+| aliasing.cpp:60:3:60:22 | Store : void | semmle.label | Store : void |
+| aliasing.cpp:60:11:60:20 | call to user_input : void | semmle.label | call to user_input : void |
+| aliasing.cpp:61:13:61:14 | Store [m1] : void | semmle.label | Store [m1] : void |
+| aliasing.cpp:62:14:62:15 | m1 | semmle.label | m1 |
 | aliasing.cpp:79:3:79:22 | Store : void | semmle.label | Store : void |
 | aliasing.cpp:79:11:79:20 | call to user_input : void | semmle.label | call to user_input : void |
 | aliasing.cpp:80:12:80:13 | m1 | semmle.label | m1 |
@@ -45,6 +54,7 @@ nodes
 #select
 | aliasing.cpp:38:11:38:12 | m1 | aliasing.cpp:37:13:37:22 | call to user_input : void | aliasing.cpp:38:11:38:12 | m1 | m1 flows from $@ | aliasing.cpp:37:13:37:22 | call to user_input : void | call to user_input : void |
 | aliasing.cpp:43:13:43:14 | m1 | aliasing.cpp:42:11:42:20 | call to user_input : void | aliasing.cpp:43:13:43:14 | m1 | m1 flows from $@ | aliasing.cpp:42:11:42:20 | call to user_input : void | call to user_input : void |
+| aliasing.cpp:62:14:62:15 | m1 | aliasing.cpp:60:11:60:20 | call to user_input : void | aliasing.cpp:62:14:62:15 | m1 | m1 flows from $@ | aliasing.cpp:60:11:60:20 | call to user_input : void | call to user_input : void |
 | aliasing.cpp:80:12:80:13 | m1 | aliasing.cpp:79:11:79:20 | call to user_input : void | aliasing.cpp:80:12:80:13 | m1 | m1 flows from $@ | aliasing.cpp:79:11:79:20 | call to user_input : void | call to user_input : void |
 | aliasing.cpp:87:12:87:13 | m1 | aliasing.cpp:86:10:86:19 | call to user_input : void | aliasing.cpp:87:12:87:13 | m1 | m1 flows from $@ | aliasing.cpp:86:10:86:19 | call to user_input : void | call to user_input : void |
 | aliasing.cpp:93:12:93:13 | m1 | aliasing.cpp:92:12:92:21 | call to user_input : void | aliasing.cpp:93:12:93:13 | m1 | m1 flows from $@ | aliasing.cpp:92:12:92:21 | call to user_input : void | call to user_input : void |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/funcs/funcsLocal.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/funcs/funcsLocal.expected
@@ -5,3 +5,4 @@
 | funcsLocal.c:37:9:37:10 | i5 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:36:7:36:8 | i5 | gets |
 | funcsLocal.c:42:9:42:10 | i6 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:41:13:41:16 | call to gets | gets |
 | funcsLocal.c:42:9:42:10 | i6 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:41:18:41:20 | i61 | gets |
+| funcsLocal.c:58:9:58:10 | e1 | The value of this argument may come from $@ and is being used as a formatting argument to printf(format) | funcsLocal.c:16:8:16:9 | i1 | fread |