Merge branch 'main' into rdmarsh2/cpp/ir-qualifier-flow

Fix test conflicts

Author: Robert Marsh

change-notes/1.26/analysis-cpp.md

@@ -23,7 +23,7 @@ The following changes in version 1.26 affect C/C++ analysis in all applications.
 * The QL class `Block`, denoting the `{ ... }` statement, is renamed to `BlockStmt`.
 * The models library now models many taint flows through `std::array`, `std::vector`, `std::deque`, `std::list` and `std::forward_list`.
 * The models library now models many more taint flows through `std::string`.
-* The models library now models some taint flows through `std::ostream`.
+* The models library now models many taint flows through `std::istream` and `std::ostream`.
 * The models library now models some taint flows through `std::shared_ptr`, `std::unique_ptr`, `std::make_shared` and `std::make_unique`.
 * The `SimpleRangeAnalysis` library now supports multiplications of the form
   `e1 * e2` and `x *= e2` when `e1` and `e2` are unsigned or constant.

cpp/ql/src/semmle/code/cpp/controlflow/internal/ConstantExprs.qll

@@ -279,20 +279,62 @@ private predicate reachableRecursive(ControlFlowNode n) {
   reachableRecursive(n.getAPredecessor())
 }
 
+/** Holds if `e` is a compile time constant with integer value `val`. */
 private predicate compileTimeConstantInt(Expr e, int val) {
-  val = e.getFullyConverted().getValue().toInt() and
-  not e instanceof StringLiteral and
-  not exists(Expr e1 | e1.getConversion() = e) // only values for fully converted expressions
+  (
+    // If we have an integer value then we are done.
+    if exists(e.getValue().toInt())
+    then val = e.getValue().toInt()
+    else
+      // Otherwise, if we are a conversion of another expression with an
+      // integer value, and that value can be converted into our type,
+      // then we have that value.
+      exists(Expr x, int valx |
+        x.getConversion() = e and
+        compileTimeConstantInt(x, valx) and
+        val = convertIntToType(valx, e.getType().getUnspecifiedType())
+      )
+  ) and
+  // If our unconverted expression is a string literal `"123"`, then we
+  // do not have integer value `123`.
+  not e.getUnconverted() instanceof StringLiteral
 }
 
