From 27a437a5141953f21e39095aa5c9d5899a0cacf6 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 15 Jan 2026 10:15:01 +0000 Subject: [PATCH 1/3] C++: Modify test to reveal a bug. --- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 30 +++++++++--------- .../modelgenerator/internal/CaptureModels.qll | 2 +- .../dataflow/ir-barrier-guards/test.cpp | 8 ++++- .../dataflow/ir-barrier-guards/test.ql | 31 ++++++++++++------- 4 files changed, 41 insertions(+), 30 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll index 1185b6a0c9c3..55325f055ce6 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll +++ b/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,7 +1163,7 @@ private module RawIndirectNodes { override predicate isGLValue() { this.getInstruction().isGLValue() } - override DataFlowType getType() { + override Type getType() { exists(int sub, DataFlowType 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. diff --git a/cpp/ql/src/utils/modelgenerator/internal/CaptureModels.qll b/cpp/ql/src/utils/modelgenerator/internal/CaptureModels.qll index 28892c5b8207..ba1221d112b7 100644 --- a/cpp/ql/src/utils/modelgenerator/internal/CaptureModels.qll +++ b/cpp/ql/src/utils/modelgenerator/internal/CaptureModels.qll @@ -78,7 +78,7 @@ module ModelGeneratorCommonInput implements ModelGeneratorCommonInputSig indirect_barrier=glval indirect_barrier=int } } \ No newline at end of file diff --git a/cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.ql b/cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.ql index 20610c55385c..5961da371228 100644 --- a/cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.ql +++ b/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; -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() ) From 28681508f3ac1977309a3608268fd6b483757f15 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 15 Jan 2026 11:13:43 +0000 Subject: [PATCH 2/3] C++: Fix bug and accept test changes. --- .../semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll | 11 +++++++++-- .../library-tests/dataflow/ir-barrier-guards/test.cpp | 4 ++-- shared/controlflow/codeql/controlflow/Guards.qll | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll index 8dc3513b444b..c4f5885cf6d4 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImpl.qll @@ -1064,8 +1064,15 @@ module BarrierGuardWithIntParam { DataFlowIntegrationInput::Guard g, SsaImpl::Definition def, IRGuards::GuardValue val, int indirectionIndex ) { - IRGuards::Guards_v1::ParameterizedValidationWrapper::guardChecksDef(g, - def, val, indirectionIndex) + exists(Instruction e | + IRGuards::Guards_v1::ParameterizedValidationWrapper::guardChecks(g, + e, val, indirectionIndex) + | + indirectionIndex = 0 and + def.(Definition).getAUse().getDef() = e + or + def.(Definition).getAnIndirectUse(indirectionIndex).getDef() = e + ) } Node getABarrierNode(int indirectionIndex) { diff --git a/cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.cpp b/cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.cpp index e9ea0790175d..9d9a89c70087 100644 --- a/cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.cpp +++ b/cpp/ql/test/library-tests/dataflow/ir-barrier-guards/test.cpp @@ -4,12 +4,12 @@ void sink(int); void testCheckArgument(int* p) { if (checkArgument(p)) { - sink(*p); // $ barrier=int indirect_barrier=int barrier=int* indirect_barrier=int* + sink(*p); // $ indirect_barrier=int barrier=int* } } void testCheckArgument(int p) { if (checkArgument(&p)) { - sink(p); // $ barrier=glval indirect_barrier=glval indirect_barrier=int + sink(p); // $ barrier=glval indirect_barrier=int } } \ No newline at end of file diff --git a/shared/controlflow/codeql/controlflow/Guards.qll b/shared/controlflow/codeql/controlflow/Guards.qll index b313afcdb6bb..f6774db1eb9f 100644 --- a/shared/controlflow/codeql/controlflow/Guards.qll +++ b/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 | From 6da7890ff528b3e6b8221f2fa0e86475e763e57a Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Thu, 15 Jan 2026 18:31:54 +0000 Subject: [PATCH 3/3] C++: Add indirect barrier guard to 'cpp/unbounded-write' to prevent FPs after fixing conflation. --- cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql b/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql index 4d33ede93150..8cb0122b668e 100644 --- a/cpp/ql/src/Security/CWE/CWE-120/UnboundedWrite.ql +++ b/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::getABarrierNode() + node = DataFlow::BarrierGuard::getABarrierNode() or + node = DataFlow::BarrierGuard::getAnIndirectBarrierNode() } predicate observeDiffInformedIncrementalMode() { any() }