Skip to content
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

Jump to bottom

Brodes/guard flow parsing #17907

Closed
wants to merge 10 commits into from
+404 −78
Closed

Brodes/guard flow parsing #17907

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
aaf3ce2
Adding test cases for boolean flow.
bdrodes Nov 4, 2024
40a6786
Adding expected false positive conditions for MissingCheckScanf once …
bdrodes Nov 4, 2024
bb989ba
Addingg expected file for test changes for MissingCheckScanf before c…
bdrodes Nov 4, 2024
ddfbb08
IRGuard overhaul to parse conditions using GVN.
bdrodes Nov 4, 2024
0795bcc
Updating expected files for new IRGuard changes.
bdrodes Nov 4, 2024
d17dee5
Updating IRGuards.qll getDerivedInstruction. Always get the deriving …
bdrodes Nov 4, 2024
6f17460
Revert "Updating IRGuards.qll getDerivedInstruction. Always get the d…
bdrodes Nov 4, 2024
41e7dae
getDerivedInstruction uses getSourceValue()
bdrodes Nov 7, 2024
418b113
Adding conversions to the getDerivedInstruction predicate. Changed al…
bdrodes Nov 11, 2024
2dfa029
Misc. poc updates to address performance issues with wireshark.
bdrodes Nov 13, 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
59 changes: 40 additions & 19 deletions cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,21 @@

import cpp
import semmle.code.cpp.ir.IR
private import semmle.code.cpp.ir.ValueNumbering
private import semmle.code.cpp.ir.implementation.raw.internal.TranslatedExpr
private import semmle.code.cpp.ir.implementation.raw.internal.InstructionTag

/**
* Returns `instr` or any instruction used to define `instr`.
*/
private Instruction getDerivedInstruction(Instruction instr) {
result = valueNumber(instr).getAnInstruction() and
result.toString() != instr.toString() and
result instanceof CompareInstruction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to use toString when implementing query/library logic. toString output is being changed consistently to provide better UX in query output without any deprecation periods, and there shouldn't be any hidden invariants in the output of these predicates. So we need to come up with something else here.

What's the job of this predicate really? I think you're trying to find an equivalent instruction that performs a comparison so that we can derive guards from that comparison instead of the original instruction, right? But why should we restrict that to be CompareInstructions only? And why check that they have a different toString? If it's because we want to not have result = instr in this case could we not simply write result != instr?

And related to the QLDoc: What do you mean by "define" instr? I think we should have an example in the QLDoc to motivate this predicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take the discussion offline. But some quick comments:

  • Agreed on tostring, but I don't know a better approach, we will discuss
  • I can't say result != instr because that's not what I'm trying to achieve here. I'm trying to not get other GVN instructions that are identical. e.g., a->foo->bar I don't need to get every a->foo->bar, I need to get the instruction that defines a->foo->bar and ignore everything else. I'm playing a game with GVN to give me the SSA def. We probably need to use SSA instead, but this was a quick demonstration of how to get what I wanted (it at least works with the AST version of this in my tests in the past)
  • I only limited CompareInstruction because I thought I saw you were doing the same, but I've been testing with and without it.
  • The TLDR for me is either we use SSA, or keep this kind of approach but use the IR to help me find only the GVN with the definition. I'm no expert on the syntax of all the different IR instruction types, but perhaps you can provide some guidance.

Taking further discussion into a DM until we settle on a few things.

or
result = instr
}

/**
* Holds if `block` consists of an `UnreachedInstruction`.
*
Expand Down Expand Up @@ -517,7 +529,7 @@ class IRGuardCondition extends Instruction {
cached
predicate comparesLt(Operand left, Operand right, int k, boolean isLessThan, boolean testIsTrue) {
exists(BooleanValue value |
compares_lt(this, left, right, k, isLessThan, value) and
compares_lt(getDerivedInstruction(this), left, right, k, isLessThan, value) and
value.getValue() = testIsTrue
)
}
Expand All @@ -528,7 +540,7 @@ class IRGuardCondition extends Instruction {
*/
cached
predicate comparesLt(Operand op, int k, boolean isLessThan, AbstractValue value) {
compares_lt(this, op, k, isLessThan, value)
compares_lt(getDerivedInstruction(this), op, k, isLessThan, value)
}

