Merge pull request #4432 from dbartol/dbartol/temporaries/work

C++: Represent temporary object initialization in AST and IR

Author: igfoo

cpp/ql/src/semmle/code/cpp/dataflow/EscapesTree.qll

@@ -83,6 +83,8 @@ private predicate pointerToPointerStep(Expr pointerIn, Expr pointerOut) {
   or
   pointerIn.getConversion() = pointerOut.(ParenthesisExpr)
   or
+  pointerIn.getConversion() = pointerOut.(TemporaryObjectExpr)
+  or
   pointerIn = pointerOut.(ConditionalExpr).getThen().getFullyConverted()
   or
   pointerIn = pointerOut.(ConditionalExpr).getElse().getFullyConverted()

cpp/ql/src/semmle/code/cpp/dataflow/internal/AddressFlow.qll

@@ -81,6 +81,8 @@ private predicate pointerToPointerStep(Expr pointerIn, Expr pointerOut) {
   or
   pointerIn.getConversion() = pointerOut.(ParenthesisExpr)
   or
+  pointerIn.getConversion() = pointerOut.(TemporaryObjectExpr)
+  or
   pointerIn = pointerOut.(ConditionalExpr).getThen().getFullyConverted()
   or
   pointerIn = pointerOut.(ConditionalExpr).getElse().getFullyConverted()

cpp/ql/src/semmle/code/cpp/exprs/Cast.qll

@@ -840,6 +840,28 @@ class ArrayToPointerConversion extends Conversion, @array_to_pointer {
   override predicate mayBeGloballyImpure() { none() }
 }
 
+/**
+ * A node representing a temporary object created as part of an expression.
+ *
+ * This is most commonly seen in the following cases:
+ * ```c++
+ * // when binding a reference to a prvalue
+ * const std::string& r = std::string("text");
+ *
+ * // when performing member access on a class prvalue
+ * strlen(std::string("text").c_str());
+ *
+ * // when a prvalue of a type with a destructor is discarded
+ * s.substr(0, 5);  // Return value is discarded but requires destruction
+ * ```
+ */
+class TemporaryObjectExpr extends Conversion, @temp_init {
+  /** Gets a textual representation of this conversion. */
+  override string toString() { result = "temporary object" }
+
+  override string getAPrimaryQlClass() { result = "TemporaryObjectExpr" }
+}
+
 /**
  * A node representing the Cast sub-class of entity `cast`.
  */

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

@@ -83,7 +83,7 @@ private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
   override predicate isSink(DataFlow::Node sink) { exists(adjustedSink(sink)) }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
-    instructionTaintStep(n1.asInstruction(), n2.asInstruction())
+    commonTaintStep(n1, n2)
   }
 
   override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
@@ -101,7 +101,7 @@ private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
   }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
-    instructionTaintStep(n1.asInstruction(), n2.asInstruction())
+    commonTaintStep(n1, n2)
     or
     writesVariable(n1.asInstruction(), n2.asVariable().(GlobalOrNamespaceVariable))
     or
