From 3cce13fa5d2e47a1e16ed46a738c02c4ddb39d63 Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Wed, 22 Jan 2025 21:03:44 +0100 Subject: [PATCH] C++: Fix join order problem in `TaintedAllocationSize` Before this did not really terminate on `silentearth/curl2`. After the barrier looks like: ``` [2025-01-22 21:22:55] Evaluated non-recursive predicate TaintedAllocationSize::TaintedAllocationSizeConfig::isBarrier/1#6f365b45@37d8bfho in 5168ms (size: 240345). Evaluated relational algebra for predicate TaintedAllocationSize::TaintedAllocationSizeConfig::isBarrier/1#6f365b45@37d8bfho with tuple counts: 43 ~0% {1} r1 = JOIN Allocation::HeuristicAllocationFunction#class#57f08eba WITH `Function::Function.getAParameter/0#dispred#511fd682` ON FIRST 1 OUTPUT Rhs.1 43 ~0% {1} | JOIN WITH `DataFlowUtil::Node.asParameter/0#dispred#81f7eba7_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1 228072 ~0% {1} r2 = JOIN `TaintedAllocationSize::readsVariable/2#e074f316_10#join_rhs` WITH `TaintedAllocationSize::hasUpperBoundsCheck/1#9dd5da82` ON FIRST 1 OUTPUT Lhs.1 228209 ~0% {1} | JOIN WITH `Operand::Operand.getDef/0#dispred#a70e8079_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1 228125 ~0% {1} | JOIN WITH DataFlowUtil::OperandNode#3e3b23f6_20#join_rhs ON FIRST 1 OUTPUT Rhs.1 1 ~0% {1} r3 = CONSTANT(unique int)[38] 103 ~0% {1} | JOIN WITH exprs_10#join_rhs ON FIRST 1 OUTPUT Rhs.1 206 ~0% {1} | JOIN WITH `Expr::Operation.getAnOperand/0#dispred#4dc2ee04#bf` ON FIRST 1 OUTPUT Rhs.1 8944 ~2% {1} r4 = `Bounded::bounded/1#e7aa9c09` UNION r3 4307 ~0% {1} | JOIN WITH `project#ExprNodes::ExprNode.getExpr/1#dispred#81a030dd_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1 70451786 ~8% {2} r5 = JOIN `TaintedAllocationSize::readsVariable/2#e074f316_10#join_rhs` WITH `TaintedAllocationSize::readsVariable/2#e074f316_10#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.1 70451768 ~1% {3} | JOIN WITH `Instruction::Instruction.getBlock/0#dispred#58a40e80` ON FIRST 1 OUTPUT Lhs.1, Lhs.0, Rhs.1 75573899 ~2% {3} | JOIN WITH `Operand::Operand.getDef/0#dispred#a70e8079_10#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Lhs.2, Rhs.1 90437235 ~0% {3} | JOIN WITH `Operand::Operand.getDef/0#dispred#a70e8079_10#join_rhs` ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.2 75675394 ~2% {3} | JOIN WITH DataFlowUtil::OperandNode#3e3b23f6_20#join_rhs ON FIRST 1 OUTPUT Lhs.2, Lhs.1, Rhs.1 5218044 ~0% {4} | JOIN WITH `project#IRGuards::Cached::compares_eq/6#511a0d6d_102#join_rhs` ON FIRST 1 OUTPUT Lhs.1, Rhs.2, Lhs.2, Rhs.1 51350331 ~2% {3} | JOIN WITH `IRGuards::IRGuardCondition.valueControls/2#eb6b9b19_120#join_rhs` ON FIRST 2 OUTPUT Rhs.2, Lhs.3, Lhs.2 25351 ~227% {1} | JOIN WITH `ValueNumberingInternal::tvalueNumber/1#f03b58f9` ON FIRST 2 OUTPUT Lhs.2 257826 ~6% {1} r6 = r1 UNION r2 UNION r4 UNION r5 return r6 ``` --- .../lib/semmle/code/cpp/controlflow/IRGuards.qll | 8 +++++++- .../CWE/CWE-190/TaintedAllocationSize.ql | 16 ++++++++-------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll index 76a1299cb536..7d05306f6d6a 100644 --- a/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll +++ b/cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll @@ -685,6 +685,12 @@ class IRGuardCondition extends Instruction { unary_compares_eq(valueNumber(this), op, k, areEqual, value) } + bindingset[value] + pragma[inline_late] + private predicate ensuresEqvalueControls(IRBlock block, AbstractValue value) { + this.valueControls(block, value) + } + /** * Holds if (determined by this guard) `left == right + k` must be `areEqual` in `block`. * If `areEqual = false` then this implies `left != right + k`. @@ -693,7 +699,7 @@ class IRGuardCondition extends Instruction { predicate ensuresEq(Operand left, Operand right, int k, IRBlock block, boolean areEqual) { exists(AbstractValue value | compares_eq(valueNumber(this), left, right, k, areEqual, value) and - this.valueControls(block, value) + this.ensuresEqvalueControls(block, value) ) } diff --git a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql index 93494987360d..6ccea7cd8966 100644 --- a/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql +++ b/cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql @@ -46,10 +46,13 @@ predicate hasUpperBoundsCheck(Variable var) { ) } -predicate nodeIsBarrierEqualityCandidate(DataFlow::Node node, Operand access, Variable checkedVar) { - exists(Instruction instr | instr = node.asOperand().getDef() | - readsVariable(instr, checkedVar) and - any(IRGuardCondition guard).ensuresEq(access, _, _, instr.getBlock(), true) +predicate nodeHasBarrierEquality(DataFlow::Node node) { + exists(Variable checkedVar, Operand access | + readsVariable(access.getDef(), checkedVar) and + exists(Instruction instr | instr = node.asOperand().getDef() | + readsVariable(pragma[only_bind_into](instr), pragma[only_bind_into](checkedVar)) and + any(IRGuardCondition guard).ensuresEq(access, _, _, instr.getBlock(), true) + ) ) } @@ -76,10 +79,7 @@ module TaintedAllocationSizeConfig implements DataFlow::ConfigSig { hasUpperBoundsCheck(checkedVar) ) or - exists(Variable checkedVar, Operand access | - readsVariable(access.getDef(), checkedVar) and - nodeIsBarrierEqualityCandidate(node, access, checkedVar) - ) + nodeHasBarrierEquality(node) or // block flow to inside of identified allocation functions (this flow leads // to duplicate results)