Skip to content

SSA: Lift barrier guards to logical operations #17673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

SSA: Lift barrier guards to logical operations #17673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
37d28de
C#: Add more barrier guards tests
hvitved Oct 7, 2024
ee1a60d
Ruby: Add more barrier guards tests
hvitved Oct 7, 2024
841e6a1
SSA: Lift barrier guards to logical operations
hvitved Oct 7, 2024
4da7919
C#: Lift barrier guards to logical operations
hvitved Oct 7, 2024
bd17a9e
Ruby: Lift barrier guards to logical operations
hvitved Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions csharp/ql/lib/semmle/code/csharp/dataflow/internal/SsaImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,6 +1078,7 @@ class PhiReadNode extends DefinitionExt, Impl::PhiReadNode {
private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
private import csharp as Cs
private import semmle.code.csharp.controlflow.BasicBlocks
private import codeql.util.Boolean

class Expr extends ControlFlow::Node {
predicate hasCfgNode(ControlFlow::BasicBlock bb, int i) { this = bb.getNode(i) }
Expand Down Expand Up @@ -1114,6 +1115,58 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
}
}

abstract class LogicalOperationGuard extends Guard {
abstract Guard getOperand(int i);

abstract predicate lift(string id, int i, boolean operandBranch, boolean branch);
}

private class NotGuard extends LogicalOperationGuard, LogicalNotExpr {
override Guard getOperand(int i) { i = 0 and result = this.getOperand() }

override predicate lift(string id, int i, boolean operandBranch, boolean branch) {
operandBranch instanceof Boolean and
id = operandBranch.toString() and
i = 0 and
branch = operandBranch.booleanNot()
}
}

abstract private class BinaryLogicalOperationGuard extends LogicalOperationGuard,
BinaryLogicalOperation
{
final override Guard getOperand(int i) {
i = 0 and result = this.getLeftOperand()
or
i = 1 and result = this.getRightOperand()
}

abstract predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch);

final override predicate lift(string id, int i, boolean operandBranch, boolean branch) {
exists(Boolean branchLeft, Boolean branchRight |
this.lift(branchLeft, branchRight, branch) and
id = branchLeft + "," + branchRight
|
i = 0 and operandBranch = branchLeft
or
i = 1 and operandBranch = branchRight
)
}
}

private class AndGuard extends BinaryLogicalOperationGuard, LogicalAndExpr {
override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) {
branch = branchLeft.booleanOr(branchRight) // yes, should not be `booleanAnd`
}
}

private class OrGuard extends BinaryLogicalOperationGuard, LogicalOrExpr {
override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) {
branch = branchLeft.booleanAnd(branchRight) // yes, should not be `booleanOr`
}
}

