C++: Flow out of functions that write to iterators.

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

@@ -381,7 +381,7 @@ private Operand fullyConvertedCallStep(Operand op) {
  */
 private Instruction getUse(Operand op) {
   result = op.getUse() and
-  not Ssa::ignoreOperand(op)
+  not Ssa::ignoreInstruction(result)
 }
 
 /** Gets a use of the instruction `instr` that is not ignored for dataflow purposes. */

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll

@@ -121,10 +121,10 @@ private newtype TDefOrUseImpl =
     not isDef(_, _, operand, _, _, _)
   } or
   TIteratorDef(
-    Operand iteratorAddress, BaseSourceVariableInstruction container, int indirectionIndex
+    Operand iteratorDerefAddress, BaseSourceVariableInstruction container, int indirectionIndex
   ) {
-    isIteratorDef(container, iteratorAddress, _, _, indirectionIndex) and
+    isIteratorDef(container, iteratorDerefAddress, _, _, indirectionIndex) and
-    any(SsaInternals0::Def def | def.isIteratorDef()).getAddressOperand() = iteratorAddress
+    any(SsaInternals0::Def def | def.isIteratorDef()).getAddressOperand() = iteratorDerefAddress
   } or
   TIteratorUse(
     Operand iteratorAddress, BaseSourceVariableInstruction container, int indirectionIndex
@@ -465,11 +465,20 @@ private predicate indirectConversionFlowStep(Node nFrom, Node nTo) {
     nodeToDefOrUse(nTo, defOrUse, _) and
     adjacentDefRead(defOrUse, _)
   ) and
-  exists(Operand op1, Operand op2, int indirectionIndex, Instruction instr |
+  (
-    hasOperandAndIndex(nFrom, op1, pragma[only_bind_into](indirectionIndex)) and
+    exists(Operand op1, Operand op2, int indirectionIndex, Instruction instr |
-    hasOperandAndIndex(nTo, op2, pragma[only_bind_into](indirectionIndex)) and
+      hasOperandAndIndex(nFrom, op1, pragma[only_bind_into](indirectionIndex)) and
-    instr = op2.getDef() and
+      hasOperandAndIndex(nTo, op2, pragma[only_bind_into](indirectionIndex)) and
-    conversionFlow(op1, instr, _, _)
+      instr = op2.getDef() and
+      conversionFlow(op1, instr, _, _)
+    )
+    or
+    exists(Operand op1, Operand op2, int indirectionIndex, Instruction instr |
+      hasOperandAndIndex(nFrom, op1, pragma[only_bind_into](indirectionIndex)) and
+      hasOperandAndIndex(nTo, op2, indirectionIndex - 1) and
+      instr = op2.getDef() and
+      isDereference(instr, op1)
+    )
   )
 }
 

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll

@@ -406,7 +406,10 @@ private predicate isModifiableAtImpl(CppType cppType, int indirectionIndex) {
   (
     exists(Type pointerType, Type base, Type t |
       pointerType = t.getUnderlyingType() and
-      (pointerType instanceof PointerOrReferenceType or pointerType instanceof Cpp::ArrayType) and
+      (
+        pointerType = any(Indirection ind).getUnderlyingType() or
+        pointerType instanceof Cpp::ArrayType
+      ) and
       cppType.hasType(t, _) and
       base = getTypeImpl(pointerType, indirectionIndex)
     |
@@ -478,9 +481,10 @@ private module Cached {
   }
 
   /**
-   * Holds if `iteratorDerefAddress` is an address of an iterator dereference (i.e., `*it`)
+   * Holds if `iteratorAddress` is an address of an iterator (i.e., `it`)
-   * that is used for a write operation that writes the value `value`. The `memory` instruction
+   * that is dereferenced and then used for a write operation that writes the value `value`.
-   * represents the memory that the IR's SSA analysis determined was read by the call to `operator*`.
+   * The `memory` instruction represents the memory that the IR's SSA analysis determined was
+   * read by the call to `operator*`.
    *
    * The `numberOfLoads` integer represents the number of dereferences this write corresponds to
    * on the underlying container that produced the iterator.
@@ -501,6 +505,77 @@ private module Cached {
     )
   }
 
+  private predicate isSource(Instruction instr, Operand iteratorAddress, int numberOfLoads) {
+    getAUse(instr) = iteratorAddress and
+    exists(BaseSourceVariableInstruction iteratorBase |
+      iteratorBase.getResultType() instanceof Interfaces::Iterator and
+      not iteratorBase.getResultType() instanceof Cpp::PointerType and
+      isUse(_, iteratorAddress, iteratorBase, numberOfLoads - 1, 0)
+    )
+  }
+
+  private predicate isSink(Instruction instr, CallInstruction call) {
+    getAUse(instr).(ArgumentOperand).getCall() = call and
+    // Don't include various operations that don't modify what the iterator points to.
+    not exists(Function f | f = call.getStaticCallTarget() |
+      f instanceof Iterator::IteratorCrementOperator or
+      f instanceof Iterator::IteratorBinaryArithmeticOperator or
+      f instanceof Iterator::IteratorAssignArithmeticOperator or
+      f instanceof Iterator::IteratorCrementMemberOperator or
+      f instanceof Iterator::IteratorBinaryArithmeticMemberOperator or
+      f instanceof Iterator::IteratorAssignArithmeticMemberOperator or
+      f instanceof Iterator::IteratorAssignmentMemberOperator
+    )
+  }
+
+  private predicate convertsIntoArgumentFwd(Instruction instr) {
+    isSource(instr, _, _)
+    or
+    exists(Instruction prev | convertsIntoArgumentFwd(prev) |
+      conversionFlow(unique( | | getAUse(prev)), instr, false, _)
+    )
+  }
+
+  private predicate convertsIntoArgumentRev(Instruction instr) {
+    convertsIntoArgumentFwd(instr) and
+    (
+      isSink(instr, _)
+      or
+      exists(Instruction next | convertsIntoArgumentRev(next) |
+        conversionFlow(unique( | | getAUse(instr)), next, false, _)
+      )
+    )
+  }
+
+  private predicate convertsIntoArgument(
+    Operand iteratorAddress, CallInstruction call, int numberOfLoads
+  ) {
+    exists(Instruction iteratorAddressDef |
+      isSource(iteratorAddressDef, iteratorAddress, numberOfLoads) and
+      isSink(iteratorAddressDef, call) and
+      convertsIntoArgumentRev(pragma[only_bind_into](iteratorAddressDef))
+    )
+  }
+
+  private predicate isChiAfterIteratorArgument(
+    Instruction memory, Operand iteratorAddress, int numberOfLoads
+  ) {
+    // Ideally, `iteratorAddress` would be an `ArgumentOperand`, but there might be
+    // various conversions applied to it before it becomes an argument.
+    // So we do a small amount of flow to find the call that the iterator is passed to.
+    exists(CallInstruction call | convertsIntoArgument(iteratorAddress, call, numberOfLoads) |
+      exists(ReadSideEffectInstruction read |
+        read.getPrimaryInstruction() = call and
+        read.getSideEffectOperand().getAnyDef() = memory
+      )
+      or
+      exists(LoadInstruction load |
+        iteratorAddress.getDef() = load and
+        memory = load.getSourceValueOperand().getAnyDef()
+      )
+    )
+  }
+
   /**
    * Holds if `iterator` is a `StoreInstruction` that stores the result of some function
    * returning an iterator into an address computed started at `containerBase`.
@@ -521,21 +596,22 @@ private module Cached {
   }
 
   /**
-   * Holds if `iteratorDerefAddress` is an address of an iterator dereference (i.e., `*it`)
+   * Holds if `iteratorAddress` is an address of an iterator that is used for
-   * that is used for a read operation. The `memory` instruction represents the memory that
+   * a read operation. The `memory` instruction represents the memory that
    * the IR's SSA analysis determined was read by the call to `operator*`.
    *
-   * Finally, the `numberOfLoads` integer represents the number of dereferences this read
+   * Finally, the `numberOfLoads` integer represents the number of dereferences
-   * corresponds to on the underlying container that produced the iterator.
+   * this read corresponds to on the underlying container that produced the iterator.
    */
   private predicate isChiBeforeIteratorUse(
-    Operand iteratorDerefAddress, Instruction memory, int numberOfLoads
+    Operand iteratorAddress, Instruction memory, int numberOfLoads
   ) {
     exists(
       BaseSourceVariableInstruction iteratorBase, LoadInstruction load,
-      ReadSideEffectInstruction read
+      ReadSideEffectInstruction read, Operand iteratorDerefAddress
     |
       numberOfLoads >= 0 and
+      isUse(_, iteratorAddress, iteratorBase, numberOfLoads + 1, 0) and
       isUse(_, iteratorDerefAddress, iteratorBase, numberOfLoads + 2, 0) and
       iteratorBase.getResultType() instanceof Interfaces::Iterator and
       load.getSourceAddressOperand() = iteratorDerefAddress and
@@ -566,24 +642,35 @@ private module Cached {
   }
 
   /**
-   * Holds if `iteratorDerefAddress` is an address of an iterator dereference (i.e., `*it`)
+   * Holds if `iteratorAddress` is an address of an iterator that is used for a
-   * that is used for a read operation to read a value from a container that created the iterator.
+   * read operation to read a value from a container that created the iterator.
-   * `container` represents the base of the address of the container that was used to create
+   * `container` represents the base of the address of the container that was used
-   * the iterator.
+   * to create the iterator.
    */
   cached
   predicate isIteratorUse(
-    BaseSourceVariableInstruction container, Operand iteratorDerefAddress, int numberOfLoads,
+    BaseSourceVariableInstruction container, Operand iteratorAddress, int numberOfLoads,
     int indirectionIndex
   ) {
+    // Direct use
     exists(Instruction begin, Instruction memory, int upper, int ind |
-      isChiBeforeIteratorUse(iteratorDerefAddress, memory, numberOfLoads) and
+      isChiBeforeIteratorUse(iteratorAddress, memory, numberOfLoads) and
       memorySucc*(begin, memory) and
       isChiAfterBegin(container, begin) and
       upper = countIndirectionsForCppType(getResultLanguageType(container)) and
       ind = numberOfLoads + [1 .. upper] and
       indirectionIndex = ind - (numberOfLoads + 1)
     )
+    or
+    // Use through function output
+    exists(Instruction memory, Instruction begin, int upper, int ind |
+      isChiAfterIteratorArgument(memory, iteratorAddress, numberOfLoads) and
+      memorySucc*(begin, memory) and
+      isChiAfterBegin(container, begin) and
+      upper = countIndirectionsForCppType(getResultLanguageType(container)) and
+      ind = numberOfLoads + [1 .. upper] and
+      indirectionIndex = ind - (numberOfLoads - 1)
+    )
   }
 
   /** Holds if `op` is the only use of its defining instruction, and that op is used in a conversation */