From eea7804b62a0228af847faed5ce8a237d9e623d4 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 17 Dec 2024 15:25:16 +0000 Subject: [PATCH 1/5] C++: Join with value number only after joining with 'controls'. --- .../code/cpp/ir/dataflow/internal/DataFlowUtil.qll | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 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 32dec1355ea9..1705a6507016 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 @@ -2275,7 +2275,7 @@ private predicate guardControlsPhiInput( */ signature predicate guardChecksSig(IRGuardCondition g, Expr e, boolean branch); -bindingset[g, n] +bindingset[g] pragma[inline_late] private predicate controls(IRGuardCondition g, Node n, boolean edge) { g.controls(n.getBasicBlock(), edge) @@ -2288,6 +2288,13 @@ private predicate controls(IRGuardCondition g, Node n, boolean edge) { * in data flow and taint tracking. */ module BarrierGuard { + bindingset[value, n] + pragma[inline_late] + private predicate convertedExprHasValueNumber(Expr e, ValueNumber value, Node n) { + e = value.getAnInstruction().getConvertedResultExpression() and + n.asConvertedExpr() = e + } + /** * Gets an expression node that is safely guarded by the given guard check. * @@ -2322,8 +2329,7 @@ module BarrierGuard { */ Node getABarrierNode() { exists(IRGuardCondition g, Expr e, ValueNumber value, boolean edge | - e = value.getAnInstruction().getConvertedResultExpression() and - result.asConvertedExpr() = e and + convertedExprHasValueNumber(e, value, result) and guardChecks(g, pragma[only_bind_into](value.getAnInstruction().getConvertedResultExpression()), edge) and controls(g, result, edge) From 9b6f39c1fe0dd543d3c45a71afde26b926ac632c Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 17 Dec 2024 15:26:49 +0000 Subject: [PATCH 2/5] C++: Apply similar join order fixes to the other cases. --- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 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 1705a6507016..bdfa735de76a 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 @@ -2380,6 +2380,15 @@ module BarrierGuard { */ Node getAnIndirectBarrierNode() { result = getAnIndirectBarrierNode(_) } + bindingset[value, n] + pragma[inline_late] + private predicate indirectConvertedExprHasValueNumber( + Expr e, int indirectionIndex, ValueNumber value, Node n + ) { + e = value.getAnInstruction().getConvertedResultExpression() and + n.asIndirectConvertedExpr(indirectionIndex) = e + } + /** * Gets an indirect expression node with indirection index `indirectionIndex` that is * safely guarded by the given guard check. @@ -2416,8 +2425,7 @@ module BarrierGuard { */ Node getAnIndirectBarrierNode(int indirectionIndex) { exists(IRGuardCondition g, Expr e, ValueNumber value, boolean edge | - e = value.getAnInstruction().getConvertedResultExpression() and - result.asIndirectConvertedExpr(indirectionIndex) = e and + indirectConvertedExprHasValueNumber(e, indirectionIndex, value, result) and guardChecks(g, pragma[only_bind_into](value.getAnInstruction().getConvertedResultExpression()), edge) and controls(g, result, edge) @@ -2456,12 +2464,18 @@ private EdgeKind getConditionalEdge(boolean branch) { * in data flow and taint tracking. */ module InstructionBarrierGuard { + bindingset[value, n] + pragma[inline_late] + private predicate operandHasValueNumber(Operand use, ValueNumber value, Node n) { + use = value.getAnInstruction().getAUse() and + n.asOperand() = use + } + /** Gets a node that is safely guarded by the given guard check. */ Node getABarrierNode() { exists(IRGuardCondition g, ValueNumber value, boolean edge, Operand use | instructionGuardChecks(g, pragma[only_bind_into](value.getAnInstruction()), edge) and - use = value.getAnInstruction().getAUse() and - result.asOperand() = use and + operandHasValueNumber(use, value, result) and controls(g, result, edge) ) or From f35155854706ce6c1886ed4975e7622e048068b9 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 17 Dec 2024 15:27:54 +0000 Subject: [PATCH 3/5] C++: While here, let's avoid materializing 'ensuresEq' and 'ensuresLt' when computing unreachable nodes in dataflow. --- .../code/cpp/ir/dataflow/internal/DataFlowPrivate.qll | 6 ++++-- .../ir/implementation/aliased_ssa/gvn/ValueNumbering.qll | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index 8f0ae53171e3..7b89e9714ff0 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -1237,12 +1237,14 @@ module IsUnreachableInCall { int getValue() { result = value } } - pragma[nomagic] + bindingset[right] + pragma[inline_late] private predicate ensuresEq(Operand left, Operand right, int k, IRBlock block, boolean areEqual) { any(G::IRGuardCondition guard).ensuresEq(left, right, k, block, areEqual) } - pragma[nomagic] + bindingset[right] + pragma[inline_late] private predicate ensuresLt(Operand left, Operand right, int k, IRBlock block, boolean areEqual) { any(G::IRGuardCondition guard).ensuresLt(left, right, k, block, areEqual) } diff --git a/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll b/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll index 2a46e16c52fe..279b43a1ca8b 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll @@ -51,6 +51,7 @@ class ValueNumber extends TValueNumber { /** * Gets an `Operand` whose definition is exact and has this value number. */ + pragma[nomagic] final Operand getAUse() { this = valueNumber(result.getDef()) } final string getKind() { From 5ed0222b1a362ce059918b5e26356ee84bde5c12 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 17 Dec 2024 15:28:04 +0000 Subject: [PATCH 4/5] C++: Sync identical files. --- .../semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll | 1 + .../cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll | 1 + 2 files changed, 2 insertions(+) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll b/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll index 2a46e16c52fe..279b43a1ca8b 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll @@ -51,6 +51,7 @@ class ValueNumber extends TValueNumber { /** * Gets an `Operand` whose definition is exact and has this value number. */ + pragma[nomagic] final Operand getAUse() { this = valueNumber(result.getDef()) } final string getKind() { diff --git a/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll b/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll index 2a46e16c52fe..279b43a1ca8b 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll @@ -51,6 +51,7 @@ class ValueNumber extends TValueNumber { /** * Gets an `Operand` whose definition is exact and has this value number. */ + pragma[nomagic] final Operand getAUse() { this = valueNumber(result.getDef()) } final string getKind() { From 2cc6ffbd28f8488ce6dd1adcb326643230ecc0ed Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 17 Dec 2024 16:55:52 +0000 Subject: [PATCH 5/5] C++: Fix ql-for-ql findings. --- .../cpp/ir/dataflow/internal/DataFlowUtil.qll | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 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 bdfa735de76a..5a5607dbf3bf 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 @@ -2290,9 +2290,11 @@ private predicate controls(IRGuardCondition g, Node n, boolean edge) { module BarrierGuard { bindingset[value, n] pragma[inline_late] - private predicate convertedExprHasValueNumber(Expr e, ValueNumber value, Node n) { - e = value.getAnInstruction().getConvertedResultExpression() and - n.asConvertedExpr() = e + private predicate convertedExprHasValueNumber(ValueNumber value, Node n) { + exists(Expr e | + e = value.getAnInstruction().getConvertedResultExpression() and + n.asConvertedExpr() = e + ) } /** @@ -2328,8 +2330,8 @@ module BarrierGuard { * NOTE: If an indirect expression is tracked, use `getAnIndirectBarrierNode` instead. */ Node getABarrierNode() { - exists(IRGuardCondition g, Expr e, ValueNumber value, boolean edge | - convertedExprHasValueNumber(e, value, result) and + exists(IRGuardCondition g, ValueNumber value, boolean edge | + convertedExprHasValueNumber(value, result) and guardChecks(g, pragma[only_bind_into](value.getAnInstruction().getConvertedResultExpression()), edge) and controls(g, result, edge) @@ -2383,10 +2385,12 @@ module BarrierGuard { bindingset[value, n] pragma[inline_late] private predicate indirectConvertedExprHasValueNumber( - Expr e, int indirectionIndex, ValueNumber value, Node n + int indirectionIndex, ValueNumber value, Node n ) { - e = value.getAnInstruction().getConvertedResultExpression() and - n.asIndirectConvertedExpr(indirectionIndex) = e + exists(Expr e | + e = value.getAnInstruction().getConvertedResultExpression() and + n.asIndirectConvertedExpr(indirectionIndex) = e + ) } /** @@ -2424,8 +2428,8 @@ module BarrierGuard { * NOTE: If a non-indirect expression is tracked, use `getABarrierNode` instead. */ Node getAnIndirectBarrierNode(int indirectionIndex) { - exists(IRGuardCondition g, Expr e, ValueNumber value, boolean edge | - indirectConvertedExprHasValueNumber(e, indirectionIndex, value, result) and + exists(IRGuardCondition g, ValueNumber value, boolean edge | + indirectConvertedExprHasValueNumber(indirectionIndex, value, result) and guardChecks(g, pragma[only_bind_into](value.getAnInstruction().getConvertedResultExpression()), edge) and controls(g, result, edge) @@ -2466,16 +2470,18 @@ private EdgeKind getConditionalEdge(boolean branch) { module InstructionBarrierGuard { bindingset[value, n] pragma[inline_late] - private predicate operandHasValueNumber(Operand use, ValueNumber value, Node n) { - use = value.getAnInstruction().getAUse() and - n.asOperand() = use + private predicate operandHasValueNumber(ValueNumber value, Node n) { + exists(Operand use | + use = value.getAnInstruction().getAUse() and + n.asOperand() = use + ) } /** Gets a node that is safely guarded by the given guard check. */ Node getABarrierNode() { - exists(IRGuardCondition g, ValueNumber value, boolean edge, Operand use | + exists(IRGuardCondition g, ValueNumber value, boolean edge | instructionGuardChecks(g, pragma[only_bind_into](value.getAnInstruction()), edge) and - operandHasValueNumber(use, value, result) and + operandHasValueNumber(value, result) and controls(g, result, edge) ) or