Cherry picking commit bbf9bcde2a (#21)

Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>

cpp/ql/lib/change-notes/2023-10-12-additional-call-targets.md

@@ -0,0 +1,4 @@
+---
+category: feature
+---
+* Added a new class `AdditionalCallTarget` for specifying additional call targets.

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

@@ -7,9 +7,12 @@ private import DataFlowImplCommon as DataFlowImplCommon
 
 /**
  * Gets a function that might be called by `call`.
+ *
+ * This predicate does not take additional call targets
+ * from `AdditionalCallTarget` into account.
  */
 cached
-DataFlowCallable viableCallable(DataFlowCall call) {
+DataFlowCallable defaultViableCallable(DataFlowCall call) {
   DataFlowImplCommon::forceCachingInSameStage() and
   result = call.getStaticCallTarget()
   or
@@ -29,6 +32,17 @@ DataFlowCallable viableCallable(DataFlowCall call) {
   result = call.(VirtualDispatch::DataSensitiveCall).resolve()
 }
 
+/**
+ * Gets a function that might be called by `call`.
+ */
+cached
+DataFlowCallable viableCallable(DataFlowCall call) {
+  result = defaultViableCallable(call)
+  or
+  // Additional call targets
+  result = any(AdditionalCallTarget additional).viableTarget(call.getUnconvertedResultExpression())
+}
+
 /**
  * Provides virtual dispatch support compatible with the original
  * implementation of `semmle.code.cpp.security.TaintTracking`.

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

@@ -14,6 +14,7 @@ private import DataFlowPrivate
 private import ModelUtil
 private import SsaInternals as Ssa
 private import DataFlowImplCommon as DataFlowImplCommon
+private import codeql.util.Unit
 
 /**
  * The IR dataflow graph consists of the following nodes:
@@ -1696,16 +1697,7 @@ private module Cached {
     // Reverse flow: data that flows from the definition node back into the indirection returned
     // by a function. This allows data to flow 'in' through references returned by a modeled
     // function such as `operator[]`.
-    exists(Operand address, int indirectionIndex |
-      nodeHasOperand(nodeTo.(IndirectReturnOutNode), address, indirectionIndex)
-    |
-      exists(StoreInstruction store |
-        nodeHasInstruction(nodeFrom, store, indirectionIndex - 1) and
-        store.getDestinationAddressOperand() = address
-      )
-      or
-      Ssa::outNodeHasAddressAndIndex(nodeFrom, address, indirectionIndex)
-    )
+    reverseFlow(nodeFrom, nodeTo)
   }
 
   private predicate simpleInstructionLocalFlowStep(Operand opFrom, Instruction iTo) {
@@ -1736,6 +1728,39 @@ private module Cached {
       )
     )
   }
+
+  private predicate reverseFlow(Node nodeFrom, Node nodeTo) {
+    reverseFlowOperand(nodeFrom, nodeTo)
+    or
+    reverseFlowInstruction(nodeFrom, nodeTo)
+  }
+
+  private predicate reverseFlowOperand(Node nodeFrom, IndirectReturnOutNode nodeTo) {
+    exists(Operand address, int indirectionIndex |
+      nodeHasOperand(nodeTo, address, indirectionIndex)
+    |
+      exists(StoreInstruction store |
+        nodeHasInstruction(nodeFrom, store, indirectionIndex - 1) and
+        store.getDestinationAddressOperand() = address
+      )
+      or
+      // We also want a write coming out of an `OutNode` to flow `nodeTo`.
+      // This is different from `reverseFlowInstruction` since `nodeFrom` can never
+      // be an `OutNode` when it's defined by an instruction.
+      Ssa::outNodeHasAddressAndIndex(nodeFrom, address, indirectionIndex)
+    )
+  }
+
+  private predicate reverseFlowInstruction(Node nodeFrom, IndirectReturnOutNode nodeTo) {
+    exists(Instruction address, int indirectionIndex |
+      nodeHasInstruction(nodeTo, address, indirectionIndex)
+    |
+      exists(StoreInstruction store |
+        nodeHasInstruction(nodeFrom, store, indirectionIndex - 1) and
+        store.getDestinationAddress() = address
+      )
+    )
+  }
 }
 
 import Cached
@@ -2215,33 +2240,41 @@ module InstructionBarrierGuard<instructionGuardChecksSig/3 instructionGuardCheck
 }
 
 /**
- * DEPRECATED: Use `BarrierGuard` module instead.
+ * A unit class for adding additional call steps.
  *
- * A guard that validates some instruction.
+ * Extend this class to add additional call steps to the data flow graph.
  *
- * To use this in a configuration, extend the class and provide a
- * characteristic predicate precisely specifying the guard, and override
- * `checks` to specify what is being validated and in which branch.
+ * For example, if the following subclass is added:
+ * ```ql
+ * class MyAdditionalCallTarget extends DataFlow::AdditionalCallTarget {
+ *   override Function viableTarget(Call call) {
+ *     call.getTarget().hasName("f") and
+ *     result.hasName("g")
+ *   }
+ * }
+ * ```
+ * then flow from `source()` to `x` in `sink(x)` is reported in the following example:
+ * ```cpp
+ * void sink(int);
+ * int source();
+ * void f(int);
  *
- * It is important that all extending classes in scope are disjoint.
+ * void g(int x) {
+ *   sink(x);
+ * }
+ *
+ * void test() {
+ *   int x = source();
+ *   f(x);
+ * }
+ * ```
+ *
+ * Note: To prevent reevaluation of cached dataflow-related predicates any
+ * subclass of `AdditionalCallTarget` must be imported in all dataflow queries.
  */
-deprecated class BarrierGuard extends IRGuardCondition {
-  /** Override this predicate to hold if this guard validates `instr` upon evaluating to `b`. */
-  predicate checksInstr(Instruction instr, boolean b) { none() }
-
-  /** Override this predicate to hold if this guard validates `expr` upon evaluating to `b`. */
-  predicate checks(Expr e, boolean b) { none() }
-
-  /** Gets a node guarded by this guard. */
-  final Node getAGuardedNode() {
-    exists(ValueNumber value, boolean edge |
-      (
-        this.checksInstr(value.getAnInstruction(), edge)
-        or
-        this.checks(value.getAnInstruction().getConvertedResultExpression(), edge)
-      ) and
-      result.asInstruction() = value.getAnInstruction() and
-      this.controls(result.asInstruction().getBlock(), edge)
-    )
-  }
+class AdditionalCallTarget extends Unit {
+  /**
+   * Gets a viable target for `call`.
+   */
+  abstract DataFlowCallable viableTarget(Call call);
 }

cpp/ql/test/library-tests/dataflow/fields/IRConfiguration.qll

@@ -1,6 +1,19 @@
 private import semmle.code.cpp.ir.dataflow.DataFlow
 private import DataFlow
 
+private class TestAdditionalCallTarget extends AdditionalCallTarget {
+  override Function viableTarget(Call call) {
+    // To test that call targets specified by `AdditionalCallTarget` are
+    // resolved correctly this subclass resolves all calls to
+    // `call_template_argument<f>(x)` as if the user had written `f(x)`.
+    exists(FunctionTemplateInstantiation inst |
+      inst.getTemplate().hasName("call_template_argument") and
+      call.getTarget() = inst and
+      result = inst.getTemplateArgument(0).(FunctionAccess).getTarget()
+    )
+  }
+}
+
 module IRConfig implements ConfigSig {
   predicate isSource(Node src) {
     src.asExpr() instanceof NewExpr

cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected

@@ -770,6 +770,9 @@ edges
 | simple.cpp:92:7:92:7 | a indirection [post update] [i] | simple.cpp:94:10:94:11 | a2 indirection [i] |
 | simple.cpp:92:11:92:20 | call to user_input | simple.cpp:92:5:92:22 | ... = ... |
 | simple.cpp:94:10:94:11 | a2 indirection [i] | simple.cpp:94:13:94:13 | i |
+| simple.cpp:103:24:103:24 | x | simple.cpp:104:14:104:14 | x |
+| simple.cpp:108:17:108:26 | call to user_input | simple.cpp:109:43:109:43 | x |
+| simple.cpp:109:43:109:43 | x | simple.cpp:103:24:103:24 | x |
 | struct_init.c:14:24:14:25 | ab indirection [a] | struct_init.c:15:8:15:9 | ab indirection [a] |
 | struct_init.c:15:8:15:9 | ab indirection [a] | struct_init.c:15:12:15:12 | a |
 | struct_init.c:20:13:20:14 | definition of ab indirection [a] | struct_init.c:22:8:22:9 | ab indirection [a] |
@@ -1576,6 +1579,10 @@ nodes
 | simple.cpp:92:11:92:20 | call to user_input | semmle.label | call to user_input |
 | simple.cpp:94:10:94:11 | a2 indirection [i] | semmle.label | a2 indirection [i] |
 | simple.cpp:94:13:94:13 | i | semmle.label | i |
+| simple.cpp:103:24:103:24 | x | semmle.label | x |
+| simple.cpp:104:14:104:14 | x | semmle.label | x |
+| simple.cpp:108:17:108:26 | call to user_input | semmle.label | call to user_input |
+| simple.cpp:109:43:109:43 | x | semmle.label | x |
 | struct_init.c:14:24:14:25 | ab indirection [a] | semmle.label | ab indirection [a] |
 | struct_init.c:15:8:15:9 | ab indirection [a] | semmle.label | ab indirection [a] |
 | struct_init.c:15:12:15:12 | a | semmle.label | a |
@@ -1782,6 +1789,7 @@ subpaths
 | simple.cpp:67:13:67:13 | i | simple.cpp:65:11:65:20 | call to user_input | simple.cpp:67:13:67:13 | i | i flows from $@ | simple.cpp:65:11:65:20 | call to user_input | call to user_input |
 | simple.cpp:84:14:84:20 | call to getf2f1 | simple.cpp:83:17:83:26 | call to user_input | simple.cpp:84:14:84:20 | call to getf2f1 | call to getf2f1 flows from $@ | simple.cpp:83:17:83:26 | call to user_input | call to user_input |
 | simple.cpp:94:13:94:13 | i | simple.cpp:92:11:92:20 | call to user_input | simple.cpp:94:13:94:13 | i | i flows from $@ | simple.cpp:92:11:92:20 | call to user_input | call to user_input |
+| simple.cpp:104:14:104:14 | x | simple.cpp:108:17:108:26 | call to user_input | simple.cpp:104:14:104:14 | x | x flows from $@ | simple.cpp:108:17:108:26 | call to user_input | call to user_input |
 | struct_init.c:15:12:15:12 | a | struct_init.c:20:20:20:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:20:20:20:29 | call to user_input | call to user_input |
 | struct_init.c:15:12:15:12 | a | struct_init.c:27:7:27:16 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:27:7:27:16 | call to user_input | call to user_input |
 | struct_init.c:15:12:15:12 | a | struct_init.c:40:20:40:29 | call to user_input | struct_init.c:15:12:15:12 | a | a flows from $@ | struct_init.c:40:20:40:29 | call to user_input | call to user_input |

cpp/ql/test/library-tests/dataflow/fields/simple.cpp

@@ -94,4 +94,21 @@ void single_field_test_typedef(A_typedef a)
     sink(a2.i); //$ ast,ir
 }
 
+namespace TestAdditionalCallTargets {
+
+    using TakesIntReturnsVoid = void(*)(int);
+    template<TakesIntReturnsVoid F>
+    void call_template_argument(int);
+
+    void call_sink(int x) {
+        sink(x); // $ ir
+    }
+
+    void test_additional_call_targets() {
+        int x = user_input();
+        call_template_argument<call_sink>(x);
+    }
+
+}
+
 } // namespace Simple