/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
predicate guardControlsBlock(Guard guard, ControlFlow::BasicBlock bb, boolean branch) {
exists(ConditionBlock conditionBlock, ControlFlow::SuccessorTypes::ConditionalSuccessor s |
Expand Down
100 changes: 100 additions & 0 deletions csharp/ql/test/library-tests/dataflow/barrier-guards/BarrierFlow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,104 @@ void M6(bool b)

Sink(x);
}

void M7()
{
var x = Source(7);

if (x == "safe" && x == "safe")
{
Sink(x);
}
else
{
Sink(x); // $ hasValueFlow=7
}

if (x != "safe" && x == "safe2")
{
Sink(x);
}
else
{
Sink(x); // $ hasValueFlow=7
}

if (x == "safe" && x != "safe2")
{
Sink(x);
}
else
{
Sink(x); // $ hasValueFlow=7
}

if (x != "safe1" && x != "safe2")
{
Sink(x); // $ hasValueFlow=7
}
else
{
Sink(x);
}

if (!(x == "safe1") && x != "safe2")
{
Sink(x); // $ hasValueFlow=7
}
else
{
Sink(x);
}
}

void M8()
{
var x = Source(8);

if (x == "safe1" || x == "safe2")
{
Sink(x);
}
else
{
Sink(x); // $ hasValueFlow=8
}

if (x != "safe1" || x == "safe2")
{
Sink(x); // $ hasValueFlow=8
}
else
{
Sink(x);
}

if (x == "safe1" || x != "safe2")
{
Sink(x); // $ hasValueFlow=8
}
else
{
Sink(x);
}

if (x != "safe" || x != "safe")
{
Sink(x); // $ hasValueFlow=8
}
else
{
Sink(x);
}

if (!(x == "safe") || x != "safe")
{
Sink(x); // $ hasValueFlow=8
}
else
{
Sink(x);
}
}
}
36 changes: 36 additions & 0 deletions csharp/ql/test/library-tests/dataflow/barrier-guards/barrier-flow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,51 @@ edges
| BarrierFlow.cs:10:17:10:25 | call to method Source : Object | BarrierFlow.cs:10:13:10:13 | access to local variable x : Object | provenance | |
| BarrierFlow.cs:17:13:17:13 | access to local variable x : Object | BarrierFlow.cs:21:18:21:18 | access to local variable x | provenance | |
| BarrierFlow.cs:17:17:17:25 | call to method Source : Object | BarrierFlow.cs:17:13:17:13 | access to local variable x : Object | provenance | |
| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:94:18:94:18 | access to local variable x | provenance | |
| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:103:18:103:18 | access to local variable x | provenance | |
| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:112:18:112:18 | access to local variable x | provenance | |
| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:117:18:117:18 | access to local variable x | provenance | |
| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | BarrierFlow.cs:126:18:126:18 | access to local variable x | provenance | |
| BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | provenance | |
| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:144:18:144:18 | access to local variable x | provenance | |
| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:149:18:149:18 | access to local variable x | provenance | |
| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:158:18:158:18 | access to local variable x | provenance | |
| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:167:18:167:18 | access to local variable x | provenance | |
| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | BarrierFlow.cs:176:18:176:18 | access to local variable x | provenance | |
| BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | provenance | |
nodes
| BarrierFlow.cs:10:13:10:13 | access to local variable x : Object | semmle.label | access to local variable x : Object |
| BarrierFlow.cs:10:17:10:25 | call to method Source : Object | semmle.label | call to method Source : Object |
| BarrierFlow.cs:12:14:12:14 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:17:13:17:13 | access to local variable x : Object | semmle.label | access to local variable x : Object |
| BarrierFlow.cs:17:17:17:25 | call to method Source : Object | semmle.label | call to method Source : Object |
| BarrierFlow.cs:21:18:21:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:86:13:86:13 | access to local variable x : Object | semmle.label | access to local variable x : Object |
| BarrierFlow.cs:86:17:86:25 | call to method Source : Object | semmle.label | call to method Source : Object |
| BarrierFlow.cs:94:18:94:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:103:18:103:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:112:18:112:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:117:18:117:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:126:18:126:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:136:13:136:13 | access to local variable x : Object | semmle.label | access to local variable x : Object |
| BarrierFlow.cs:136:17:136:25 | call to method Source : Object | semmle.label | call to method Source : Object |
| BarrierFlow.cs:144:18:144:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:149:18:149:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:158:18:158:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:167:18:167:18 | access to local variable x | semmle.label | access to local variable x |
| BarrierFlow.cs:176:18:176:18 | access to local variable x | semmle.label | access to local variable x |
subpaths
testFailures
#select
| BarrierFlow.cs:12:14:12:14 | access to local variable x | BarrierFlow.cs:10:17:10:25 | call to method Source : Object | BarrierFlow.cs:12:14:12:14 | access to local variable x | $@ | BarrierFlow.cs:10:17:10:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:21:18:21:18 | access to local variable x | BarrierFlow.cs:17:17:17:25 | call to method Source : Object | BarrierFlow.cs:21:18:21:18 | access to local variable x | $@ | BarrierFlow.cs:17:17:17:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:94:18:94:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:94:18:94:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:103:18:103:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:103:18:103:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:112:18:112:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:112:18:112:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:117:18:117:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:117:18:117:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:126:18:126:18 | access to local variable x | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | BarrierFlow.cs:126:18:126:18 | access to local variable x | $@ | BarrierFlow.cs:86:17:86:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:144:18:144:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:144:18:144:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:149:18:149:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:149:18:149:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:158:18:158:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:158:18:158:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:167:18:167:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:167:18:167:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object |
| BarrierFlow.cs:176:18:176:18 | access to local variable x | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | BarrierFlow.cs:176:18:176:18 | access to local variable x | $@ | BarrierFlow.cs:136:17:136:25 | call to method Source : Object | call to method Source : Object |
24 changes: 0 additions & 24 deletions ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,30 +31,6 @@ private predicate stringConstCompare(CfgNodes::AstCfgNode guard, CfgNode testedN
)
or
stringConstCaseCompare(guard, testedNode, branch)
or
exists(CfgNodes::ExprNodes::BinaryOperationCfgNode g |
g = guard and
stringConstCompareOr(guard, branch) and
stringConstCompare(g.getLeftOperand(), testedNode, _)
)
}

