C++: Impoved alias analysis of smart pointers

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.(CallInstruction)
+  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
@@ -74,6 +139,10 @@ IntValue getPointerBitOffset(PointerOffsetInstruction instr) {
  * be a constant, then `bitOffset` is `unknown()`.
  */
 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.
@@ -118,9 +187,6 @@ private predicate operandIsPropagated(Operand operand, IntValue bitOffset, Instr
     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
   )
 }
 
@@ -215,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
@@ -271,7 +330,9 @@ predicate allocationEscapes(Configuration::Allocation allocation) {
 /**
  * Equivalent to `operandIsPropagated()`, but includes interprocedural propagation.
  */
-private predicate operandIsPropagatedIncludingByCall(Operand operand, IntValue bitOffset, Instruction instr) {
+private predicate operandIsPropagatedIncludingByCall(
+  Operand operand, IntValue bitOffset, Instruction instr
+) {
   operandIsPropagated(operand, bitOffset, instr)
   or
   exists(CallInstruction call, Instruction init |

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/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.(CallInstruction)
+  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
@@ -74,6 +139,10 @@ IntValue getPointerBitOffset(PointerOffsetInstruction instr) {
  * be a constant, then `bitOffset` is `unknown()`.
  */
 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.
@@ -118,9 +187,6 @@ private predicate operandIsPropagated(Operand operand, IntValue bitOffset, Instr
     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
   )
 }
 
@@ -215,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
@@ -271,7 +330,9 @@ predicate allocationEscapes(Configuration::Allocation allocation) {
 /**
  * Equivalent to `operandIsPropagated()`, but includes interprocedural propagation.
  */
-private predicate operandIsPropagatedIncludingByCall(Operand operand, IntValue bitOffset, Instruction instr) {
+private predicate operandIsPropagatedIncludingByCall(
+  Operand operand, IntValue bitOffset, Instruction instr
+) {
   operandIsPropagated(operand, bitOffset, instr)
   or
   exists(CallInstruction call, Instruction init |

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,36 @@ 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 PointerWrapperDataFlow extends DataFlowFunction {
-  PointerWrapperDataFlow() {
-    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, AliasFunction, DataFlowFunction,
+  SideEffectFunction, TaintFunction {
+  PointerUnwrapperFunction() {
+    exists(PointerWrapper wrapper | wrapper.getAnUnwrapperFunction() = this)
+  }
+
+  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()
   }
 
   override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
@@ -32,6 +59,11 @@ private class PointerWrapperDataFlow extends DataFlowFunction {
     input.isReturnValueDeref() and
     output.isQualifierObject()
   }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    input.isQualifierObject() and
+    output.isReturnValue()
+  }
 }
 
 /**
@@ -62,31 +94,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() { this instanceof Constructor }
 
-/**
- * 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() { this instanceof ConstMemberFunction }
+
+  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/test/library-tests/ir/points_to/points_to.ql

@@ -5,7 +5,16 @@ private import semmle.code.cpp.ir.internal.IntegerConstant as Ints
 private predicate ignoreAllocation(string name) {
   name = "i" or
   name = "p" or
-  name = "q"
+  name = "q" or
+  name = "s" or
+  name = "t" or
+  name = "?{AllAliased}"
+}
+
+private predicate ignoreFile(File file) {
+  // Ignore standard headers.
+  file.getBaseName() = ["memory.h", "type_traits.h", "utility.h"] or
+  not file.fromSource()
 }
 
 module Raw {
@@ -29,7 +38,8 @@ module Raw {
         not ignoreAllocation(memLocation.getAllocation().getAllocationString()) and
         value = memLocation.toString() and
         element = instr.toString() and
-        location = instr.getLocation()
+        location = instr.getLocation() and
+        not ignoreFile(location.getFile())
       )
     }
   }
@@ -52,13 +62,14 @@ module UnaliasedSSA {
     override predicate hasActualResult(Location location, string element, string tag, string value) {
       exists(Instruction instr, MemoryLocation memLocation |
         memLocation = getAMemoryAccess(instr) and
-        not memLocation instanceof AliasedVirtualVariable and
+        not memLocation.getVirtualVariable() instanceof AliasedVirtualVariable and
         not memLocation instanceof AllNonLocalMemory and
         tag = "ussa" and
         not ignoreAllocation(memLocation.getAllocation().getAllocationString()) and
         value = memLocation.toString() and
         element = instr.toString() and
-        location = instr.getLocation()
+        location = instr.getLocation() and
+        not ignoreFile(location.getFile())
       )
     }
   }

cpp/ql/test/library-tests/ir/points_to/smart_pointer.cpp

@@ -0,0 +1,33 @@
+#include "../../../include/memory.h"
+#include "../../../include/utility.h"
+
+using std::shared_ptr;
+using std::unique_ptr;
+
+struct S {
+    int x;
+};
+
+void unique_ptr_init(S s) {
+    unique_ptr<S> p(new S); //$ussa=dynamic{1}
+    int i = (*p).x; //$ussa=dynamic{1}[0..4)<int>
+    *p = s;  //$ussa=dynamic{1}[0..4)<S>
+    unique_ptr<S> q = std::move(p);
+    *(q.get()) = s;  //$ussa=dynamic{1}[0..4)<S>
+    shared_ptr<S> t(std::move(q));
+    t->x = 5; //$ussa=dynamic{1}[0..4)<int>
+    *t = s; //$ussa=dynamic{1}[0..4)<S>
+    *(t.get()) = s; //$ussa=dynamic{1}[0..4)<S>
+}
+
+void shared_ptr_init(S s) {
+    shared_ptr<S> p(new S); //$ussa=dynamic{1}
+    int i = (*p).x; //$ussa=dynamic{1}[0..4)<int>
+    *p = s;  //$ussa=dynamic{1}[0..4)<S>
+    shared_ptr<S> q = std::move(p);
+    *(q.get()) = s;  //$ussa=dynamic{1}[0..4)<S>
+    shared_ptr<S> t(q);
+    t->x = 5; //$ussa=dynamic{1}[0..4)<int>
+    *t = s; //$ussa=dynamic{1}[0..4)<S>
+    *(t.get()) = s; //$ussa=dynamic{1}[0..4)<S>
+}