@@ -125,7 +125,7 @@ private class FromGlobalVarTaintTrackingCfg extends DataFlow2::Configuration {
   override predicate isSink(DataFlow::Node sink) { exists(adjustedSink(sink)) }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
-    instructionTaintStep(n1.asInstruction(), n2.asInstruction())
+    commonTaintStep(n1, n2)
     or
     // Additional step for flow out of variables. There is no flow _into_
     // variables in this configuration, so this step only serves to take flow
@@ -215,19 +215,62 @@ private predicate nodeIsBarrierIn(DataFlow::Node node) {
 }
 
 cached
-private predicate instructionTaintStep(Instruction i1, Instruction i2) {
+private predicate commonTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+  instructionToInstructionTaintStep(fromNode.asInstruction(), toNode.asInstruction())
+  or
+  operandToInstructionTaintStep(fromNode.asOperand(), toNode.asInstruction())
+  or
+  operandToOperandTaintStep(fromNode.asOperand(), toNode.asOperand())
+}
+
+private predicate operandToOperandTaintStep(Operand fromOperand, Operand toOperand) {
+  exists(ReadSideEffectInstruction readInstr |
+    fromOperand = readInstr.getArgumentOperand() and
+    toOperand = readInstr.getSideEffectOperand()
+  )
+}
+
+private predicate operandToInstructionTaintStep(Operand fromOperand, Instruction toInstr) {
   // Expressions computed from tainted data are also tainted
-  exists(CallInstruction call, int argIndex | call = i2 |
+  exists(CallInstruction call, int argIndex | call = toInstr |
     isPureFunction(call.getStaticCallTarget().getName()) and
-    i1 = getACallArgumentOrIndirection(call, argIndex) and
-    forall(Instruction arg | arg = call.getAnArgument() |
-      arg = getACallArgumentOrIndirection(call, argIndex) or predictableInstruction(arg)
+    fromOperand = getACallArgumentOrIndirection(call, argIndex) and
+    forall(Operand argOperand | argOperand = call.getAnArgumentOperand() |
+      argOperand = getACallArgumentOrIndirection(call, argIndex) or
+      predictableInstruction(argOperand.getAnyDef())
     ) and
     // flow through `strlen` tends to cause dubious results, if the length is
     // bounded.
     not call.getStaticCallTarget().getName() = "strlen"
   )
   or
+  // Flow from argument to return value
+  toInstr =
+    any(CallInstruction call |
+      exists(int indexIn |
+        modelTaintToReturnValue(call.getStaticCallTarget(), indexIn) and
+        fromOperand = getACallArgumentOrIndirection(call, indexIn) and
+        not predictableOnlyFlow(call.getStaticCallTarget().getName())
+      )
+    )
+  or
+  // Flow from input argument to output argument
+  // TODO: This won't work in practice as long as all aliased memory is tracked
+  // together in a single virtual variable.
+  // TODO: Will this work on the test for `TaintedPath.ql`, where the output arg
+  // is a pointer addition expression?
+  toInstr =
+    any(WriteSideEffectInstruction outInstr |
+      exists(CallInstruction call, int indexIn, int indexOut |
+        modelTaintToParameter(call.getStaticCallTarget(), indexIn, indexOut) and
+        fromOperand = getACallArgumentOrIndirection(call, indexIn) and
+        outInstr.getIndex() = indexOut and
+        outInstr.getPrimaryInstruction() = call
+      )
+    )
+}
+
+private predicate instructionToInstructionTaintStep(Instruction i1, Instruction i2) {
   // Flow through pointer dereference
   i2.(LoadInstruction).getSourceAddress() = i1
   or
@@ -291,29 +334,6 @@ private predicate instructionTaintStep(Instruction i1, Instruction i2) {
     read.getAnOperand().(SideEffectOperand).getAnyDef() = i1 and
     read.getArgumentDef() = i2
   )
-  or
-  // Flow from argument to return value
-  i2 =
-    any(CallInstruction call |
-      exists(int indexIn |
-        modelTaintToReturnValue(call.getStaticCallTarget(), indexIn) and
-        i1 = getACallArgumentOrIndirection(call, indexIn) and
-        not predictableOnlyFlow(call.getStaticCallTarget().getName())
-      )
-    )
-  or
-  // Flow from input argument to output argument
-  // TODO: Will this work on the test for `TaintedPath.ql`, where the output arg
-  // is a pointer addition expression?
-  i2 =
-    any(WriteSideEffectInstruction outNode |
-      exists(CallInstruction call, int indexIn, int indexOut |
-        modelTaintToParameter(call.getStaticCallTarget(), indexIn, indexOut) and
-        i1 = getACallArgumentOrIndirection(call, indexIn) and
-        outNode.getIndex() = indexOut and
-        outNode.getPrimaryInstruction() = call
-      )
-    )
 }
 
 pragma[noinline]
@@ -329,15 +349,25 @@ private InitializeParameterInstruction getInitializeParameter(IRFunction f, Para
 }
 
 /**
- * Get an instruction that goes into argument `argumentIndex` of `call`. This
+ * Returns the index of the side effect instruction corresponding to the specified function output,
+ * if one exists.
+ */
+private int getWriteSideEffectIndex(FunctionOutput output) {
+  output.isParameterDeref(result)
+  or
+  output.isQualifierObject() and result = -1
+}
+
+/**
+ * Get an operand that goes into argument `argumentIndex` of `call`. This
  * can be either directly or through one pointer indirection.
  */
-private Instruction getACallArgumentOrIndirection(CallInstruction call, int argumentIndex) {
-  result = call.getPositionalArgument(argumentIndex)
+private Operand getACallArgumentOrIndirection(CallInstruction call, int argumentIndex) {
+  result = call.getPositionalArgumentOperand(argumentIndex)
   or
   exists(ReadSideEffectInstruction readSE |
     // TODO: why are read side effect operands imprecise?
-    result = readSE.getSideEffectOperand().getAnyDef() and
+    result = readSE.getSideEffectOperand() and
     readSE.getPrimaryInstruction() = call and
     readSE.getIndex() = argumentIndex
   )
@@ -351,7 +381,7 @@ private predicate modelTaintToParameter(Function f, int parameterIn, int paramet
       f.(TaintFunction).hasTaintFlow(modelIn, modelOut)
     ) and
     (modelIn.isParameter(parameterIn) or modelIn.isParameterDeref(parameterIn)) and
-    modelOut.isParameterDeref(parameterOut)
+    parameterOut = getWriteSideEffectIndex(modelOut)
   )
 }
 
@@ -540,7 +570,7 @@ module TaintedWithPath {
     }
 
     override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
-      instructionTaintStep(n1.asInstruction(), n2.asInstruction())
+      commonTaintStep(n1, n2)
       or
       exists(TaintTrackingConfiguration cfg | cfg.taintThroughGlobals() |
         writesVariable(n1.asInstruction(), n2.asVariable().(GlobalOrNamespaceVariable))

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRConsistency.qll

@@ -494,4 +494,34 @@ module InstructionConsistency {
       irFunc = getInstructionIRFunction(instr, irFuncText)
     )
   }
+
+  /**
+   * Holds if the object address operand for the given `FieldAddress` instruction does not have an
+   * address type.
+   */
+  query predicate fieldAddressOnNonPointer(
+    FieldAddressInstruction instr, string message, OptionalIRFunction irFunc, string irFuncText
+  ) {
+    not instr.getObjectAddressOperand().getIRType() instanceof IRAddressType and
+    message =
+      "FieldAddress instruction '" + instr.toString() +
+        "' has an object address operand that is not an address, in function '$@'." and
+    irFunc = getInstructionIRFunction(instr, irFuncText)
+  }
+
+  /**
+   * Holds if the `this` argument operand for the given `Call` instruction does not have an address
+   * type.
+   */
+  query predicate thisArgumentIsNonPointer(
+    CallInstruction instr, string message, OptionalIRFunction irFunc, string irFuncText
+  ) {
+    exists(ThisArgumentOperand thisOperand | thisOperand = instr.getThisArgumentOperand() |
+      not thisOperand.getIRType() instanceof IRAddressType
+    ) and
+    message =
+      "Call instruction '" + instr.toString() +
+        "' has a `this` argument operand that is not an address, in function '$@'." and
+    irFunc = getInstructionIRFunction(instr, irFuncText)
+  }
 }

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRConsistency.qll

@@ -494,4 +494,34 @@ module InstructionConsistency {
       irFunc = getInstructionIRFunction(instr, irFuncText)
     )
   }
+
+  /**
+   * Holds if the object address operand for the given `FieldAddress` instruction does not have an
+   * address type.
+   */
+  query predicate fieldAddressOnNonPointer(
+    FieldAddressInstruction instr, string message, OptionalIRFunction irFunc, string irFuncText
+  ) {
+    not instr.getObjectAddressOperand().getIRType() instanceof IRAddressType and
+    message =
+      "FieldAddress instruction '" + instr.toString() +
+        "' has an object address operand that is not an address, in function '$@'." and
+    irFunc = getInstructionIRFunction(instr, irFuncText)
+  }
+
+  /**
+   * Holds if the `this` argument operand for the given `Call` instruction does not have an address
+   * type.
+   */
+  query predicate thisArgumentIsNonPointer(
+    CallInstruction instr, string message, OptionalIRFunction irFunc, string irFuncText
+  ) {
+    exists(ThisArgumentOperand thisOperand | thisOperand = instr.getThisArgumentOperand() |
+      not thisOperand.getIRType() instanceof IRAddressType
+    ) and
+    message =
+      "Call instruction '" + instr.toString() +
+        "' has a `this` argument operand that is not an address, in function '$@'." and
+    irFunc = getInstructionIRFunction(instr, irFuncText)
+  }
 }

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

