C++: Respond to review comments.

Author: MathiasVP

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

@@ -326,7 +326,7 @@ private class VariablePartialDefinitionNode extends PartialDefinitionNode {
  * A synthetic data flow node used for flow into a collection when an iterator
  * write occurs in a callee.
  */
-class IteratorPartialDefinitionNode extends PartialDefinitionNode {
+private class IteratorPartialDefinitionNode extends PartialDefinitionNode {
   override IteratorPartialDefinition pd;
 
   override Node getPreUpdateNode() { pd.definesExpressions(_, result.asExpr()) }
@@ -338,7 +338,7 @@ class IteratorPartialDefinitionNode extends PartialDefinitionNode {
  * A synthetic data flow node used for flow into a collection when a smart pointer
  * write occurs in a callee.
  */
-class SmartPointerPartialDefinitionNode extends PartialDefinitionNode {
+private class SmartPointerPartialDefinitionNode extends PartialDefinitionNode {
   override SmartPointerPartialDefinition pd;
 
   override Node getPreUpdateNode() { pd.definesExpressions(_, result.asExpr()) }

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

@@ -159,18 +159,14 @@ private module PartialDefinitions {
     Expr innerDefinedExpr;
 
     IteratorPartialDefinition() {
-      exists(Expr convertedInner |
-        not this instanceof Conversion and
-        valueToUpdate(convertedInner, this.getFullyConverted(), node) and
-        innerDefinedExpr = convertedInner.getUnconverted() and
-        (
-          innerDefinedExpr.(Call).getQualifier() = getAnIteratorAccess(collection)
-          or
-          innerDefinedExpr.(Call).getQualifier() = collection.getAnAccess() and
-          collection instanceof IteratorParameter
-        ) and
-        innerDefinedExpr.(Call).getTarget() instanceof IteratorPointerDereferenceMemberOperator
-      )
+      innerDefinedExpr = getInnerDefinedExpr(this, node) and
+      (
+        innerDefinedExpr.(Call).getQualifier() = getAnIteratorAccess(collection)
+        or
+        innerDefinedExpr.(Call).getQualifier() = collection.getAnAccess() and
+        collection instanceof IteratorParameter
+      ) and
+      innerDefinedExpr.(Call).getTarget() instanceof IteratorPointerDereferenceMemberOperator
       or
       // iterators passed by value without a copy constructor
       exists(Call call |
@@ -208,16 +204,18 @@ private module PartialDefinitions {
     }
   }
 
+  private Expr getInnerDefinedExpr(Expr e, ControlFlowNode node) {
+    not e instanceof Conversion and
+    exists(Expr convertedInner |
+      valueToUpdate(convertedInner, e.getFullyConverted(), node) and
+      result = convertedInner.getUnconverted()
+    )
+  }
+
   class VariablePartialDefinition extends PartialDefinition {
     Expr innerDefinedExpr;
 
-    VariablePartialDefinition() {
-      not this instanceof Conversion and
-      exists(Expr convertedInner |
-        valueToUpdate(convertedInner, this.getFullyConverted(), node) and
-        innerDefinedExpr = convertedInner.getUnconverted()
-      )
-    }
+    VariablePartialDefinition() { innerDefinedExpr = getInnerDefinedExpr(this, node) }
 
     deprecated override predicate partiallyDefines(Variable v) {
       innerDefinedExpr = v.getAnAccess()
@@ -249,30 +247,15 @@ private module PartialDefinitions {
     Expr innerDefinedExpr;
 
     SmartPointerPartialDefinition() {
-      exists(Expr convertedInner |
-        not this instanceof Conversion and
-        valueToUpdate(convertedInner, this.getFullyConverted(), node) and
-        innerDefinedExpr = convertedInner.getUnconverted() and
-        innerDefinedExpr = getAPointerWrapperAccess(pointer)
-      )
+      innerDefinedExpr = pragma[only_bind_out](getInnerDefinedExpr(this, node)) and
+      innerDefinedExpr = getAPointerWrapperAccess(pointer, -1)
       or
-      // pointer wrappers passed by value without a copy constructor
+      // Pointer wrappers passed to a function by value.
       exists(Call call |
         call = node and
         call.getAnArgument() = innerDefinedExpr and
         innerDefinedExpr = this and
-        this = getAPointerWrapperAccess(pointer) and
-        not call instanceof OverloadedPointerDereferenceExpr
-      )
-      or
-      // pointer wrappers passed by value with a copy constructor
-      exists(Call call, ConstructorCall copy |
-        copy.getTarget() instanceof CopyConstructor and
-        call = node and
-        call.getAnArgument() = copy and
-        copy.getArgument(0) = getAPointerWrapperAccess(pointer) and
-        innerDefinedExpr = this and
-        this = copy and
+        this = getAPointerWrapperAccess(pointer, 0) and
         not call instanceof OverloadedPointerDereferenceExpr
       )
     }
@@ -893,9 +876,20 @@ module FlowVar_internal {
     )
   }
 
-  Call getAPointerWrapperAccess(Variable pointer) {
-    pointer.getUnspecifiedType() instanceof PointerWrapper and
-    [result.getQualifier(), result.getAnArgument()] = pointer.getAnAccess()
+  /**
+   * 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 {

cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

@@ -3223,7 +3223,6 @@
 | smart_pointer.cpp:11:30:11:50 | call to make_shared | smart_pointer.cpp:12:11:12:11 | p |  |
 | smart_pointer.cpp:11:30:11:50 | call to make_shared | smart_pointer.cpp:13:10:13:10 | p |  |
 | smart_pointer.cpp:11:52:11:57 | call to source | smart_pointer.cpp:11:30:11:50 | call to make_shared | TAINT |
-| smart_pointer.cpp:12:10:12:10 | call to operator* [post update] | smart_pointer.cpp:13:10:13:10 | p |  |
 | smart_pointer.cpp:12:11:12:11 | p | smart_pointer.cpp:12:10:12:10 | call to operator* | TAINT |
 | smart_pointer.cpp:12:11:12:11 | ref arg p | smart_pointer.cpp:13:10:13:10 | p |  |
 | smart_pointer.cpp:17:32:17:54 | call to make_shared | smart_pointer.cpp:18:11:18:11 | p |  |
@@ -3234,7 +3233,6 @@
 | smart_pointer.cpp:23:30:23:50 | call to make_unique | smart_pointer.cpp:24:11:24:11 | p |  |
 | smart_pointer.cpp:23:30:23:50 | call to make_unique | smart_pointer.cpp:25:10:25:10 | p |  |
 | smart_pointer.cpp:23:52:23:57 | call to source | smart_pointer.cpp:23:30:23:50 | call to make_unique | TAINT |
-| smart_pointer.cpp:24:10:24:10 | call to operator* [post update] | smart_pointer.cpp:25:10:25:10 | p |  |
 | smart_pointer.cpp:24:11:24:11 | p | smart_pointer.cpp:24:10:24:10 | call to operator* | TAINT |
 | smart_pointer.cpp:24:11:24:11 | ref arg p | smart_pointer.cpp:25:10:25:10 | p |  |
 | smart_pointer.cpp:29:32:29:54 | call to make_unique | smart_pointer.cpp:30:11:30:11 | p |  |