C++: Replace the new SmartPointerPartialDefinition with additional steps in AddressFlow.qll

Author: MathiasVP

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

@@ -15,6 +15,7 @@
  */
 
 private import cpp
+private import semmle.code.cpp.models.interfaces.PointerWrapper
 
 /**
  * Holds if `f` is an instantiation of the `std::move` or `std::forward`
@@ -58,6 +59,8 @@ private predicate pointerToLvalueStep(Expr pointerIn, Expr lvalueOut) {
   pointerIn = lvalueOut.(ArrayExpr).getArrayBase().getFullyConverted()
   or
   pointerIn = lvalueOut.(PointerDereferenceExpr).getOperand().getFullyConverted()
+  or
+  pointerIn = lvalueOut.(OverloadedPointerDereferenceExpr).getQualifier().getFullyConverted()
 }
 
 private predicate lvalueToPointerStep(Expr lvalueIn, Expr pointerOut) {
@@ -94,6 +97,12 @@ private predicate pointerToPointerStep(Expr pointerIn, Expr pointerOut) {
 
 private predicate lvalueToReferenceStep(Expr lvalueIn, Expr referenceOut) {
   lvalueIn.getConversion() = referenceOut.(ReferenceToExpr)
+  or
+  exists(PointerWrapper wrapper, Call call | call = referenceOut |
+    referenceOut.getUnspecifiedType() instanceof ReferenceType and
+    call = wrapper.getAnUnwrapperFunction().getACallToThisFunction() and
+    lvalueIn = call.getQualifier().getFullyConverted()
+  )
 }
 
 private predicate referenceToLvalueStep(Expr referenceIn, Expr lvalueOut) {
@@ -106,6 +115,13 @@ private predicate referenceToPointerStep(Expr referenceIn, Expr pointerOut) {
       stdAddressOf(call.getTarget()) and
       referenceIn = call.getArgument(0).getFullyConverted()
     )
+  or
+  exists(CopyConstructor copy, Call call | call = pointerOut |
+    copy.getDeclaringType() instanceof PointerWrapper and
+    call.getTarget() = copy and
+    // The 0'th argument is the value being copied.
+    referenceIn = call.getArgument(0).getFullyConverted()
+  )
 }
 
 private predicate referenceToReferenceStep(Expr referenceIn, Expr referenceOut) {
@@ -190,6 +206,19 @@ private predicate pointerToUpdate(Expr pointer, Expr outer, ControlFlowNode node
         // See the `lvalueToUpdate` case for an explanation of this conjunct.
         call.getType().isDeeplyConstBelow()
       )
+      or
+      // Pointer wrappers behave as raw pointers for dataflow purposes.
+      outer = call.getAnArgument().getFullyConverted() and
+      exists(PointerWrapper wrapper | wrapper = outer.getType().stripTopLevelSpecifiers() |
+        not wrapper.pointsToConst()
+      )
+      or
+      outer = call.getQualifier().getFullyConverted() and
+      outer.getUnspecifiedType() instanceof PointerWrapper and
+      not (
+        call.getTarget().hasSpecifier("const") and
+        call.getType().isDeeplyConstBelow()
+      )
     )
     or
     exists(PointerFieldAccess fa |

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

@@ -199,8 +199,6 @@ class DefinitionByReferenceOrIteratorNode extends PartialDefinitionNode {
       this.getPartialDefinition() instanceof DefinitionByReference
       or
       this.getPartialDefinition() instanceof DefinitionByIterator
-      or
-      this.getPartialDefinition() instanceof DefinitionBySmartPointer
     )
   }
 
@@ -332,18 +330,6 @@ private class IteratorPartialDefinitionNode extends PartialDefinitionNode {
   override Node getPreUpdateNode() { pd.definesExpressions(_, result.asExpr()) }
 }
 
-/**
- * INTERNAL: do not use.
- *
- * A synthetic data flow node used for flow into a collection when a smart pointer
- * write occurs in a callee.
- */
-private class SmartPointerPartialDefinitionNode extends PartialDefinitionNode {
-  override SmartPointerPartialDefinition pd;
-
-  override Node getPreUpdateNode() { pd.definesExpressions(_, result.asExpr()) }
-}
-
 /**
  * A post-update node on the `e->f` in `f(&e->f)` (and other forms).
  */

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

