Revert "Cherry picking commit bbf9bcde2a (#21)"

d4e5b27969 This reverts commit d4e5b27969.
2026-06-03 12:50:16 +02:00 · 2023-10-18 10:17:53 -04:00
parent ab827a5acd
commit f19919bb52
6 changed files with 36 additions and 125 deletions
--- a/cpp/ql/lib/change-notes/2023-10-12-additional-call-targets.md
+++ b/cpp/ql/lib/change-notes/2023-10-12-additional-call-targets.md
@@ -1,4 +0,0 @@
---
-category: feature
---
-* Added a new class `AdditionalCallTarget` for specifying additional call targets.
--- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowDispatch.qll
@@ -7,12 +7,9 @@ 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 defaultViableCallable(DataFlowCall call) {
+DataFlowCallable viableCallable(DataFlowCall call) {
  DataFlowImplCommon::forceCachingInSameStage() and
  result = call.getStaticCallTarget()
  or
@@ -32,17 +29,6 @@ DataFlowCallable defaultViableCallable(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`.
--- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
@@ -14,7 +14,6 @@ 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:
@@ -1697,7 +1696,16 @@ 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[]`.
-    reverseFlow(nodeFrom, nodeTo)
+    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)
+    )
  }

  private predicate simpleInstructionLocalFlowStep(Operand opFrom, Instruction iTo) {
@@ -1728,39 +1736,6 @@ 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
@@ -2240,41 +2215,33 @@ module InstructionBarrierGuard<instructionGuardChecksSig/3 instructionGuardCheck
 }

 /**
- * A unit class for adding additional call steps.
+ * DEPRECATED: Use `BarrierGuard` module instead.
 *
- * Extend this class to add additional call steps to the data flow graph.
+ * A guard that validates some instruction.
 *
- * 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);
+ * 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.
 *
- * 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.
+ * It is important that all extending classes in scope are disjoint.
 */
-class AdditionalCallTarget extends Unit {
-  /**
-   * Gets a viable target for `call`.
-   */
-  abstract DataFlowCallable viableTarget(Call call);
+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)
+    )
+  }
 }
--- a/cpp/ql/test/library-tests/dataflow/fields/IRConfiguration.qll
+++ b/cpp/ql/test/library-tests/dataflow/fields/IRConfiguration.qll
@@ -1,19 +1,6 @@
 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
--- a/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected
+++ b/cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected
@@ -770,9 +770,6 @@ 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] |
@@ -1579,10 +1576,6 @@ 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 |
@@ -1789,7 +1782,6 @@ 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 |
--- a/cpp/ql/test/library-tests/dataflow/fields/simple.cpp
+++ b/cpp/ql/test/library-tests/dataflow/fields/simple.cpp
@@ -94,21 +94,4 @@ 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