@@ -8,6 +8,16 @@ private import TranslatedElement
 private import TranslatedExpr
 private import TranslatedFunction
 
+/**
+ * Gets the `CallInstruction` from the `TranslatedCallExpr` for the specified expression.
+ */
+private CallInstruction getTranslatedCallInstruction(Call call) {
+  exists(TranslatedCallExpr translatedCall |
+    translatedCall.getExpr() = call and
+    result = translatedCall.getInstruction(CallTag())
+  )
+}
+
 /**
  * The IR translation of a call to a function. The call may be from an actual
  * call in the source code, or could be a call that is part of the translation
@@ -388,7 +398,7 @@ class TranslatedAllocationSideEffects extends TranslatedSideEffects,
     tag = OnlyInstructionTag() and
     if expr instanceof NewOrNewArrayExpr
     then result = getTranslatedAllocatorCall(expr).getInstruction(CallTag())
-    else result = getTranslatedExpr(expr).getInstruction(CallTag())
+    else result = getTranslatedCallInstruction(expr)
   }
 }
 
@@ -409,7 +419,7 @@ class TranslatedCallSideEffects extends TranslatedSideEffects, TTranslatedCallSi
 
   override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
     tag = OnlyInstructionTag() and
-    result = getTranslatedExpr(expr).getInstruction(CallTag())
+    result = getTranslatedCallInstruction(expr)
   }
 }
 
@@ -599,7 +609,7 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff
 
   override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
     tag = OnlyInstructionTag() and
-    result = getTranslatedExpr(call).getInstruction(CallTag())
+    result = getTranslatedCallInstruction(call)
   }
 
   final override int getInstructionIndex(InstructionTag tag) {