C++: Respond to review comments and accept test changes.

2025-12-18 01:33:15 +01:00 · 2020-10-29 16:30:35 +01:00
parent f3f9a044e0
commit 177f94368e
9 changed files with 72 additions and 27 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
@@ -251,10 +251,10 @@ private predicate getWrittenField(Instruction instr, Field f, Class c) {
 }

 private predicate fieldStoreStepChi(Node node1, FieldContent f, PostUpdateNode node2) {
-  exists(StoreValueOperand operand, ChiInstruction chi |
+  exists(ChiPartialOperand operand, ChiInstruction chi |
+    chi.getPartialOperand() = operand and
    node1.asOperand() = operand and
    node2.asInstruction() = chi and
-    chi.getPartial() = operand.getUse() and
    exists(Class c |
      c = chi.getResultType() and
      exists(int startBit, int endBit |
@@ -262,7 +262,7 @@ private predicate fieldStoreStepChi(Node node1, FieldContent f, PostUpdateNode n
        f.hasOffset(c, startBit, endBit)
      )
      or
-      getWrittenField(operand.getUse(), f.getAField(), c) and
+      getWrittenField(operand.getDef(), f.getAField(), c) and
      f.hasOffset(c, _, _)
    )
  )
@@ -270,9 +270,13 @@ private predicate fieldStoreStepChi(Node node1, FieldContent f, PostUpdateNode n

 private predicate arrayStoreStepChi(Node node1, ArrayContent a, PostUpdateNode node2) {
  a = TArrayContent() and
-  exists(StoreValueOperand operand, StoreInstruction store |
-    store.getSourceValueOperand() = operand and
+  exists(ChiPartialOperand operand, ChiInstruction chi, StoreInstruction store |
+    chi.getPartialOperand() = operand and
+    store = operand.getDef() and
    node1.asOperand() = operand and
+    // This `ChiInstruction` will always have a non-conflated result because both `ArrayStoreNode`
+    // and `PointerStoreNode` require it in their characteristic predicates.
+    node2.asInstruction() = chi and
    (
      // `x[i] = taint()`
      // This matches the characteristic predicate in `ArrayStoreNode`.
@@ -281,10 +285,7 @@ private predicate arrayStoreStepChi(Node node1, ArrayContent a, PostUpdateNode n
      // `*p = taint()`
      // This matches the characteristic predicate in `PointerStoreNode`.
      store.getDestinationAddress().(CopyValueInstruction).getUnary() instanceof LoadInstruction
-    ) and
-    // This `ChiInstruction` will always have a non-conflated result because both `ArrayStoreNode`
-    // and `PointerStoreNode` require it in their characteristic predicates.
-    node2.asInstruction().(ChiInstruction).getPartial() = store
+    )
  )
 }

@@ -385,10 +386,10 @@ private Instruction skipOneCopyValueInstructionRec(CopyValueInstruction copy) {
  result = skipOneCopyValueInstructionRec(copy.getUnary())
 }

-private Instruction skipCopyValueInstructions(Instruction instr) {
-  not result instanceof CopyValueInstruction and result = instr
+private Instruction skipCopyValueInstructions(Operand op) {
+  not result instanceof CopyValueInstruction and result = op.getDef()
  or
-  result = skipOneCopyValueInstructionRec(instr)
+  result = skipOneCopyValueInstructionRec(op.getDef())
 }

 private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
@@ -398,7 +399,7 @@ private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
    operand.isDefinitionInexact() and
    node1.asInstruction() = operand.getAnyDef() and
    operand = node2.asOperand() and
-    address = skipCopyValueInstructions(operand.getUse().(LoadInstruction).getSourceAddress()) and
+    address = skipCopyValueInstructions(operand.getAddressOperand()) and
    (
      address instanceof LoadInstruction or
      address instanceof ArrayToPointerConvertInstruction or
@@ -419,7 +420,7 @@ private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
 * 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`)
+ * is a `WriteSideEffect`). 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/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
+++ b/cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
@@ -396,16 +396,16 @@ private FieldAddressInstruction getFieldInstruction(Instruction instr) {

 /**
 * The target of a `fieldStoreStepAfterArraySuppression` store step, which is used to convert
- * an `ArrayContent` to a `FieldContent` when the `BufferMayWriteSideEffect` instruction stores
+ * an `ArrayContent` to a `FieldContent` when the `WriteSideEffect` 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 {
+private class WriteSideEffectFieldStoreQualifierNode extends PartialDefinitionNode {
  override ChiInstruction instr;
-  BufferMayWriteSideEffectInstruction write;
+  WriteSideEffectInstruction write;
  FieldAddressInstruction field;

-  BufferMayWriteSideEffectFieldStoreQualifierNode() {
+  WriteSideEffectFieldStoreQualifierNode() {
    not instr.isResultConflated() and
    instr.getPartial() = write and
    field = getFieldInstruction(write.getDestinationAddress())
--- a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected
+++ b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected
@@ -121,6 +121,14 @@ postWithInFlow
 | complex.cpp:12:22:12:27 | Chi | PostUpdateNode should not be the target of local flow. |
 | complex.cpp:14:26:14:26 | Chi | PostUpdateNode should not be the target of local flow. |
 | complex.cpp:14:33:14:33 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:22:11:22:17 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:25:7:25:7 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:42:16:42:16 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:43:16:43:16 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:53:12:53:12 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:54:12:54:12 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:55:12:55:12 | Chi | PostUpdateNode should not be the target of local flow. |
+| complex.cpp:56:12:56:12 | Chi | PostUpdateNode should not be the target of local flow. |
 | constructors.cpp:20:24:20:29 | Chi | PostUpdateNode should not be the target of local flow. |
 | constructors.cpp:21:24:21:29 | Chi | PostUpdateNode should not be the target of local flow. |
 | constructors.cpp:23:28:23:28 | Chi | PostUpdateNode should not be the target of local flow. |
--- a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected
+++ b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected
@@ -127,18 +127,34 @@ edges
 | by_reference.cpp:128:15:128:23 | Chi [a] | by_reference.cpp:136:16:136:16 | a |
 | by_reference.cpp:128:15:128:23 | taint_a_ref output argument [array content] | by_reference.cpp:128:15:128:23 | Chi |
 | complex.cpp:40:17:40:17 | *b [a_] | complex.cpp:42:18:42:18 | call to a |
+| complex.cpp:40:17:40:17 | *b [b_] | complex.cpp:42:16:42:16 | Chi [b_] |
 | complex.cpp:40:17:40:17 | *b [b_] | complex.cpp:42:16:42:16 | a output argument [b_] |
 | complex.cpp:40:17:40:17 | *b [b_] | complex.cpp:43:18:43:18 | call to b |
+| complex.cpp:42:16:42:16 | Chi [b_] | complex.cpp:43:18:43:18 | call to b |
+| complex.cpp:42:16:42:16 | a output argument [b_] | complex.cpp:42:16:42:16 | Chi [b_] |
 | complex.cpp:42:16:42:16 | a output argument [b_] | complex.cpp:43:18:43:18 | call to b |
+| complex.cpp:53:12:53:12 | Chi [a_] | complex.cpp:40:17:40:17 | *b [a_] |
 | complex.cpp:53:12:53:12 | setA output argument [a_] | complex.cpp:40:17:40:17 | *b [a_] |
+| complex.cpp:53:12:53:12 | setA output argument [a_] | complex.cpp:53:12:53:12 | Chi [a_] |
 | complex.cpp:53:19:53:28 | call to user_input | complex.cpp:53:12:53:12 | setA output argument [a_] |
+| complex.cpp:54:12:54:12 | Chi [b_] | complex.cpp:40:17:40:17 | *b [b_] |
 | complex.cpp:54:12:54:12 | setB output argument [b_] | complex.cpp:40:17:40:17 | *b [b_] |
+| complex.cpp:54:12:54:12 | setB output argument [b_] | complex.cpp:54:12:54:12 | Chi [b_] |
 | complex.cpp:54:19:54:28 | call to user_input | complex.cpp:54:12:54:12 | setB output argument [b_] |
+| complex.cpp:55:12:55:12 | Chi [a_] | complex.cpp:40:17:40:17 | *b [a_] |
+| complex.cpp:55:12:55:12 | Chi [a_] | complex.cpp:56:12:56:12 | Chi [a_] |
+| complex.cpp:55:12:55:12 | Chi [a_] | complex.cpp:56:12:56:12 | setB output argument [a_] |
 | complex.cpp:55:12:55:12 | setA output argument [a_] | complex.cpp:40:17:40:17 | *b [a_] |
+| complex.cpp:55:12:55:12 | setA output argument [a_] | complex.cpp:55:12:55:12 | Chi [a_] |
+| complex.cpp:55:12:55:12 | setA output argument [a_] | complex.cpp:56:12:56:12 | Chi [a_] |
 | complex.cpp:55:12:55:12 | setA output argument [a_] | complex.cpp:56:12:56:12 | setB output argument [a_] |
 | complex.cpp:55:19:55:28 | call to user_input | complex.cpp:55:12:55:12 | setA output argument [a_] |
+| complex.cpp:56:12:56:12 | Chi [a_] | complex.cpp:40:17:40:17 | *b [a_] |
+| complex.cpp:56:12:56:12 | Chi [b_] | complex.cpp:40:17:40:17 | *b [b_] |
 | complex.cpp:56:12:56:12 | setB output argument [a_] | complex.cpp:40:17:40:17 | *b [a_] |
+| complex.cpp:56:12:56:12 | setB output argument [a_] | complex.cpp:56:12:56:12 | Chi [a_] |
 | complex.cpp:56:12:56:12 | setB output argument [b_] | complex.cpp:40:17:40:17 | *b [b_] |
+| complex.cpp:56:12:56:12 | setB output argument [b_] | complex.cpp:56:12:56:12 | Chi [b_] |
 | complex.cpp:56:19:56:28 | call to user_input | complex.cpp:56:12:56:12 | setB output argument [b_] |
 | constructors.cpp:26:15:26:15 | *f [a_] | constructors.cpp:28:12:28:12 | call to a |
 | constructors.cpp:26:15:26:15 | *f [b_] | constructors.cpp:28:10:28:10 | a output argument [b_] |
@@ -340,15 +356,21 @@ nodes
 | by_reference.cpp:136:16:136:16 | a | semmle.label | a |
 | complex.cpp:40:17:40:17 | *b [a_] | semmle.label | *b [a_] |
 | complex.cpp:40:17:40:17 | *b [b_] | semmle.label | *b [b_] |
+| complex.cpp:42:16:42:16 | Chi [b_] | semmle.label | Chi [b_] |
 | complex.cpp:42:16:42:16 | a output argument [b_] | semmle.label | a output argument [b_] |
 | complex.cpp:42:18:42:18 | call to a | semmle.label | call to a |
 | complex.cpp:43:18:43:18 | call to b | semmle.label | call to b |
+| complex.cpp:53:12:53:12 | Chi [a_] | semmle.label | Chi [a_] |
 | complex.cpp:53:12:53:12 | setA output argument [a_] | semmle.label | setA output argument [a_] |
 | complex.cpp:53:19:53:28 | call to user_input | semmle.label | call to user_input |
+| complex.cpp:54:12:54:12 | Chi [b_] | semmle.label | Chi [b_] |
 | complex.cpp:54:12:54:12 | setB output argument [b_] | semmle.label | setB output argument [b_] |
 | complex.cpp:54:19:54:28 | call to user_input | semmle.label | call to user_input |
+| complex.cpp:55:12:55:12 | Chi [a_] | semmle.label | Chi [a_] |
 | complex.cpp:55:12:55:12 | setA output argument [a_] | semmle.label | setA output argument [a_] |
 | complex.cpp:55:19:55:28 | call to user_input | semmle.label | call to user_input |
+| complex.cpp:56:12:56:12 | Chi [a_] | semmle.label | Chi [a_] |
+| complex.cpp:56:12:56:12 | Chi [b_] | semmle.label | Chi [b_] |
 | complex.cpp:56:12:56:12 | setB output argument [a_] | semmle.label | setB output argument [a_] |
 | complex.cpp:56:12:56:12 | setB output argument [b_] | semmle.label | setB output argument [b_] |
 | complex.cpp:56:19:56:28 | call to user_input | semmle.label | call to user_input |
--- a/cpp/ql/test/library-tests/dataflow/fields/partial-definition-diff.expected
+++ b/cpp/ql/test/library-tests/dataflow/fields/partial-definition-diff.expected
@@ -294,22 +294,16 @@
 | complex.cpp:11:22:11:23 | a_ | AST only |
 | complex.cpp:12:22:12:23 | b_ | AST only |
 | complex.cpp:42:8:42:8 | b | AST only |
-| complex.cpp:42:10:42:14 | inner | AST only |
 | complex.cpp:42:16:42:16 | f | AST only |
 | complex.cpp:43:8:43:8 | b | AST only |
-| complex.cpp:43:10:43:14 | inner | AST only |
 | complex.cpp:43:16:43:16 | f | AST only |
 | complex.cpp:53:3:53:4 | b1 | AST only |
-| complex.cpp:53:6:53:10 | inner | AST only |
 | complex.cpp:53:12:53:12 | f | AST only |
 | complex.cpp:54:3:54:4 | b2 | AST only |
-| complex.cpp:54:6:54:10 | inner | AST only |
 | complex.cpp:54:12:54:12 | f | AST only |
 | complex.cpp:55:3:55:4 | b3 | AST only |
-| complex.cpp:55:6:55:10 | inner | AST only |
 | complex.cpp:55:12:55:12 | f | AST only |
 | complex.cpp:56:3:56:4 | b3 | AST only |
-| complex.cpp:56:6:56:10 | inner | AST only |
 | complex.cpp:56:12:56:12 | f | AST only |
 | complex.cpp:59:7:59:8 | b1 | AST only |
 | complex.cpp:62:7:62:8 | b2 | AST only |
--- a/cpp/ql/test/library-tests/dataflow/fields/partial-definition-ir.expected
+++ b/cpp/ql/test/library-tests/dataflow/fields/partial-definition-ir.expected
@@ -51,6 +51,12 @@
 | by_reference.cpp:128:15:128:20 | pouter |
 | complex.cpp:11:22:11:23 | this |
 | complex.cpp:12:22:12:23 | this |
+| complex.cpp:42:10:42:14 | inner |
+| complex.cpp:43:10:43:14 | inner |
+| complex.cpp:53:6:53:10 | inner |
+| complex.cpp:54:6:54:10 | inner |
+| complex.cpp:55:6:55:10 | inner |
+| complex.cpp:56:6:56:10 | inner |
 | constructors.cpp:20:24:20:25 | this |
 | constructors.cpp:21:24:21:25 | this |
 | qualifiers.cpp:9:30:9:33 | this |
--- a/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected
+++ b/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected
@@ -1491,6 +1491,7 @@ postWithInFlow
 | conditional_destructors.cpp:18:13:18:19 | Chi | PostUpdateNode should not be the target of local flow. |
 | cpp11.cpp:65:19:65:45 | Store | PostUpdateNode should not be the target of local flow. |
 | cpp11.cpp:82:17:82:55 | Chi | PostUpdateNode should not be the target of local flow. |
+| cpp11.cpp:82:17:82:55 | Chi | PostUpdateNode should not be the target of local flow. |
 | cpp11.cpp:82:45:82:48 | Chi | PostUpdateNode should not be the target of local flow. |
 | defdestructordeleteexpr.cpp:4:9:4:15 | Chi | PostUpdateNode should not be the target of local flow. |
 | deleteexpr.cpp:7:9:7:15 | Chi | PostUpdateNode should not be the target of local flow. |
@@ -1541,6 +1542,18 @@ postWithInFlow
 | ir.cpp:659:9:659:14 | Chi | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:660:13:660:13 | Chi | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:661:9:661:13 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:662:9:662:19 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:663:5:663:5 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:745:8:745:8 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:745:8:745:8 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:748:10:748:10 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:754:8:754:8 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:757:12:757:12 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:763:8:763:8 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:766:13:766:13 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:775:15:775:15 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:784:15:784:15 | Chi | PostUpdateNode should not be the target of local flow. |
+| ir.cpp:793:15:793:15 | Chi | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:943:3:943:11 | Chi | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:947:3:947:25 | Chi | PostUpdateNode should not be the target of local flow. |
 | ir.cpp:962:17:962:47 | Chi | PostUpdateNode should not be the target of local flow. |
@@ -1561,3 +1574,4 @@ postWithInFlow
 | range_analysis.c:102:5:102:15 | Chi | PostUpdateNode should not be the target of local flow. |
 | static_init_templates.cpp:3:2:3:8 | Chi | PostUpdateNode should not be the target of local flow. |
 | static_init_templates.cpp:21:2:21:12 | Chi | PostUpdateNode should not be the target of local flow. |
+| static_init_templates.cpp:240:7:240:7 | Chi | PostUpdateNode should not be the target of local flow. |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/TaintedAllocationSize/TaintedAllocationSize.expected
@@ -143,7 +143,7 @@ nodes
 | test.cpp:235:2:235:9 | Argument 0 | semmle.label | Argument 0 |
 | test.cpp:237:2:237:8 | Argument 0 | semmle.label | Argument 0 |
 | test.cpp:241:2:241:32 | Chi [array content] | semmle.label | Chi [array content] |
-| test.cpp:241:2:241:32 | StoreValue | semmle.label | StoreValue |
+| test.cpp:241:2:241:32 | ChiPartial | semmle.label | ChiPartial |
 | test.cpp:241:18:241:23 | call to getenv | semmle.label | call to getenv |
 | test.cpp:241:18:241:31 | (const char *)... | semmle.label | (const char *)... |
 | test.cpp:249:20:249:25 | call to getenv | semmle.label | call to getenv |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/uncontrolled/ArithmeticUncontrolled.expected
@@ -109,11 +109,11 @@ nodes
 | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand |
 | test.cpp:8:9:8:12 | call to rand | semmle.label | call to rand |
 | test.cpp:13:2:13:15 | Chi [array content] | semmle.label | Chi [array content] |
-| test.cpp:13:2:13:15 | StoreValue | semmle.label | StoreValue |
+| test.cpp:13:2:13:15 | ChiPartial | semmle.label | ChiPartial |
 | test.cpp:13:10:13:13 | call to rand | semmle.label | call to rand |
 | test.cpp:13:10:13:13 | call to rand | semmle.label | call to rand |
 | test.cpp:18:2:18:14 | Chi [array content] | semmle.label | Chi [array content] |
-| test.cpp:18:2:18:14 | StoreValue | semmle.label | StoreValue |
+| test.cpp:18:2:18:14 | ChiPartial | semmle.label | ChiPartial |
 | test.cpp:18:9:18:12 | call to rand | semmle.label | call to rand |
 | test.cpp:18:9:18:12 | call to rand | semmle.label | call to rand |
 | test.cpp:24:11:24:18 | call to get_rand | semmle.label | call to get_rand |