C++: Fix TODO by blocking summary flow through functions that don't preserve identity.

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

@@ -7,6 +7,9 @@ private import SsaInternals as Ssa
 private import DataFlowImplCommon as DataFlowImplCommon
 private import codeql.util.Unit
 private import Node0ToString
+private import ModelUtil
+private import semmle.code.cpp.models.interfaces.FunctionInputsAndOutputs as IO
+private import semmle.code.cpp.models.interfaces.DataFlow as DF
 
 cached
 private module Cached {
@@ -1178,6 +1181,19 @@ private int countNumberOfBranchesUsingParameter(SwitchInstruction switch, Parame
   )
 }
 
+pragma[nomagic]
+private predicate isInputOutput(
+  DF::DataFlowFunction target, Node node1, Node node2, IO::FunctionInput input,
+  IO::FunctionOutput output
+) {
+  exists(CallInstruction call |
+    node1 = callInput(call, input) and
+    node2 = callOutput(call, output) and
+    call.getStaticCallTarget() = target and
+    target.hasDataFlow(input, output)
+  )
+}
+
 /**
  * Holds if the data-flow step from `node1` to `node2` can be used to
  * determine where side-effects may return from a callable.
@@ -1189,6 +1205,11 @@ private int countNumberOfBranchesUsingParameter(SwitchInstruction switch, Parame
  * int x = *p;
  * ```
  * does not preserve the identity of `*p`.
+ *
+ * Similarly, functions that copy the contents of a string into a new location
+ * does not also preserve the identity. For example, `strdup(p)` does not
+ * preserve the identity of `*p` (since it allocates new storage and copies
+ * the string into the new storage).
  */
 bindingset[node1, node2]
 pragma[inline_late]
@@ -1225,7 +1246,16 @@ predicate validParameterAliasStep(Node node1, Node node2) {
   not exists(Operand operand |
     node1.asOperand() = operand and
     node2.asInstruction().(StoreInstruction).getSourceValueOperand() = operand
+  ) and
+  (
+    // Either this is not a modeled flow.
+    not isInputOutput(_, node1, node2, _, _)
+    or
+    exists(DF::DataFlowFunction target, IO::FunctionInput input, IO::FunctionOutput output |
+      // Or it is a modeled flow and there's `*input` to `*output` flow
+      isInputOutput(target, node1, node2, input.getIndirectionInput(), output.getIndirectionOutput()) and
+      // and in that case there should also be `input` to `output` flow
+      target.hasDataFlow(input, output)
+    )
   )
-  // TODO: Also block flow through models that don't preserve identity such
-  // as `strdup`.
 }

cpp/ql/lib/semmle/code/cpp/models/interfaces/FunctionInputsAndOutputs.qll

@@ -23,6 +23,14 @@ private newtype TFunctionInput =
 class FunctionInput extends TFunctionInput {
   abstract string toString();
 
+  /**
+   * INTERNAL: Do not use.
+   *
+   * Gets the `FunctionInput` that represents the indirection of this input,
+   * if any.
+   */
+  FunctionInput getIndirectionInput() { none() }
+
   /**
    * Holds if this is the input value of the parameter with index `index`.
    *
@@ -226,6 +234,8 @@ class InParameter extends FunctionInput, TInParameter {
   ParameterIndex getIndex() { result = index }
 
   override predicate isParameter(ParameterIndex i) { i = index }
+
+  override FunctionInput getIndirectionInput() { result = TInParameterDeref(index, 1) }
 }
 
 /**
@@ -257,6 +267,10 @@ class InParameterDeref extends FunctionInput, TInParameterDeref {
   override predicate isParameterDeref(ParameterIndex i, int indirection) {
     i = index and indirectionIndex = indirection
   }
+
+  override FunctionInput getIndirectionInput() {
+    result = TInParameterDeref(index, indirectionIndex + 1)
+  }
 }
 
 /**
@@ -275,6 +289,8 @@ class InQualifierObject extends FunctionInput, TInQualifierObject {
   override string toString() { result = "InQualifierObject" }
 
   override predicate isQualifierObject() { any() }
+
+  override FunctionInput getIndirectionInput() { none() }
 }
 
 /**
@@ -293,6 +309,8 @@ class InQualifierAddress extends FunctionInput, TInQualifierAddress {
   override string toString() { result = "InQualifierAddress" }
 
   override predicate isQualifierAddress() { any() }
+
+  override FunctionInput getIndirectionInput() { result = TInQualifierObject() }
 }
 
 /**
@@ -321,6 +339,8 @@ class InReturnValueDeref extends FunctionInput, TInReturnValueDeref {
   override string toString() { result = "InReturnValueDeref" }
 
   override predicate isReturnValueDeref() { any() }
+
+  override FunctionInput getIndirectionInput() { none() }
 }
 
 private newtype TFunctionOutput =
@@ -340,6 +360,14 @@ private newtype TFunctionOutput =
 class FunctionOutput extends TFunctionOutput {
   abstract string toString();
 
+  /**
+   * INTERNAL: Do not use.
+   *
+   * Gets the `FunctionOutput` that represents the indirection of this output,
+   * if any.
+   */
+  FunctionOutput getIndirectionOutput() { none() }
+
   /**
    * Holds if this is the output value pointed to by a pointer parameter to a function, or the
    * output value referred to by a reference parameter to a function, where the parameter has
@@ -512,6 +540,10 @@ class OutParameterDeref extends FunctionOutput, TOutParameterDeref {
   override predicate isParameterDeref(ParameterIndex i, int ind) {
     i = index and ind = indirectionIndex
   }
+
+  override FunctionOutput getIndirectionOutput() {
+    result = TOutParameterDeref(index, indirectionIndex + 1)
+  }
 }
 
 /**
@@ -530,6 +562,8 @@ class OutQualifierObject extends FunctionOutput, TOutQualifierObject {
   override string toString() { result = "OutQualifierObject" }
 
   override predicate isQualifierObject() { any() }
+
+  override FunctionOutput getIndirectionOutput() { none() }
 }
 
 /**
@@ -552,6 +586,8 @@ class OutReturnValue extends FunctionOutput, TOutReturnValue {
   override string toString() { result = "OutReturnValue" }
 
   override predicate isReturnValue() { any() }
+
+  override FunctionOutput getIndirectionOutput() { result = TOutReturnValueDeref(1) }
 }
 
 /**
@@ -571,11 +607,19 @@ class OutReturnValue extends FunctionOutput, TOutReturnValue {
  *   of `getInt()` is neither a pointer nor a reference.
  */
 class OutReturnValueDeref extends FunctionOutput, TOutReturnValueDeref {
+  int indirectionIndex;
+
+  OutReturnValueDeref() { this = TOutReturnValueDeref(indirectionIndex) }
+
   override string toString() { result = "OutReturnValueDeref" }
 
   override predicate isReturnValueDeref() { any() }
 
-  override predicate isReturnValueDeref(int indirectionIndex) {
-    this = TOutReturnValueDeref(indirectionIndex)
+  override predicate isReturnValueDeref(int indirectionIndex_) {
+    indirectionIndex = indirectionIndex_
+  }
+
+  override FunctionOutput getIndirectionOutput() {
+    result = TOutReturnValueDeref(indirectionIndex + 1)
   }
 }