/**
* Holds if `guard` is an `or` expression whose operands are string comparison guards.
* For example:
*
* ```rb
* x == "foo" or x == "bar"
* ```
*/
private predicate stringConstCompareOr(
CfgNodes::ExprNodes::BinaryOperationCfgNode guard, boolean branch
) {
guard.getExpr() instanceof LogicalOrExpr and
branch = true and
forall(CfgNode innerGuard | innerGuard = guard.getAnOperand() |
stringConstCompare(innerGuard, any(Ssa::Definition def).getARead(), branch)
)
}

/**
Expand Down
74 changes: 74 additions & 0 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,7 @@ class ParameterExt extends TParameterExt {

private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInputSig {
private import codeql.ruby.controlflow.internal.Guards as Guards
private import codeql.util.Boolean

class Parameter = ParameterExt;

Expand All @@ -560,6 +561,79 @@ private module DataFlowIntegrationInput implements Impl::DataFlowIntegrationInpu
predicate hasCfgNode(SsaInput::BasicBlock bb, int i) { this = bb.getNode(i) }
}

abstract class LogicalOperationGuard extends Guard {
abstract Guard getOperand(int i);

abstract predicate lift(string id, int i, boolean operandBranch, boolean branch);
}

private class NotGuard extends LogicalOperationGuard,
Cfg::CfgNodes::ExprNodes::UnaryOperationCfgNode
{
NotGuard() { this.getExpr() instanceof NotExpr }

override Guard getOperand(int i) { i = 0 and result = this.getOperand() }

override predicate lift(string id, int i, boolean operandBranch, boolean branch) {
operandBranch instanceof Boolean and
id = operandBranch.toString() and
i = 0 and
branch = operandBranch.booleanNot()
}
}

private class StmtSequenceGuard extends LogicalOperationGuard,
Cfg::CfgNodes::ExprNodes::StmtSequenceCfgNode
{
override Guard getOperand(int i) { i = 0 and result = this.getLastStmt() }

override predicate lift(string id, int i, boolean operandBranch, boolean branch) {
operandBranch instanceof Boolean and
id = operandBranch.toString() and
i = 0 and
branch = operandBranch
}
}

abstract private class BinaryLogicalOperationGuard extends LogicalOperationGuard,
Cfg::CfgNodes::ExprNodes::BinaryOperationCfgNode
{
final override Guard getOperand(int i) {
i = 0 and result = this.getLeftOperand()
or
i = 1 and result = this.getRightOperand()
}

abstract predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch);

final override predicate lift(string id, int i, boolean operandBranch, boolean branch) {
exists(Boolean branchLeft, Boolean branchRight |
this.lift(branchLeft, branchRight, branch) and
id = branchLeft + "," + branchRight
|
i = 0 and operandBranch = branchLeft
or
i = 1 and operandBranch = branchRight
)
}
}

private class AndGuard extends BinaryLogicalOperationGuard {
AndGuard() { this.getExpr() instanceof LogicalAndExpr }

override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) {
branch = branchLeft.booleanOr(branchRight) // yes, should not be `booleanAnd`
}
}

private class OrGuard extends BinaryLogicalOperationGuard {
OrGuard() { this.getExpr() instanceof LogicalOrExpr }

override predicate lift(Boolean branchLeft, Boolean branchRight, boolean branch) {
branch = branchLeft.booleanAnd(branchRight) // yes, should not be `booleanOr`
}
}

/** Holds if the guard `guard` controls block `bb` upon evaluating to `branch`. */
predicate guardControlsBlock(Guard guard, SsaInput::BasicBlock bb, boolean branch) {
Guards::guardControlsBlock(guard, bb, branch)
Expand Down
Loading
Loading