@@ -242,39 +242,6 @@ private module PartialDefinitions {
     }
   }
 
-  class SmartPointerPartialDefinition extends PartialDefinition {
-    Variable pointer;
-    Expr innerDefinedExpr;
-
-    SmartPointerPartialDefinition() {
-      innerDefinedExpr = pragma[only_bind_out](getInnerDefinedExpr(this, node)) and
-      innerDefinedExpr = getAPointerWrapperAccess(pointer, -1)
-      or
-      // Pointer wrappers passed to a function by value.
-      exists(Call call |
-        call = node and
-        call.getAnArgument() = innerDefinedExpr and
-        innerDefinedExpr = this and
-        this = getAPointerWrapperAccess(pointer, 0) and
-        not call instanceof OverloadedPointerDereferenceExpr
-      )
-    }
-
-    deprecated override predicate partiallyDefines(Variable v) { v = pointer }
-
-    deprecated override predicate partiallyDefinesThis(ThisExpr e) { none() }
-
-    override predicate definesExpressions(Expr inner, Expr outer) {
-      inner = innerDefinedExpr and
-      outer = this
-    }
-
-    override predicate partiallyDefinesVariableAt(Variable v, ControlFlowNode cfn) {
-      v = pointer and
-      cfn = node
-    }
-  }
-
   /**
    * A partial definition that's a definition via an output iterator.
    */
@@ -288,15 +255,6 @@ private module PartialDefinitions {
   class DefinitionByReference extends VariablePartialDefinition {
     DefinitionByReference() { exists(Call c | this = c.getAnArgument() or this = c.getQualifier()) }
   }
-
-  /**
-   * A partial definition that's a definition via a smart pointer being passed into a function.
-   */
-  class DefinitionBySmartPointer extends SmartPointerPartialDefinition {
-    DefinitionBySmartPointer() {
-      exists(Call c | this = c.getAnArgument() or this = c.getQualifier())
-    }
-  }
 }
 
 import PartialDefinitions
@@ -876,22 +834,6 @@ module FlowVar_internal {
     )
   }
 
-  /**
-   * Gets either:
-   * - A call to an unwrapper function, where an access to `pointer` is the qualifier, or
-   * - A call to the `CopyConstructor` that copies the value of an access to `pointer`.
-   */
-  Call getAPointerWrapperAccess(Variable pointer, int n) {
-    exists(PointerWrapper wrapper | wrapper = pointer.getUnspecifiedType().stripType() |
-      n = -1 and
-      result.getQualifier() = pointer.getAnAccess() and
-      result.getTarget() = wrapper.getAnUnwrapperFunction()
-      or
-      result.getArgument(n) = pointer.getAnAccess() and
-      result.getTarget() instanceof CopyConstructor
-    )
-  }
-
   class IteratorParameter extends Parameter {
     IteratorParameter() { this.getUnspecifiedType() instanceof Iterator }
   }

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

@@ -13,6 +13,8 @@ private class UniqueOrSharedPtr extends Class, PointerWrapper {
     or
     result.getClassAndName(["operator->", "get"]) = this
   }
+
+  override predicate pointsToConst() { this.getTemplateArgument(0).(Type).isConst() }
 }
 
 /** Any function that unwraps a pointer wrapper class to reveal the underlying pointer. */

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

@@ -11,4 +11,7 @@ abstract class PointerWrapper extends Class {
    * that return a reference to the pointed-to object.
    */
   abstract MemberFunction getAnUnwrapperFunction();
+
+  /** Holds if the type of the data that is pointed to by this pointer wrapper is `const`. */
+  abstract predicate pointsToConst();
 }