/**
Expand All @@ -538,7 +550,8 @@ class IRGuardCondition extends Instruction {
cached
predicate ensuresLt(Operand left, Operand right, int k, IRBlock block, boolean isLessThan) {
exists(AbstractValue value |
compares_lt(this, left, right, k, isLessThan, value) and this.valueControls(block, value)
compares_lt(getDerivedInstruction(this), left, right, k, isLessThan, value) and
this.valueControls(block, value)
)
}

Expand All @@ -549,7 +562,8 @@ class IRGuardCondition extends Instruction {
cached
predicate ensuresLt(Operand op, int k, IRBlock block, boolean isLessThan) {
exists(AbstractValue value |
compares_lt(this, op, k, isLessThan, value) and this.valueControls(block, value)
compares_lt(getDerivedInstruction(this), op, k, isLessThan, value) and
this.valueControls(block, value)
)
}

Expand All @@ -562,7 +576,7 @@ class IRGuardCondition extends Instruction {
Operand left, Operand right, int k, IRBlock pred, IRBlock succ, boolean isLessThan
) {
exists(AbstractValue value |
compares_lt(this, left, right, k, isLessThan, value) and
compares_lt(getDerivedInstruction(this), left, right, k, isLessThan, value) and
this.valueControlsEdge(pred, succ, value)
)
}
Expand All @@ -574,7 +588,7 @@ class IRGuardCondition extends Instruction {
cached
predicate ensuresLtEdge(Operand left, int k, IRBlock pred, IRBlock succ, boolean isLessThan) {
exists(AbstractValue value |
compares_lt(this, left, k, isLessThan, value) and
compares_lt(getDerivedInstruction(this), left, k, isLessThan, value) and
this.valueControlsEdge(pred, succ, value)
)
}
Expand All @@ -583,15 +597,15 @@ class IRGuardCondition extends Instruction {
cached
predicate comparesEq(Operand left, Operand right, int k, boolean areEqual, boolean testIsTrue) {
exists(BooleanValue value |
compares_eq(this, left, right, k, areEqual, value) and
compares_eq(getDerivedInstruction(this), left, right, k, areEqual, value) and
value.getValue() = testIsTrue
)
}

/** Holds if (determined by this guard) `op == k` evaluates to `areEqual` if this expression evaluates to `value`. */
cached
predicate comparesEq(Operand op, int k, boolean areEqual, AbstractValue value) {
unary_compares_eq(this, op, k, areEqual, false, value)
unary_compares_eq(getDerivedInstruction(this), op, k, areEqual, false, value)
}

/**
Expand All @@ -601,7 +615,8 @@ class IRGuardCondition extends Instruction {
cached
predicate ensuresEq(Operand left, Operand right, int k, IRBlock block, boolean areEqual) {
exists(AbstractValue value |
compares_eq(this, left, right, k, areEqual, value) and this.valueControls(block, value)
compares_eq(getDerivedInstruction(this), left, right, k, areEqual, value) and
this.valueControls(block, value)
)
}

Expand All @@ -612,7 +627,8 @@ class IRGuardCondition extends Instruction {
cached
predicate ensuresEq(Operand op, int k, IRBlock block, boolean areEqual) {
exists(AbstractValue value |
unary_compares_eq(this, op, k, areEqual, false, value) and this.valueControls(block, value)
unary_compares_eq(getDerivedInstruction(this), op, k, areEqual, false, value) and
this.valueControls(block, value)
)
}

Expand All @@ -625,7 +641,7 @@ class IRGuardCondition extends Instruction {
Operand left, Operand right, int k, IRBlock pred, IRBlock succ, boolean areEqual
) {
exists(AbstractValue value |
compares_eq(this, left, right, k, areEqual, value) and
compares_eq(getDerivedInstruction(this), left, right, k, areEqual, value) and
this.valueControlsEdge(pred, succ, value)
)
}
Expand All @@ -637,7 +653,7 @@ class IRGuardCondition extends Instruction {
cached
predicate ensuresEqEdge(Operand op, int k, IRBlock pred, IRBlock succ, boolean areEqual) {
exists(AbstractValue value |
unary_compares_eq(this, op, k, areEqual, false, value) and
unary_compares_eq(getDerivedInstruction(this), op, k, areEqual, false, value) and
this.valueControlsEdge(pred, succ, value)
)
}
Expand Down Expand Up @@ -759,10 +775,12 @@ private predicate compares_eq(
or
/* (x is true => (left == right + k)) => (!x is false => (left == right + k)) */
exists(AbstractValue dual | value = dual.getDualValue() |
compares_eq(test.(LogicalNotInstruction).getUnary(), left, right, k, areEqual, dual)
compares_eq(getDerivedInstruction(test.(LogicalNotInstruction).getUnary()), left, right, k,
areEqual, dual)
)
or
compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), left, right, k, areEqual, value)
compares_eq(getDerivedInstruction(test.(BuiltinExpectCallInstruction).getCondition()), left,
right, k, areEqual, value)
}