-library class CompileTimeConstantInt extends Expr {
-  CompileTimeConstantInt() { compileTimeConstantInt(this, _) }
+/**
+ * Get `val` represented as type `t`, if that is possible without
+ * overflow or underflows.
+ */
+bindingset[val, t]
+private int convertIntToType(int val, IntegralType t) {
+  if t instanceof BoolType
+  then if val = 0 then result = 0 else result = 1
+  else
+    if t.isUnsigned()
+    then if val >= 0 and val.bitShiftRight(t.getSize() * 8) = 0 then result = val else none()
+    else
+      if val >= 0 and val.bitShiftRight(t.getSize() * 8 - 1) = 0
+      then result = val
+      else
+        if (-(val + 1)).bitShiftRight(t.getSize() * 8 - 1) = 0
+        then result = val
+        else none()
+}
+
+/**
+ * INTERNAL: Do not use.
+ * An expression that has been found to have an integer value at compile
+ * time.
+ */
+class CompileTimeConstantInt extends Expr {
+  int val;
+
+  CompileTimeConstantInt() { compileTimeConstantInt(this.getFullyConverted(), val) }
 
-  int getIntValue() { compileTimeConstantInt(this, result) }
+  int getIntValue() { result = val }
 }
 
 library class CompileTimeVariableExpr extends Expr {
-  CompileTimeVariableExpr() { not compileTimeConstantInt(this, _) }
+  CompileTimeVariableExpr() { not this instanceof CompileTimeConstantInt }
 }
 
 /** A helper class for evaluation of expressions. */

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -264,9 +264,6 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
     t instanceof Union
     or
     t instanceof ArrayType
-    or
-    // Buffers of unknown size
-    t instanceof UnknownType
   )
   or
   exists(BinaryInstruction bin |

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

@@ -197,15 +197,17 @@ private class CollectionContent extends Content, TCollectionContent {
 }
 
 private class ArrayContent extends Content, TArrayContent {
-  override string toString() { result = "array" }
+  ArrayContent() { this = TArrayContent() }
+
+  override string toString() { result = "array content" }
 }
 
-private predicate storeStepNoChi(Node node1, Content f, PostUpdateNode node2) {
+private predicate fieldStoreStepNoChi(Node node1, FieldContent f, PostUpdateNode node2) {
   exists(StoreInstruction store, Class c |
     store = node2.asInstruction() and
     store.getSourceValue() = node1.asInstruction() and
     getWrittenField(store, f.(FieldContent).getAField(), c) and
-    f.(FieldContent).hasOffset(c, _, _)
+    f.hasOffset(c, _, _)
   )
 }
 
@@ -218,7 +220,7 @@ private predicate getWrittenField(StoreInstruction store, Field f, Class c) {
   )
 }
 
-private predicate storeStepChi(Node node1, Content f, PostUpdateNode node2) {
+private predicate fieldStoreStepChi(Node node1, FieldContent f, PostUpdateNode node2) {
   exists(StoreInstruction store, ChiInstruction chi |
     node1.asInstruction() = store and
     node2.asInstruction() = chi and
@@ -227,23 +229,43 @@ private predicate storeStepChi(Node node1, Content f, PostUpdateNode node2) {
       c = chi.getResultType() and
       exists(int startBit, int endBit |
         chi.getUpdatedInterval(startBit, endBit) and
-        f.(FieldContent).hasOffset(c, startBit, endBit)
+        f.hasOffset(c, startBit, endBit)
       )
       or
-      getWrittenField(store, f.(FieldContent).getAField(), c) and
-      f.(FieldContent).hasOffset(c, _, _)
+      getWrittenField(store, f.getAField(), c) and
+      f.hasOffset(c, _, _)
     )
   )
 }
 
+private predicate arrayStoreStepChi(Node node1, ArrayContent a, PostUpdateNode node2) {
+  a = TArrayContent() and
+  exists(StoreInstruction store |
+    node1.asInstruction() = store and
+    (
+      // `x[i] = taint()`
+      // This matches the characteristic predicate in `ArrayStoreNode`.
+      store.getDestinationAddress() instanceof PointerAddInstruction
+      or
+      // `*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
+  )
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` via an assignment to `f`.
  * Thus, `node2` references an object with a field `f` that contains the
  * value of `node1`.
  */
 predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
-  storeStepNoChi(node1, f, node2) or
-  storeStepChi(node1, f, node2)
+  fieldStoreStepNoChi(node1, f, node2) or
+  fieldStoreStepChi(node1, f, node2) or
+  arrayStoreStepChi(node1, f, node2)
 }
 
 bindingset[result, i]
@@ -263,23 +285,41 @@ private predicate getLoadedField(LoadInstruction load, Field f, Class c) {
  * Thus, `node1` references an object with a field `f` whose value ends up in
  * `node2`.
  */
-predicate readStep(Node node1, Content f, Node node2) {
+private predicate fieldReadStep(Node node1, FieldContent f, Node node2) {
   exists(LoadInstruction load |
     node2.asInstruction() = load and
     node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
     exists(Class c |
       c = load.getSourceValueOperand().getAnyDef().getResultType() and
       exists(int startBit, int endBit |
         load.getSourceValueOperand().getUsedInterval(unbindInt(startBit), unbindInt(endBit)) and
-        f.(FieldContent).hasOffset(c, startBit, endBit)
+        f.hasOffset(c, startBit, endBit)
       )
       or
-      getLoadedField(load, f.(FieldContent).getAField(), c) and
-      f.(FieldContent).hasOffset(c, _, _)
+      getLoadedField(load, f.getAField(), c) and
+      f.hasOffset(c, _, _)
     )
   )
 }
 
+private predicate arrayReadStep(Node node1, ArrayContent a, Node node2) {
+  a = TArrayContent() and
+  exists(LoadInstruction load |
+    node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
+    load = node2.asInstruction()
+  )
+}
+
+/**
+ * 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
+ * `node2`.
+ */
+predicate readStep(Node node1, Content f, Node node2) {
+  fieldReadStep(node1, f, node2) or
+  arrayReadStep(node1, f, node2)
+}
+
 /**
  * Holds if values stored inside content `c` are cleared at node `n`.
  */

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

@@ -389,6 +389,45 @@ private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNod
   }
 }
 
+/**
+ * The `PostUpdateNode` that is the target of a `arrayStoreStepChi` store step. The overriden
+ * `ChiInstruction` corresponds to the instruction represented by `node2` in `arrayStoreStepChi`.
+ */
+private class ArrayStoreNode extends PartialDefinitionNode {
+  override ChiInstruction instr;
+  PointerAddInstruction add;
+
+  ArrayStoreNode() {
+    not instr.isResultConflated() and
+    exists(StoreInstruction store |
+      instr.getPartial() = store and
+      add = store.getDestinationAddress()
+    )
+  }
+
+  override Node getPreUpdateNode() { result.asOperand() = instr.getTotalOperand() }
+
+  override Expr getDefinedExpr() { result = add.getLeft().getUnconvertedResultExpression() }
+}
+
+/**
+ * The `PostUpdateNode` that is the target of a `arrayStoreStepChi` store step. The overriden
+ * `ChiInstruction` corresponds to the instruction represented by `node2` in `arrayStoreStepChi`.
+ */
+private class PointerStoreNode extends PostUpdateNode {
+  override ChiInstruction instr;
+
+  PointerStoreNode() {
+    not instr.isResultConflated() and
+    exists(StoreInstruction store |
+      instr.getPartial() = store and
+      store.getDestinationAddress().(CopyValueInstruction).getUnary() instanceof LoadInstruction
+    )
+  }
+
+  override Node getPreUpdateNode() { result.asOperand() = instr.getTotalOperand() }
+}
+
 /**
  * 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.
@@ -545,6 +584,7 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) { simpleLocalFlowStep(nodeFr
  * This is the local flow predicate that's used as a building block in global
  * data flow. It may have less flow than the `localFlowStep` predicate.
  */
+cached
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   // Operand -> Instruction flow
   simpleInstructionLocalFlowStep(nodeFrom.asOperand(), nodeTo.asInstruction())
@@ -562,10 +602,11 @@ private predicate getFieldSizeOfClass(Class c, Type type, int size) {
   )
 }
 
-private predicate isSingleFieldClass(Type type, Class cTo) {
-  exists(int size |
-    cTo.getSize() = size and
-    getFieldSizeOfClass(cTo, type, size)
+private predicate isSingleFieldClass(Type type, Operand op) {
+  exists(int size, Class c |
+    c = op.getType().getUnderlyingType() and
+    c.getSize() = size and
+    getFieldSizeOfClass(c, type, size)
   )
 }
 
@@ -601,11 +642,10 @@ private predicate simpleOperandLocalFlowStep(Instruction iFrom, Operand opTo) {
   exists(LoadInstruction load |
     load.getSourceValueOperand() = opTo and
     opTo.getAnyDef() = iFrom and
-    isSingleFieldClass(iFrom.getResultType(), opTo.getType().getUnderlyingType())
+    isSingleFieldClass(iFrom.getResultType(), opTo)
   )
 }
 
-cached
 private predicate simpleInstructionLocalFlowStep(Operand opFrom, Instruction iTo) {
   iTo.(CopyInstruction).getSourceValueOperand() = opFrom
   or