Merge pull request #5737 from dbartol/dbartol/smart-pointers/work

C++: IR Alias Analysis for smart pointers

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

@@ -694,7 +694,12 @@ private predicate exprToExprStep_nocfg(Expr fromExpr, Expr toExpr) {
           fromExpr = call.getQualifier()
         ) and
         call.getTarget() = f and
-        outModel.isReturnValue()
+        // AST dataflow treats a reference as if it were the referred-to object, while the dataflow
+        // models treat references as pointers. If the return type of the call is a reference, then
+        // look for data flow the the referred-to object, rather than the reference itself.
+        if call.getType().getUnspecifiedType() instanceof ReferenceType
+        then outModel.isReturnValueDeref()
+        else outModel.isReturnValue()
       )
     )
 }

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

@@ -1645,6 +1645,19 @@ class CallInstruction extends Instruction {
    * Gets the number of arguments of the call, including the `this` pointer, if any.
    */
   final int getNumberOfArguments() { result = count(this.getAnArgumentOperand()) }
+
+  /**
+   * Holds if the result is a side effect for the argument at the specified index, or `this` if
+   * `index` is `-1`.
+   *
+   * This helper predicate makes it easy to join on both of these columns at once, avoiding
+   * pathological join orders in case the argument index should get joined first.
+   */
+  pragma[noinline]
+  final SideEffectInstruction getAParameterSideEffect(int index) {
+    this = result.getPrimaryInstruction() and
+    index = result.(IndexedInstruction).getIndex()
+  }
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll

@@ -4,6 +4,71 @@ private import AliasAnalysisImports
 
 private class IntValue = Ints::IntValue;
 
+/**
+ * If `instr` is a `SideEffectInstruction`, gets the primary `CallInstruction` that caused the side
+ * effect. If `instr` is a `CallInstruction`, gets that same `CallInstruction`.
+ */
+private CallInstruction getPrimaryCall(Instruction instr) {
+  result = instr
+  or
+  result = instr.(SideEffectInstruction).getPrimaryInstruction()
+}
+
+/**
+ * Holds if `operand` serves as an input argument (or indirection) to `call`, in the position
+ * specified by `input`.
+ */
+private predicate isCallInput(
+  CallInstruction call, Operand operand, AliasModels::FunctionInput input
+) {
+  call = getPrimaryCall(operand.getUse()) and
+  (
+    exists(int index |
+      input.isParameterOrQualifierAddress(index) and
+      operand = call.getArgumentOperand(index)
+    )
+    or
+    exists(int index, ReadSideEffectInstruction read |
+      input.isParameterDerefOrQualifierObject(index) and
+      read = call.getAParameterSideEffect(index) and
+      operand = read.getSideEffectOperand()
+    )
+  )
+}
+
+/**
+ * Holds if `instr` serves as a return value or output argument indirection for `call`, in the
+ * position specified by `output`.
+ */
+private predicate isCallOutput(
+  CallInstruction call, Instruction instr, AliasModels::FunctionOutput output
+) {
+  call = getPrimaryCall(instr) and
+  (
+    output.isReturnValue() and instr = call
+    or
+    exists(int index, WriteSideEffectInstruction write |
+      output.isParameterDerefOrQualifierObject(index) and
+      write = call.getAParameterSideEffect(index) and
+      instr = write
+    )
+  )
+}
+
+/**
+ * Holds if the address in `operand` flows directly to the result of `resultInstr` due to modeled
+ * address flow through a function call.
+ */
+private predicate hasAddressFlowThroughCall(Operand operand, Instruction resultInstr) {
+  exists(
+    CallInstruction call, AliasModels::FunctionInput input, AliasModels::FunctionOutput output
+  |
+    call.getStaticCallTarget().(AliasModels::AliasFunction).hasAddressFlow(input, output) and
+    isCallInput(call, operand, input) and
+    isCallOutput(call, resultInstr, output)
+  )
+}
+
 /**
  * Holds if the operand `tag` of instruction `instr` is used in a way that does
  * not result in any address held in that operand from escaping beyond the
@@ -34,7 +99,7 @@ private predicate operandIsConsumedWithoutEscaping(Operand operand) {
 
 private predicate operandEscapesDomain(Operand operand) {
   not operandIsConsumedWithoutEscaping(operand) and
-  not operandIsPropagated(operand, _) and
+  not operandIsPropagated(operand, _, _) and
   not isArgumentForParameter(_, operand, _) and
   not isOnlyEscapesViaReturnArgument(operand) and
   not operand.getUse() instanceof ReturnValueInstruction and
@@ -69,67 +134,67 @@ IntValue getPointerBitOffset(PointerOffsetInstruction instr) {
 }
 
 /**
- * Holds if any address held in operand `tag` of instruction `instr` is
- * propagated to the result of `instr`, offset by the number of bits in
- * `bitOffset`. If the address is propagated, but the offset is not known to be
- * a constant, then `bitOffset` is unknown.
+ * Holds if any address held in operand `operand` is propagated to the result of `instr`, offset by
+ * the number of bits in `bitOffset`. If the address is propagated, but the offset is not known to
+ * be a constant, then `bitOffset` is `unknown()`.
  */
-private predicate operandIsPropagated(Operand operand, IntValue bitOffset) {
-  exists(Instruction instr |
-    instr = operand.getUse() and
-    (
-      // Converting to a non-virtual base class adds the offset of the base class.
-      exists(ConvertToNonVirtualBaseInstruction convert |
-        convert = instr and
-        bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8)
-      )
-      or
-      // Conversion using dynamic_cast results in an unknown offset
-      instr instanceof CheckedConvertOrNullInstruction and
-      bitOffset = Ints::unknown()
-      or
-      // Converting to a derived class subtracts the offset of the base class.
-      exists(ConvertToDerivedInstruction convert |
-        convert = instr and
-        bitOffset = Ints::neg(Ints::mul(convert.getDerivation().getByteOffset(), 8))
-      )
-      or
-      // Converting to a virtual base class adds an unknown offset.
-      instr instanceof ConvertToVirtualBaseInstruction and
-      bitOffset = Ints::unknown()
-      or
-      // Conversion to another pointer type propagates the source address.
-      exists(ConvertInstruction convert, IRType resultType |
-        convert = instr and
-        resultType = convert.getResultIRType() and
-        resultType instanceof IRAddressType and
-        bitOffset = 0
-      )
-      or
-      // Adding an integer to or subtracting an integer from a pointer propagates
-      // the address with an offset.
-      exists(PointerOffsetInstruction ptrOffset |
-        ptrOffset = instr and
-        operand = ptrOffset.getLeftOperand() and
-        bitOffset = getPointerBitOffset(ptrOffset)
-      )
-      or
-      // Computing a field address from a pointer propagates the address plus the
-      // offset of the field.
-      bitOffset = Language::getFieldBitOffset(instr.(FieldAddressInstruction).getField())
-      or
-      // A copy propagates the source value.
-      operand = instr.(CopyInstruction).getSourceValueOperand() and bitOffset = 0
-      or
-      // Some functions are known to propagate an argument
-      isAlwaysReturnedArgument(operand) and bitOffset = 0
+private predicate operandIsPropagated(Operand operand, IntValue bitOffset, Instruction instr) {
+  // Some functions are known to propagate an argument
+  hasAddressFlowThroughCall(operand, instr) and
+  bitOffset = 0
+  or
+  instr = operand.getUse() and
+  (
+    // Converting to a non-virtual base class adds the offset of the base class.
+    exists(ConvertToNonVirtualBaseInstruction convert |
+      convert = instr and
+      bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8)
     )
+    or
+    // Conversion using dynamic_cast results in an unknown offset
+    instr instanceof CheckedConvertOrNullInstruction and
+    bitOffset = Ints::unknown()
+    or
+    // Converting to a derived class subtracts the offset of the base class.
+    exists(ConvertToDerivedInstruction convert |
+      convert = instr and
+      bitOffset = Ints::neg(Ints::mul(convert.getDerivation().getByteOffset(), 8))
+    )
+    or
+    // Converting to a virtual base class adds an unknown offset.
+    instr instanceof ConvertToVirtualBaseInstruction and
+    bitOffset = Ints::unknown()
+    or
+    // Conversion to another pointer type propagates the source address.
+    exists(ConvertInstruction convert, IRType resultType |
+      convert = instr and
+      resultType = convert.getResultIRType() and
+      resultType instanceof IRAddressType and
+      bitOffset = 0
+    )
+    or
+    // Adding an integer to or subtracting an integer from a pointer propagates
+    // the address with an offset.
+    exists(PointerOffsetInstruction ptrOffset |
+      ptrOffset = instr and
+      operand = ptrOffset.getLeftOperand() and
+      bitOffset = getPointerBitOffset(ptrOffset)
+    )
+    or
+    // Computing a field address from a pointer propagates the address plus the
+    // offset of the field.
+    bitOffset = Language::getFieldBitOffset(instr.(FieldAddressInstruction).getField())
+    or
+    // A copy propagates the source value.
+    operand = instr.(CopyInstruction).getSourceValueOperand() and bitOffset = 0
   )
 }
 
 private predicate operandEscapesNonReturn(Operand operand) {
-  // The address is propagated to the result of the instruction, and that result itself is returned
-  operandIsPropagated(operand, _) and resultEscapesNonReturn(operand.getUse())
+  exists(Instruction instr |
+    // The address is propagated to the result of the instruction, and that result itself is returned
+    operandIsPropagated(operand, _, instr) and resultEscapesNonReturn(instr)
+  )
   or
   // The operand is used in a function call which returns it, and the return value is then returned
   exists(CallInstruction ci, Instruction init |
@@ -151,9 +216,11 @@ private predicate operandEscapesNonReturn(Operand operand) {
 }
 
 private predicate operandMayReachReturn(Operand operand) {
-  // The address is propagated to the result of the instruction, and that result itself is returned
-  operandIsPropagated(operand, _) and
-  resultMayReachReturn(operand.getUse())
+  exists(Instruction instr |
+    // The address is propagated to the result of the instruction, and that result itself is returned
+    operandIsPropagated(operand, _, instr) and
+    resultMayReachReturn(instr)
+  )
   or
   // The operand is used in a function call which returns it, and the return value is then returned
   exists(CallInstruction ci, Instruction init |
@@ -173,9 +240,9 @@ private predicate operandMayReachReturn(Operand operand) {
 
 private predicate operandReturned(Operand operand, IntValue bitOffset) {
   // The address is propagated to the result of the instruction, and that result itself is returned
-  exists(IntValue bitOffset1, IntValue bitOffset2 |
-    operandIsPropagated(operand, bitOffset1) and
-    resultReturned(operand.getUse(), bitOffset2) and
+  exists(Instruction instr, IntValue bitOffset1, IntValue bitOffset2 |
+    operandIsPropagated(operand, bitOffset1, instr) and
+    resultReturned(instr, bitOffset2) and
     bitOffset = Ints::add(bitOffset1, bitOffset2)
   )
   or
@@ -214,13 +281,6 @@ private predicate isArgumentForParameter(
   )
 }
 
-private predicate isAlwaysReturnedArgument(Operand operand) {
-  exists(AliasModels::AliasFunction f |
-    f = operand.getUse().(CallInstruction).getStaticCallTarget() and
-    f.parameterIsAlwaysReturned(operand.(PositionalArgumentOperand).getIndex())
-  )
-}
-
 private predicate isOnlyEscapesViaReturnArgument(Operand operand) {
   exists(AliasModels::AliasFunction f |
     f = operand.getUse().(CallInstruction).getStaticCallTarget() and
@@ -270,12 +330,15 @@ predicate allocationEscapes(Configuration::Allocation allocation) {
 /**
  * Equivalent to `operandIsPropagated()`, but includes interprocedural propagation.
  */
-private predicate operandIsPropagatedIncludingByCall(Operand operand, IntValue bitOffset) {
-  operandIsPropagated(operand, bitOffset)
+private predicate operandIsPropagatedIncludingByCall(
+  Operand operand, IntValue bitOffset, Instruction instr
+) {
+  operandIsPropagated(operand, bitOffset, instr)
   or
   exists(CallInstruction call, Instruction init |
     isArgumentForParameter(call, operand, init) and
-    resultReturned(init, bitOffset)
+    resultReturned(init, bitOffset) and
+    instr = call
   )
 }
 
@@ -292,8 +355,7 @@ private predicate hasBaseAndOffset(AddressOperand addrOperand, Instruction base,
     // We already have an offset from `middle`.
     hasBaseAndOffset(addrOperand, middle, previousBitOffset) and
     // `middle` is propagated from `base`.
-    middleOperand = middle.getAnOperand() and
-    operandIsPropagatedIncludingByCall(middleOperand, additionalBitOffset) and
+    operandIsPropagatedIncludingByCall(middleOperand, additionalBitOffset, middle) and
     base = middleOperand.getDef() and
     bitOffset = Ints::add(previousBitOffset, additionalBitOffset)
   )

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasConfiguration.qll

@@ -105,7 +105,21 @@ class DynamicAllocation extends Allocation, TDynamicAllocation {
   DynamicAllocation() { this = TDynamicAllocation(call) }
 
   final override string toString() {
-    result = call.toString() + " at " + call.getLocation() // This isn't performant, but it's only used in test/dump code right now.
+    // This isn't performant, but it's only used in test/dump code right now.
+    // Dynamic allocations within a function are numbered in the order by start
+    // line number. This keeps them stable when the function moves within the
+    // file, or when non-allocating lines are added and removed within the
+    // function.
+    exists(int i |
+      result = "dynamic{" + i.toString() + "}" and
+      call =
+        rank[i](CallInstruction rangeCall |
+          exists(TDynamicAllocation(rangeCall)) and
+          rangeCall.getEnclosingIRFunction() = call.getEnclosingIRFunction()
+        |
+          rangeCall order by rangeCall.getLocation().getStartLine()
+        )
+    )
   }
 
   final override CallInstruction getABaseInstruction() { result = call }

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

@@ -1645,6 +1645,19 @@ class CallInstruction extends Instruction {
    * Gets the number of arguments of the call, including the `this` pointer, if any.
    */
   final int getNumberOfArguments() { result = count(this.getAnArgumentOperand()) }
+
+  /**
+   * Holds if the result is a side effect for the argument at the specified index, or `this` if
+   * `index` is `-1`.
+   *
+   * This helper predicate makes it easy to join on both of these columns at once, avoiding
+   * pathological join orders in case the argument index should get joined first.
+   */
+  pragma[noinline]
+  final SideEffectInstruction getAParameterSideEffect(int index) {
+    this = result.getPrimaryInstruction() and
+    index = result.(IndexedInstruction).getIndex()
+  }
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/Instruction.qll

@@ -1645,6 +1645,19 @@ class CallInstruction extends Instruction {
    * Gets the number of arguments of the call, including the `this` pointer, if any.
    */
   final int getNumberOfArguments() { result = count(this.getAnArgumentOperand()) }
+
+  /**
+   * Holds if the result is a side effect for the argument at the specified index, or `this` if
+   * `index` is `-1`.
+   *
+   * This helper predicate makes it easy to join on both of these columns at once, avoiding
+   * pathological join orders in case the argument index should get joined first.
+   */
+  pragma[noinline]
+  final SideEffectInstruction getAParameterSideEffect(int index) {
+    this = result.getPrimaryInstruction() and
+    index = result.(IndexedInstruction).getIndex()
+  }
 }
 
 /**

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll

@@ -4,6 +4,71 @@ private import AliasAnalysisImports
 
 private class IntValue = Ints::IntValue;
 
+/**
+ * If `instr` is a `SideEffectInstruction`, gets the primary `CallInstruction` that caused the side
+ * effect. If `instr` is a `CallInstruction`, gets that same `CallInstruction`.
+ */
+private CallInstruction getPrimaryCall(Instruction instr) {
+  result = instr
+  or
+  result = instr.(SideEffectInstruction).getPrimaryInstruction()
+}
+
+/**
+ * Holds if `operand` serves as an input argument (or indirection) to `call`, in the position
+ * specified by `input`.
+ */
+private predicate isCallInput(
+  CallInstruction call, Operand operand, AliasModels::FunctionInput input
+) {
+  call = getPrimaryCall(operand.getUse()) and
+  (
+    exists(int index |
+      input.isParameterOrQualifierAddress(index) and
+      operand = call.getArgumentOperand(index)
+    )
+    or
+    exists(int index, ReadSideEffectInstruction read |
+      input.isParameterDerefOrQualifierObject(index) and
+      read = call.getAParameterSideEffect(index) and
+      operand = read.getSideEffectOperand()
+    )
+  )
+}
+
+/**
+ * Holds if `instr` serves as a return value or output argument indirection for `call`, in the
+ * position specified by `output`.
+ */
+private predicate isCallOutput(
+  CallInstruction call, Instruction instr, AliasModels::FunctionOutput output
+) {
+  call = getPrimaryCall(instr) and
+  (
+    output.isReturnValue() and instr = call
+    or
+    exists(int index, WriteSideEffectInstruction write |
+      output.isParameterDerefOrQualifierObject(index) and
+      write = call.getAParameterSideEffect(index) and
+      instr = write
+    )
+  )
+}
+
+/**
+ * Holds if the address in `operand` flows directly to the result of `resultInstr` due to modeled
+ * address flow through a function call.
+ */
+private predicate hasAddressFlowThroughCall(Operand operand, Instruction resultInstr) {
+  exists(
+    CallInstruction call, AliasModels::FunctionInput input, AliasModels::FunctionOutput output
+  |
+    call.getStaticCallTarget().(AliasModels::AliasFunction).hasAddressFlow(input, output) and
+    isCallInput(call, operand, input) and
+    isCallOutput(call, resultInstr, output)
+  )
+}
+
 /**
  * Holds if the operand `tag` of instruction `instr` is used in a way that does
  * not result in any address held in that operand from escaping beyond the
@@ -34,7 +99,7 @@ private predicate operandIsConsumedWithoutEscaping(Operand operand) {
 
 private predicate operandEscapesDomain(Operand operand) {
   not operandIsConsumedWithoutEscaping(operand) and
-  not operandIsPropagated(operand, _) and
+  not operandIsPropagated(operand, _, _) and
   not isArgumentForParameter(_, operand, _) and
   not isOnlyEscapesViaReturnArgument(operand) and
   not operand.getUse() instanceof ReturnValueInstruction and
@@ -69,67 +134,67 @@ IntValue getPointerBitOffset(PointerOffsetInstruction instr) {
 }
 
 /**
- * Holds if any address held in operand `tag` of instruction `instr` is
- * propagated to the result of `instr`, offset by the number of bits in
- * `bitOffset`. If the address is propagated, but the offset is not known to be
- * a constant, then `bitOffset` is unknown.
+ * Holds if any address held in operand `operand` is propagated to the result of `instr`, offset by
+ * the number of bits in `bitOffset`. If the address is propagated, but the offset is not known to
+ * be a constant, then `bitOffset` is `unknown()`.
  */
-private predicate operandIsPropagated(Operand operand, IntValue bitOffset) {
-  exists(Instruction instr |
-    instr = operand.getUse() and
-    (
-      // Converting to a non-virtual base class adds the offset of the base class.
-      exists(ConvertToNonVirtualBaseInstruction convert |
-        convert = instr and
-        bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8)
-      )
-      or
-      // Conversion using dynamic_cast results in an unknown offset
-      instr instanceof CheckedConvertOrNullInstruction and
-      bitOffset = Ints::unknown()
-      or
-      // Converting to a derived class subtracts the offset of the base class.
-      exists(ConvertToDerivedInstruction convert |
-        convert = instr and
-        bitOffset = Ints::neg(Ints::mul(convert.getDerivation().getByteOffset(), 8))
-      )
-      or
-      // Converting to a virtual base class adds an unknown offset.
-      instr instanceof ConvertToVirtualBaseInstruction and
-      bitOffset = Ints::unknown()
-      or
-      // Conversion to another pointer type propagates the source address.
-      exists(ConvertInstruction convert, IRType resultType |
-        convert = instr and
-        resultType = convert.getResultIRType() and
-        resultType instanceof IRAddressType and
-        bitOffset = 0
-      )
-      or
-      // Adding an integer to or subtracting an integer from a pointer propagates
-      // the address with an offset.
-      exists(PointerOffsetInstruction ptrOffset |
-        ptrOffset = instr and
-        operand = ptrOffset.getLeftOperand() and
-        bitOffset = getPointerBitOffset(ptrOffset)
-      )
-      or
-      // Computing a field address from a pointer propagates the address plus the
-      // offset of the field.
-      bitOffset = Language::getFieldBitOffset(instr.(FieldAddressInstruction).getField())
-      or
-      // A copy propagates the source value.
-      operand = instr.(CopyInstruction).getSourceValueOperand() and bitOffset = 0
-      or
-      // Some functions are known to propagate an argument
-      isAlwaysReturnedArgument(operand) and bitOffset = 0
+private predicate operandIsPropagated(Operand operand, IntValue bitOffset, Instruction instr) {
+  // Some functions are known to propagate an argument
+  hasAddressFlowThroughCall(operand, instr) and
+  bitOffset = 0
+  or
+  instr = operand.getUse() and
+  (
+    // Converting to a non-virtual base class adds the offset of the base class.
+    exists(ConvertToNonVirtualBaseInstruction convert |
+      convert = instr and
+      bitOffset = Ints::mul(convert.getDerivation().getByteOffset(), 8)
     )
+    or
+    // Conversion using dynamic_cast results in an unknown offset
+    instr instanceof CheckedConvertOrNullInstruction and
+    bitOffset = Ints::unknown()
+    or
+    // Converting to a derived class subtracts the offset of the base class.
+    exists(ConvertToDerivedInstruction convert |
+      convert = instr and
+      bitOffset = Ints::neg(Ints::mul(convert.getDerivation().getByteOffset(), 8))
+    )
+    or
+    // Converting to a virtual base class adds an unknown offset.
+    instr instanceof ConvertToVirtualBaseInstruction and
+    bitOffset = Ints::unknown()
+    or
+    // Conversion to another pointer type propagates the source address.
+    exists(ConvertInstruction convert, IRType resultType |
+      convert = instr and
+      resultType = convert.getResultIRType() and
+      resultType instanceof IRAddressType and
+      bitOffset = 0
+    )
+    or
+    // Adding an integer to or subtracting an integer from a pointer propagates
+    // the address with an offset.
+    exists(PointerOffsetInstruction ptrOffset |
+      ptrOffset = instr and
+      operand = ptrOffset.getLeftOperand() and
+      bitOffset = getPointerBitOffset(ptrOffset)
+    )
+    or
+    // Computing a field address from a pointer propagates the address plus the
+    // offset of the field.
+    bitOffset = Language::getFieldBitOffset(instr.(FieldAddressInstruction).getField())
+    or
+    // A copy propagates the source value.
+    operand = instr.(CopyInstruction).getSourceValueOperand() and bitOffset = 0
   )
 }
 
 private predicate operandEscapesNonReturn(Operand operand) {
-  // The address is propagated to the result of the instruction, and that result itself is returned
-  operandIsPropagated(operand, _) and resultEscapesNonReturn(operand.getUse())
+  exists(Instruction instr |
+    // The address is propagated to the result of the instruction, and that result itself is returned
+    operandIsPropagated(operand, _, instr) and resultEscapesNonReturn(instr)
+  )
   or
   // The operand is used in a function call which returns it, and the return value is then returned
   exists(CallInstruction ci, Instruction init |
@@ -151,9 +216,11 @@ private predicate operandEscapesNonReturn(Operand operand) {
 }
 
 private predicate operandMayReachReturn(Operand operand) {
-  // The address is propagated to the result of the instruction, and that result itself is returned
-  operandIsPropagated(operand, _) and
-  resultMayReachReturn(operand.getUse())
+  exists(Instruction instr |
+    // The address is propagated to the result of the instruction, and that result itself is returned
+    operandIsPropagated(operand, _, instr) and
+    resultMayReachReturn(instr)
+  )
   or
   // The operand is used in a function call which returns it, and the return value is then returned
   exists(CallInstruction ci, Instruction init |
@@ -173,9 +240,9 @@ private predicate operandMayReachReturn(Operand operand) {
 
 private predicate operandReturned(Operand operand, IntValue bitOffset) {
   // The address is propagated to the result of the instruction, and that result itself is returned
-  exists(IntValue bitOffset1, IntValue bitOffset2 |
-    operandIsPropagated(operand, bitOffset1) and
-    resultReturned(operand.getUse(), bitOffset2) and
+  exists(Instruction instr, IntValue bitOffset1, IntValue bitOffset2 |
+    operandIsPropagated(operand, bitOffset1, instr) and
+    resultReturned(instr, bitOffset2) and
     bitOffset = Ints::add(bitOffset1, bitOffset2)
   )
   or
@@ -214,13 +281,6 @@ private predicate isArgumentForParameter(
   )
 }
 
-private predicate isAlwaysReturnedArgument(Operand operand) {
-  exists(AliasModels::AliasFunction f |
-    f = operand.getUse().(CallInstruction).getStaticCallTarget() and
-    f.parameterIsAlwaysReturned(operand.(PositionalArgumentOperand).getIndex())
-  )
-}
-
 private predicate isOnlyEscapesViaReturnArgument(Operand operand) {
   exists(AliasModels::AliasFunction f |
     f = operand.getUse().(CallInstruction).getStaticCallTarget() and
@@ -270,12 +330,15 @@ predicate allocationEscapes(Configuration::Allocation allocation) {
 /**
  * Equivalent to `operandIsPropagated()`, but includes interprocedural propagation.
  */
-private predicate operandIsPropagatedIncludingByCall(Operand operand, IntValue bitOffset) {
-  operandIsPropagated(operand, bitOffset)
+private predicate operandIsPropagatedIncludingByCall(
+  Operand operand, IntValue bitOffset, Instruction instr
+) {
+  operandIsPropagated(operand, bitOffset, instr)
   or
   exists(CallInstruction call, Instruction init |
     isArgumentForParameter(call, operand, init) and
-    resultReturned(init, bitOffset)
+    resultReturned(init, bitOffset) and
+    instr = call
   )
 }
 
@@ -292,8 +355,7 @@ private predicate hasBaseAndOffset(AddressOperand addrOperand, Instruction base,
     // We already have an offset from `middle`.
     hasBaseAndOffset(addrOperand, middle, previousBitOffset) and
     // `middle` is propagated from `base`.
-    middleOperand = middle.getAnOperand() and
-    operandIsPropagatedIncludingByCall(middleOperand, additionalBitOffset) and
+    operandIsPropagatedIncludingByCall(middleOperand, additionalBitOffset, middle) and
     base = middleOperand.getDef() and
     bitOffset = Ints::add(previousBitOffset, additionalBitOffset)
   )

cpp/ql/src/semmle/code/cpp/models/implementations/SmartPointer.qll

@@ -1,12 +1,14 @@
+import semmle.code.cpp.models.interfaces.Alias
+import semmle.code.cpp.models.interfaces.SideEffect
 import semmle.code.cpp.models.interfaces.Taint
 import semmle.code.cpp.models.interfaces.DataFlow
 import semmle.code.cpp.models.interfaces.PointerWrapper
 
 /**
- * The `std::shared_ptr` and `std::unique_ptr` template classes.
+ * The `std::shared_ptr`, `std::weak_ptr`, and `std::unique_ptr` template classes.
  */
-private class UniqueOrSharedPtr extends Class, PointerWrapper {
-  UniqueOrSharedPtr() { this.hasQualifiedName(["std", "bsl"], ["shared_ptr", "unique_ptr"]) }
+private class SmartPtr extends Class, PointerWrapper {
+  SmartPtr() { this.hasQualifiedName(["std", "bsl"], ["shared_ptr", "weak_ptr", "unique_ptr"]) }
 
   override MemberFunction getAnUnwrapperFunction() {
     result.(OverloadedPointerDereferenceFunction).getDeclaringType() = this
@@ -17,11 +19,19 @@ private class UniqueOrSharedPtr extends Class, PointerWrapper {
   override predicate pointsToConst() { this.getTemplateArgument(0).(Type).isConst() }
 }
 
-/** Any function that unwraps a pointer wrapper class to reveal the underlying pointer. */
-private class PointerWrapperFlow extends TaintFunction, DataFlowFunction {
-  PointerWrapperFlow() {
-    this = any(PointerWrapper wrapper).getAnUnwrapperFunction() and
-    not this.getUnspecifiedType() instanceof ReferenceType
+/**
+ * Any function that returns the address wrapped by a `PointerWrapper`, whether as a pointer or a
+ * reference.
+ *
+ * Examples:
+ * - `std::unique_ptr<T>::get()`
+ * - `std::shared_ptr<T>::operator->()`
+ * - `std::weak_ptr<T>::operator*()`
+ */
+private class PointerUnwrapperFunction extends MemberFunction, TaintFunction, DataFlowFunction,
+  SideEffectFunction, AliasFunction {
+  PointerUnwrapperFunction() {
+    exists(PointerWrapper wrapper | wrapper.getAnUnwrapperFunction() = this)
   }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
@@ -32,6 +42,23 @@ private class PointerWrapperFlow extends TaintFunction, DataFlowFunction {
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
     input.isQualifierObject() and output.isReturnValue()
   }
+
+  override predicate hasOnlySpecificReadSideEffects() { any() }
+
+  override predicate hasOnlySpecificWriteSideEffects() { any() }
+
+  override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
+    // Only reads from `*this`.
+    i = -1 and buffer = false
+  }
+
+  override predicate parameterNeverEscapes(int index) { index = -1 }
+
+  override predicate parameterEscapesOnlyViaReturn(int index) { none() }
+
+  override predicate hasAddressFlow(FunctionInput input, FunctionOutput output) {
+    input.isQualifierObject() and output.isReturnValue()
+  }
 }
 
 /**
@@ -62,31 +89,80 @@ private class MakeUniqueOrShared extends TaintFunction {
 }
 
 /**
- * A prefix `operator*` member function for a `shared_ptr` or `unique_ptr` type.
+ * A function that sets the value of a smart pointer.
+ *
+ * This could be a constructor, an assignment operator, or a named member function like `reset()`.
  */
-private class UniqueOrSharedDereferenceMemberOperator extends MemberFunction, TaintFunction {
-  UniqueOrSharedDereferenceMemberOperator() {
-    this.hasName("operator*") and
-    this.getDeclaringType() instanceof UniqueOrSharedPtr
+private class SmartPtrSetterFunction extends MemberFunction, AliasFunction, SideEffectFunction {
+  SmartPtrSetterFunction() {
+    this.getDeclaringType() instanceof SmartPtr and
+    not this.isStatic() and
+    (
+      this instanceof Constructor
+      or
+      this.hasName("operator=")
+      or
+      this.hasName("reset")
+    )
   }
 
-  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
-    input.isQualifierObject() and
-    output.isReturnValueDeref()
-  }
-}
+  override predicate hasOnlySpecificReadSideEffects() { none() }
 
-/**
- * The `std::shared_ptr` or `std::unique_ptr` function `get`.
- */
-private class UniqueOrSharedGet extends TaintFunction {
-  UniqueOrSharedGet() {
-    this.hasName("get") and
-    this.getDeclaringType() instanceof UniqueOrSharedPtr
+  override predicate hasOnlySpecificWriteSideEffects() { none() }
+
+  override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
+    // Always write to the destination smart pointer itself.
+    i = -1 and buffer = false and mustWrite = true
+    or
+    // When taking ownership of a smart pointer via an rvalue reference, always overwrite the input
+    // smart pointer.
+    getPointerInput().isParameterDeref(i) and
+    this.getParameter(i).getUnspecifiedType() instanceof RValueReferenceType and
+    buffer = false and
+    mustWrite = true
   }
 
-  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
-    input.isQualifierObject() and
+  override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
+    getPointerInput().isParameterDeref(i) and
+    buffer = false
+    or
+    not this instanceof Constructor and
+    i = -1 and
+    buffer = false
+  }
+
+  override predicate parameterNeverEscapes(int index) { index = -1 }
+
+  override predicate parameterEscapesOnlyViaReturn(int index) { none() }
+
+  override predicate hasAddressFlow(FunctionInput input, FunctionOutput output) {
+    input = getPointerInput() and
+    output.isQualifierObject()
+    or
+    // Assignment operator always returns a reference to `*this`.
+    this.hasName("operator=") and
+    input.isQualifierAddress() and
     output.isReturnValue()
   }
+
+  private FunctionInput getPointerInput() {
+    exists(Parameter param0 |
+      param0 = this.getParameter(0) and
+      (
+        param0.getUnspecifiedType().(ReferenceType).getBaseType() instanceof SmartPtr and
+        if this.getParameter(1).getUnspecifiedType() instanceof PointerType
+        then
+          // This is one of the constructors of `std::shared_ptr<T>` that creates a smart pointer that
+          // wraps a raw pointer with ownership controlled by an unrelated smart pointer. We propagate
+          // the raw pointer in the second parameter, rather than the smart pointer in the first
+          // parameter.
+          result.isParameter(1)
+        else result.isParameterDeref(0)
+      )
+      or
+      // One of the functions that takes ownership of a raw pointer.
+      param0.getUnspecifiedType() instanceof PointerType and
+      result.isParameter(0)
+    )
+  }
 }

cpp/ql/src/semmle/code/cpp/models/interfaces/Alias.qll

@@ -50,5 +50,16 @@ abstract class AliasFunction extends Function {
   /**
    * Holds if the function always returns the value of the parameter at the specified index.
    */
-  abstract predicate parameterIsAlwaysReturned(int index);
+  predicate parameterIsAlwaysReturned(int index) { none() }
+
+  /**
+   * Holds if the address passed in via `input` is always propagated to `output`.
+   */
+  predicate hasAddressFlow(FunctionInput input, FunctionOutput output) {
+    exists(int index |
+      // By default, just use the old `parameterIsAlwaysReturned` predicate to detect flow from the
+      // parameter to the return value.
+      input.isParameter(index) and output.isReturnValue() and this.parameterIsAlwaysReturned(index)
+    )
+  }
 }