/**
Expand Down Expand Up @@ -817,7 +835,8 @@ private predicate unary_compares_eq(
/* (x is true => (op == k)) => (!x is false => (op == k)) */
exists(AbstractValue dual, boolean inNonZeroCase0 |
value = dual.getDualValue() and
unary_compares_eq(test.(LogicalNotInstruction).getUnary(), op, k, inNonZeroCase0, areEqual, dual)
unary_compares_eq(getDerivedInstruction(test.(LogicalNotInstruction).getUnary()), op, k,
inNonZeroCase0, areEqual, dual)
|
k = 0 and inNonZeroCase = inNonZeroCase0
or
Expand Down Expand Up @@ -937,7 +956,7 @@ private predicate builtin_expect_eq(
exists(BuiltinExpectCallInstruction call, Instruction const, AbstractValue innerValue |
int_value(const) = 0 and
cmp.hasOperands(call.getAUse(), const.getAUse()) and
compares_eq(call.getCondition(), left, right, k, areEqual, innerValue)
compares_eq(getDerivedInstruction(call.getCondition()), left, right, k, areEqual, innerValue)
|
cmp instanceof CompareNEInstruction and
value = innerValue
Expand Down Expand Up @@ -968,7 +987,8 @@ private predicate unary_builtin_expect_eq(
exists(BuiltinExpectCallInstruction call, Instruction const, AbstractValue innerValue |
int_value(const) = 0 and
cmp.hasOperands(call.getAUse(), const.getAUse()) and
unary_compares_eq(call.getCondition(), op, k, areEqual, inNonZeroCase, innerValue)
unary_compares_eq(getDerivedInstruction(call.getCondition()), op, k, areEqual, inNonZeroCase,
innerValue)
|
cmp instanceof CompareNEInstruction and
value = innerValue
Expand Down Expand Up @@ -1008,7 +1028,8 @@ private predicate compares_lt(
or
/* (x is true => (left < right + k)) => (!x is false => (left < right + k)) */
exists(AbstractValue dual | value = dual.getDualValue() |
compares_lt(test.(LogicalNotInstruction).getUnary(), left, right, k, isLt, dual)
compares_lt(getDerivedInstruction(test.(LogicalNotInstruction).getUnary()), left, right, k,
isLt, dual)
)
}

Expand All @@ -1021,7 +1042,7 @@ private predicate compares_lt(Instruction test, Operand op, int k, boolean isLt,
or
/* (x is true => (op < k)) => (!x is false => (op < k)) */
exists(AbstractValue dual | value = dual.getDualValue() |
compares_lt(test.(LogicalNotInstruction).getUnary(), op, k, isLt, dual)
compares_lt(getDerivedInstruction(test.(LogicalNotInstruction).getUnary()), op, k, isLt, dual)
)
or
exists(int k1, int k2, ConstantInstruction const |
Expand Down