Merge pull request #2569 from jbj/ir-total-chi-flow

C++: IR data flow through total chi operands

Author: Robert Marsh

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

@@ -59,10 +59,12 @@ class Node extends TIRDataFlowNode {
   Parameter asParameter() { result = instr.(InitializeParameterInstruction).getParameter() }
 
   /**
+   * DEPRECATED: See UninitializedNode.
+   *
    * Gets the uninitialized local variable corresponding to this node, if
    * any.
    */
-  LocalVariable asUninitialized() { result = instr.(UninitializedInstruction).getLocalVariable() }
+  LocalVariable asUninitialized() { none() }
 
   /**
    * Gets an upper bound on the type of this node.
@@ -140,15 +142,19 @@ private class ThisParameterNode extends Node {
 }
 
 /**
+ * DEPRECATED: Data flow was never an accurate way to determine what
+ * expressions might be uninitialized. It errs on the side of saying that
+ * everything is uninitialized, and this is even worse in the IR because the IR
+ * doesn't use syntactic hints to rule out variables that are definitely
+ * initialized.
+ *
  * The value of an uninitialized local variable, viewed as a node in a data
  * flow graph.
  */
-class UninitializedNode extends Node {
-  override UninitializedInstruction instr;
-
-  LocalVariable getLocalVariable() { result = instr.getLocalVariable() }
+deprecated class UninitializedNode extends Node {
+  UninitializedNode() { none() }
 
-  override string toString() { result = this.getLocalVariable().toString() }
+  LocalVariable getLocalVariable() { none() }
 }
 
 /**
@@ -259,7 +265,21 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   iTo.(PhiInstruction).getAnOperand().getDef() = iFrom or
   // Treat all conversions as flow, even conversions between different numeric types.
   iTo.(ConvertInstruction).getUnary() = iFrom or
-  iTo.(InheritanceConversionInstruction).getUnary() = iFrom
+  iTo.(InheritanceConversionInstruction).getUnary() = iFrom or
+  // A chi instruction represents a point where a new value (the _partial_
+  // operand) may overwrite an old value (the _total_ operand), but the alias
+  // analysis couldn't determine that it surely will overwrite every bit of it or
+  // that it surely will overwrite no bit of it.
+  //
+  // By allowing flow through the total operand, we ensure that flow is not lost
+  // due to shortcomings of the alias analysis. We may get false flow in cases
+  // where the data is indeed overwritten.
+  //
+  // Allowing flow through the partial operand would be more noisy, especially
+  // for variables that have escaped: for soundness, the IR has to assume that
+  // every write to an unknown address can affect every escaped variable, and
+  // this assumption shows up as data flowing through partial chi operands.
+  iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
 }
 
 /**

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

@@ -29,6 +29,9 @@
 | ref.cpp:109:15:109:20 | ref.cpp:132:13:132:15 | AST only |
 | ref.cpp:122:23:122:28 | ref.cpp:123:13:123:15 | AST only |
 | ref.cpp:125:19:125:24 | ref.cpp:126:13:126:15 | AST only |
+| test.cpp:75:7:75:8 | test.cpp:76:8:76:9 | AST only |
+| test.cpp:83:7:83:8 | test.cpp:84:8:84:18 | AST only |
+| test.cpp:83:7:83:8 | test.cpp:86:8:86:9 | AST only |
 | test.cpp:89:28:89:34 | test.cpp:92:8:92:14 | IR only |
 | test.cpp:100:13:100:18 | test.cpp:103:10:103:12 | AST only |
 | test.cpp:109:9:109:14 | test.cpp:110:10:110:12 | IR only |

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

@@ -31,9 +31,6 @@
 | test.cpp:31:8:31:8 | c | test.cpp:36:13:36:18 | call to source |
 | test.cpp:58:10:58:10 | t | test.cpp:50:14:50:19 | call to source |
 | test.cpp:71:8:71:9 | x4 | test.cpp:66:30:66:36 | source1 |
-| test.cpp:76:8:76:9 | u1 | test.cpp:75:7:75:8 | u1 |
-| test.cpp:84:8:84:18 | ... ? ... : ... | test.cpp:83:7:83:8 | u2 |
-| test.cpp:86:8:86:9 | i1 | test.cpp:83:7:83:8 | u2 |
 | test.cpp:90:8:90:14 | source1 | test.cpp:89:28:89:34 | source1 |
 | test.cpp:92:8:92:14 | source1 | test.cpp:89:28:89:34 | source1 |
 | test.cpp:110:10:110:12 | (reference dereference) | test.cpp:109:9:109:14 | call to source |

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

@@ -22,6 +22,5 @@
 | taint.cpp:250:8:250:8 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:256:8:256:8 | taint.cpp:223:10:223:15 | AST only |
 | taint.cpp:261:7:261:7 | taint.cpp:258:7:258:12 | AST only |
-| taint.cpp:350:7:350:7 | taint.cpp:330:6:330:11 | AST only |
 | taint.cpp:351:7:351:7 | taint.cpp:330:6:330:11 | AST only |
 | taint.cpp:352:7:352:7 | taint.cpp:330:6:330:11 | AST only |

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

@@ -14,3 +14,4 @@
 | taint.cpp:290:7:290:7 | x | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:291:7:291:7 | y | taint.cpp:275:6:275:11 | call to source |
 | taint.cpp:337:7:337:7 | t | taint.cpp:330:6:330:11 | call to source |
+| taint.cpp:350:7:350:7 | t | taint.cpp:330:6:330:11 | call to source |