Merge pull request #21171 from MathiasVP/fix-conflation-in-guards

C++: Fix conflation in barrier guards

Author: MathiasVP

cpp/ql/lib/change-notes/2026-01-16-barrier-guard-conflation.md

@@ -0,0 +1,4 @@
+---
+category: fix
+---
+* Fixed a bug in the `DataFlow::BarrierGuard<...>::getABarrierNode` predicate which caused the predicate to return `DataFlow::Node`s with incorrect indirections. If you use `getABarrierNode` to implement barriers in a dataflow/taint-tracking query it may result in more query results. You can use `DataFlow::BarrierGuard<...>::getAnIndirectBarrierNode` to remove those query results.

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

@@ -156,7 +156,7 @@ class Node extends TIRDataFlowNode {
    * If `isGLValue()` holds, then the type of this node
    * should be thought of as "pointer to `getType()`".
    */
-  DataFlowType getType() { none() } // overridden in subclasses
+  Type getType() { none() } // overridden in subclasses
 
   /** Gets the instruction corresponding to this node, if any. */
   Instruction asInstruction() { result = this.(InstructionNode).getInstruction() }
@@ -541,7 +541,7 @@ class Node extends TIRDataFlowNode {
   /**
    * Gets an upper bound on the type of this node.
    */
-  DataFlowType getTypeBound() { result = this.getType() }
+  Type getTypeBound() { result = this.getType() }
 
   /** Gets the location of this element. */
   cached
@@ -585,7 +585,7 @@ private class Node0 extends Node, TNode0 {
 
   override string toStringImpl() { result = node.toString() }
 
-  override DataFlowType getType() { result = node.getType() }
+  override Type getType() { result = node.getType() }
 
   override predicate isGLValue() { node.isGLValue() }
 }
@@ -704,7 +704,7 @@ class SsaSynthNode extends Node, TSsaSynthNode {
 
   override Declaration getFunction() { result = node.getBasicBlock().getEnclosingFunction() }
 
-  override DataFlowType getType() { result = node.getSourceVariable().getType() }
+  override Type getType() { result = node.getSourceVariable().getType() }
 
   override predicate isGLValue() { node.getSourceVariable().isGLValue() }
 
@@ -732,7 +732,7 @@ class SsaIteratorNode extends Node, TSsaIteratorNode {
 
   override Declaration getFunction() { result = node.getFunction() }
 
-  override DataFlowType getType() { result = node.getType() }
+  override Type getType() { result = node.getType() }
 
   final override Location getLocationImpl() { result = node.getLocation() }
 
@@ -792,7 +792,7 @@ class FinalGlobalValue extends Node, TFinalGlobalValue {
 
   override Declaration getFunction() { result = globalUse.getIRFunction().getFunction() }
 
-  override DataFlowType getType() {
+  override Type getType() {
     exists(int indirectionIndex |
       indirectionIndex = globalUse.getIndirectionIndex() and
       result = getTypeImpl(globalUse.getUnderlyingType(), indirectionIndex)
@@ -826,7 +826,7 @@ class InitialGlobalValue extends Node, TInitialGlobalValue {
 
   final override predicate isGLValue() { globalDef.getIndirectionIndex() = 0 }
 
-  override DataFlowType getType() { result = globalDef.getUnderlyingType() }
+  override Type getType() { result = globalDef.getUnderlyingType() }
 
   final override Location getLocationImpl() { result = globalDef.getLocation() }
 
@@ -853,7 +853,7 @@ class BodyLessParameterNodeImpl extends Node, TBodyLessParameterNodeImpl {
   /** Gets the indirection index of this node. */
   int getIndirectionIndex() { result = indirectionIndex }
 
-  override DataFlowType getType() {
+  override Type getType() {
     result = getTypeImpl(p.getUnderlyingType(), this.getIndirectionIndex())
   }
 
@@ -1117,8 +1117,8 @@ private module RawIndirectNodes {
 
     override predicate isGLValue() { this.getOperand().isGLValue() }
 
-    override DataFlowType getType() {
-      exists(int sub, DataFlowType type, boolean isGLValue |
+    override Type getType() {
+      exists(int sub, Type type, boolean isGLValue |
         type = getOperandType(this.getOperand(), isGLValue) and
         if isGLValue = true then sub = 1 else sub = 0
       |
@@ -1163,8 +1163,8 @@ private module RawIndirectNodes {
 
     override predicate isGLValue() { this.getInstruction().isGLValue() }
 
-    override DataFlowType getType() {
-      exists(int sub, DataFlowType type, boolean isGLValue |
+    override Type getType() {
+      exists(int sub, Type type, boolean isGLValue |
         type = getInstructionType(this.getInstruction(), isGLValue) and
         if isGLValue = true then sub = 1 else sub = 0
       |
@@ -1263,7 +1263,7 @@ class FinalParameterNode extends Node, TFinalParameterNode {
     result.asSourceCallable() = this.getFunction()
   }
 
-  override DataFlowType getType() { result = getTypeImpl(p.getUnderlyingType(), indirectionIndex) }
+  override Type getType() { result = getTypeImpl(p.getUnderlyingType(), indirectionIndex) }
 
   final override Location getLocationImpl() {
     // Parameters can have multiple locations. When there's a unique location we use
@@ -1539,7 +1539,7 @@ abstract class PostUpdateNode extends Node {
    */
   abstract Node getPreUpdateNode();
 
-  final override DataFlowType getType() { result = this.getPreUpdateNode().getType() }
+  final override Type getType() { result = this.getPreUpdateNode().getType() }
 }
 
 /**
@@ -1632,9 +1632,7 @@ class VariableNode extends Node, TGlobalLikeVariableNode {
     result.asSourceCallable() = v
   }
 
-  override DataFlowType getType() {
-    result = getTypeImpl(v.getUnderlyingType(), indirectionIndex - 1)
-  }
+  override Type getType() { result = getTypeImpl(v.getUnderlyingType(), indirectionIndex - 1) }
 
   final override Location getLocationImpl() {
     // Certain variables (such as parameters) can have multiple locations.

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

@@ -53,7 +53,7 @@ private module SourceVariables {
      * the type of this source variable should be thought of as "pointer
      * to `getType()`".
      */
-    DataFlowType getType() {
+    Type getType() {
       if this.isGLValue()
       then result = base.getType()
       else result = getTypeImpl(base.getType(), ind - 1)
@@ -1064,8 +1064,15 @@ module BarrierGuardWithIntParam<guardChecksNodeSig/4 guardChecksNode> {
     DataFlowIntegrationInput::Guard g, SsaImpl::Definition def, IRGuards::GuardValue val,
     int indirectionIndex
   ) {
-    IRGuards::Guards_v1::ParameterizedValidationWrapper<int, guardChecksInstr/4>::guardChecksDef(g,
-      def, val, indirectionIndex)
+    exists(Instruction e |
+      IRGuards::Guards_v1::ParameterizedValidationWrapper<int, guardChecksInstr/4>::guardChecks(g,
+        e, val, indirectionIndex)
+    |
+      indirectionIndex = 0 and
+      def.(Definition).getAUse().getDef() = e
+      or
+      def.(Definition).getAnIndirectUse(indirectionIndex).getDef() = e
+    )
   }
 
   Node getABarrierNode(int indirectionIndex) {

cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql

@@ -122,7 +122,8 @@ module Config implements DataFlow::ConfigSig {
 
   predicate isBarrier(DataFlow::Node node) {
     // Block flow if the node is guarded by any <, <= or = operations.
-    node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getABarrierNode()
+    node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getABarrierNode() or
+    node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getAnIndirectBarrierNode()
   }
 
   predicate observeDiffInformedIncrementalMode() { any() }

cpp/ql/src/utils/modelgenerator/internal/CaptureModels.qll

@@ -78,7 +78,7 @@ module ModelGeneratorCommonInput implements ModelGeneratorCommonInputSig<Cpp::Lo
 {
   private module DataFlow = Df::DataFlow;
 
-  class Type = DataFlowPrivate::DataFlowType;
+  class Type = Cpp::Type;
 
   // Note: This also includes `this`
   class Parameter = DataFlow::ParameterNode;

cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.cpp

@@ -4,6 +4,12 @@ void sink(int);
 
 void testCheckArgument(int* p) {
   if (checkArgument(p)) {
-    sink(*p); // $ barrier barrier=1
+    sink(*p); // $ indirect_barrier=int barrier=int*
+  }
+}
+
+void testCheckArgument(int p) {
+  if (checkArgument(&p)) {
+    sink(p); // $ barrier=glval<int> indirect_barrier=int
   }
 }

cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.ql

@@ -13,26 +13,33 @@ predicate instructionGuardChecks(IRGuardCondition gc, Instruction checked, boole
 
 module BarrierGuard = DataFlow::InstructionBarrierGuard<instructionGuardChecks/3>;
 
-predicate indirectBarrierGuard(DataFlow::Node node, int indirectionIndex) {
-  node = BarrierGuard::getAnIndirectBarrierNode(indirectionIndex)
+predicate indirectBarrierGuard(DataFlow::Node node, string s) {
+  node = BarrierGuard::getAnIndirectBarrierNode(_) and
+  if node.isGLValue()
+  then s = "glval<" + node.getType().toString().replaceAll(" ", "") + ">"
+  else s = node.getType().toString().replaceAll(" ", "")
 }
 
-predicate barrierGuard(DataFlow::Node node) { node = BarrierGuard::getABarrierNode() }
+predicate barrierGuard(DataFlow::Node node, string s) {
+  node = BarrierGuard::getABarrierNode() and
+  if node.isGLValue()
+  then s = "glval<" + node.getType().toString().replaceAll(" ", "") + ">"
+  else s = node.getType().toString().replaceAll(" ", "")
+}
 
 module Test implements TestSig {
-  string getARelevantTag() { result = "barrier" }
+  string getARelevantTag() { result = ["barrier", "indirect_barrier"] }
 
   predicate hasActualResult(Location location, string element, string tag, string value) {
-    exists(DataFlow::Node node |
-      barrierGuard(node) and
-      value = ""
+    exists(DataFlow::Node node, string s |
+      indirectBarrierGuard(node, s) and
+      value = s and
+      tag = "indirect_barrier"
       or
-      exists(int indirectionIndex |
-        indirectBarrierGuard(node, indirectionIndex) and
-        value = indirectionIndex.toString()
-      )
+      barrierGuard(node, s) and
+      value = s and
+      tag = "barrier"
     |
-      tag = "barrier" and
       element = node.toString() and
       location = node.getLocation()
     )

shared/controlflow/codeql/controlflow/Guards.qll

@@ -1364,7 +1364,7 @@ module Make<
       /**
        * Holds if the guard `g` validates the expression `e` upon evaluating to `val`.
        */
-      private predicate guardChecks(Guard g, Expr e, GuardValue val, P par) {
+      predicate guardChecks(Guard g, Expr e, GuardValue val, P par) {
         guardChecks0(g, e, val, par)
         or
         exists(NonOverridableMethodCall call, ParameterPosition ppos, ArgumentPosition apos |