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
Closed

Brodes/guard flow parsing #17907

Show file tree
Hide file tree
Changes from all commits
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
232 changes: 154 additions & 78 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,54 @@

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

/**
* Variant of valueNumber that accounts for conversions.
* Note: this predicate is defined in IRGuards to take advantage of
* IRGuards definition of int_value.
*/
ValueNumber valueNumberWithConversions(Instruction instr) {
exists(ValueNumber vn | vn = valueNumber(instr) |
// GVN using valueNumber directly
result = vn
or
// GVN for the ConvertInstruction conversion
result = valueNumberWithConversions(vn.getAnInstruction().(ConvertInstruction).getUnary())
or
// GVN for the CompareNEInstruction conversion for a comparison with 0 (conversion of value to boolean in c)
exists(CompareNEInstruction cmp | cmp = vn.getAnInstruction() |
result = valueNumberWithConversions(cmp.getLeft()) and
int_value(cmp.getRight()) = 0
)
)
}

/**
* Returns `instr` or any instruction used to define `instr`.
*/
pragma[inline]
private Instruction getDerivedInstruction(Instruction instr) {
result = instr
or
result = valueNumber(instr).getAnInstruction().(StoreInstruction).getSourceValue()
or
result = derivedThroughConversion(valueNumber(instr))
}

private Instruction derivedThroughConversion(ValueNumber vn) {
// GVN for the ConvertInstruction conversion
result = getDerivedInstruction(vn.getAnInstruction().(ConvertInstruction).getUnary())
or
// GVN for the CompareNEInstruction conversion for a comparison with 0 (conversion of value to boolean in c)
exists(CompareNEInstruction comp | comp = vn.getAnInstruction() |
result = getDerivedInstruction(comp.getLeft()) and
int_value(comp.getRight()) = 0
)
}

/**
* Holds if `block` consists of an `UnreachedInstruction`.
*
Expand Down Expand Up @@ -538,7 +583,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(this, left, right, k, isLessThan, value) and
this.valueControls(block, value)
)
}

Expand All @@ -549,7 +595,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(this, op, k, isLessThan, value) and
this.valueControls(block, value)
)
}

Expand Down Expand Up @@ -601,7 +648,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(this, left, right, k, areEqual, value) and
this.valueControls(block, value)
)
}

Expand All @@ -612,7 +660,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(this, op, k, areEqual, false, value) and
this.valueControls(block, value)
)
}

Expand Down Expand Up @@ -742,27 +791,30 @@ private Instruction getBranchForCondition(Instruction guard) {
private predicate compares_eq(
Instruction test, Operand left, Operand right, int k, boolean areEqual, AbstractValue value
) {
/* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
exists(AbstractValue v | simple_comparison_eq(test, left, right, k, v) |
areEqual = true and value = v
exists(Instruction derived | derived = getDerivedInstruction(test) |
/* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
exists(AbstractValue v | simple_comparison_eq(derived, left, right, k, v) |
areEqual = true and value = v
or
areEqual = false and value = v.getDualValue()
)
or
areEqual = false and value = v.getDualValue()
)
or
// I think this is handled by forwarding in controlsBlock.
//or
//logical_comparison_eq(test, left, right, k, areEqual, testIsTrue)
/* a == b + k => b == a - k */
exists(int mk | k = -mk | compares_eq(test, right, left, mk, areEqual, value))
or
complex_eq(test, left, right, k, areEqual, value)
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)
// I think this is handled by forwarding in controlsBlock.
//or
//logical_comparison_eq(test, left, right, k, areEqual, testIsTrue)
/* a == b + k => b == a - k */
exists(int mk | k = -mk | compares_eq(derived, right, left, mk, areEqual, value))
or
complex_eq(derived, left, right, k, areEqual, value)
or
/* (x is true => (left == right + k)) => (!x is false => (left == right + k)) */
exists(AbstractValue dual | value = dual.getDualValue() |
compares_eq(derived.(LogicalNotInstruction).getUnary(), left, right, k, areEqual, dual)
)
or
compares_eq(derived.(BuiltinExpectCallInstruction).getCondition(), left, right, k, areEqual,
value)
)
or
compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), left, right, k, areEqual, value)
}

/**
Expand Down Expand Up @@ -803,38 +855,41 @@ private predicate compares_eq(
private predicate unary_compares_eq(
Instruction test, Operand op, int k, boolean areEqual, boolean inNonZeroCase, AbstractValue value
) {
/* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
exists(AbstractValue v |
unary_simple_comparison_eq(test, k, inNonZeroCase, v) and op.getDef() = test
|
areEqual = true and value = v
exists(Instruction derived | derived = getDerivedInstruction(test) |
/* The simple case where the test *is* the comparison so areEqual = testIsTrue xor eq. */
exists(AbstractValue v |
unary_simple_comparison_eq(derived, k, inNonZeroCase, v) and op.getDef() = derived
|
areEqual = true and value = v
or
areEqual = false and value = v.getDualValue()
)
or
areEqual = false and value = v.getDualValue()
)
or
unary_complex_eq(test, op, k, areEqual, inNonZeroCase, value)
or
/* (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)
|
k = 0 and inNonZeroCase = inNonZeroCase0
unary_complex_eq(derived, op, k, areEqual, inNonZeroCase, value)
or
k != 0 and inNonZeroCase = true
)
or
// ((test is `areEqual` => op == const + k2) and const == `k1`) =>
// test is `areEqual` => op == k1 + k2
inNonZeroCase = false and
exists(int k1, int k2, ConstantInstruction const |
compares_eq(test, op, const.getAUse(), k2, areEqual, value) and
int_value(const) = k1 and
k = k1 + k2
/* (x is true => (op == k)) => (!x is false => (op == k)) */
exists(AbstractValue dual, boolean inNonZeroCase0 |
value = dual.getDualValue() and
unary_compares_eq(derived.(LogicalNotInstruction).getUnary(), op, k, inNonZeroCase0, areEqual,
dual)
|
k = 0 and inNonZeroCase = inNonZeroCase0
or
k != 0 and inNonZeroCase = true
)
or
// ((test is `areEqual` => op == const + k2) and const == `k1`) =>
// test is `areEqual` => op == k1 + k2
inNonZeroCase = false and
exists(int k1, int k2, ConstantInstruction const |
compares_eq(derived, op, const.getAUse(), k2, areEqual, value) and
int_value(const) = k1 and
k = k1 + k2
)
or
unary_compares_eq(derived.(BuiltinExpectCallInstruction).getCondition(), op, k, areEqual,
inNonZeroCase, value)
)
or
unary_compares_eq(test.(BuiltinExpectCallInstruction).getCondition(), op, k, areEqual,
inNonZeroCase, value)
}

/** Rearrange various simple comparisons into `left == right + k` form. */
Expand Down Expand Up @@ -868,6 +923,18 @@ private predicate unary_simple_comparison_eq(
inNonZeroCase = false
)
or
(
exists(BinaryLogicalOperation e | e.getAnOperand() = test.getUnconvertedResultExpression())
or
exists(UnaryLogicalOperation e | e.getAnOperand() = test.getUnconvertedResultExpression())
or
exists(IfStmt i | i.getCondition() = test.getUnconvertedResultExpression())
or
exists(Loop i | i.getCondition() = test.getUnconvertedResultExpression())
or
exists(ConditionalExpr c | c.getCondition() = test.getUnconvertedResultExpression())
// Todo recursive conditionalExpr in guard
Comment on lines +926 to +936
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to do the pruning like this. We really want to avoid depending on the specific syntax this deep inside the libraries.

And as you say yourself: This also still misses some cases. Once this is done properly and completely, we can avoid these ad-hoc conjuncts below:

  not test.isGLValue() and
  not simple_comparison_eq(test, _, _, _, _) and
  not simple_comparison_lt(test, _, _, _) and
  not test = any(SwitchInstruction switch).getExpression() and
  (
    test.getResultIRType() instanceof IRAddressType or
    test.getResultIRType() instanceof IRIntegerType or
    test.getResultIRType() instanceof IRBooleanType
  )

) and
// Any instruction with an integral type could potentially be part of a
// check for nullness when used in a guard. So we include all integral
// typed instructions here. However, since some of these instructions are
Expand Down Expand Up @@ -912,6 +979,8 @@ private predicate unary_simple_comparison_eq(
value.(BooleanValue).getValue() = false and
inNonZeroCase = false
)
// Has to be in a boolean operator (operanad of OR or AND)
// has to be a 'guard' (loop, if, switch, ternary)
}

/** A call to the builtin operation `__builtin_expect`. */
Expand Down Expand Up @@ -997,37 +1066,44 @@ private predicate unary_complex_eq(
private predicate compares_lt(
Instruction test, Operand left, Operand right, int k, boolean isLt, AbstractValue value
) {
/* In the simple case, the test is the comparison, so isLt = testIsTrue */
simple_comparison_lt(test, left, right, k) and
value.(BooleanValue).getValue() = isLt
or
complex_lt(test, left, right, k, isLt, value)
or
/* (not (left < right + k)) => (left >= right + k) */
exists(boolean isGe | isLt = isGe.booleanNot() | compares_ge(test, left, right, k, isGe, value))
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)
exists(Instruction derived | derived = getDerivedInstruction(test) |
// exists(thing | thing = derived ... )
/* In the simple case, the test is the comparison, so isLt = testIsTrue */
simple_comparison_lt(derived, left, right, k) and
value.(BooleanValue).getValue() = isLt
or
complex_lt(derived, left, right, k, isLt, value)
or
/* (not (left < right + k)) => (left >= right + k) */
exists(boolean isGe | isLt = isGe.booleanNot() |
compares_ge(derived, left, right, k, isGe, value)
)
or
/* (x is true => (left < right + k)) => (!x is false => (left < right + k)) */
exists(AbstractValue dual | value = dual.getDualValue() |
compares_lt(derived.(LogicalNotInstruction).getUnary(), left, right, k, isLt, dual)
)
)
}

/** Holds if `op < k` evaluates to `isLt` given that `test` evaluates to `value`. */
private predicate compares_lt(Instruction test, Operand op, int k, boolean isLt, AbstractValue value) {
unary_simple_comparison_lt(test, k, isLt, value) and
op.getDef() = test
or
complex_lt(test, op, k, isLt, value)
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)
)
or
exists(int k1, int k2, ConstantInstruction const |
compares_lt(test, op, const.getAUse(), k2, isLt, value) and
int_value(const) = k1 and
k = k1 + k2
exists(Instruction derived | derived = getDerivedInstruction(test) |
unary_simple_comparison_lt(derived, k, isLt, value) and
op.getDef() = derived
or
complex_lt(derived, op, k, isLt, value)
or
/* (x is true => (op < k)) => (!x is false => (op < k)) */
exists(AbstractValue dual | value = dual.getDualValue() |
compares_lt(derived.(LogicalNotInstruction).getUnary(), op, k, isLt, dual)
)
or
exists(int k1, int k2, ConstantInstruction const |
compares_lt(derived, op, const.getAUse(), k2, isLt, value) and
int_value(const) = k1 and
k = k1 + k2
)
)
}

Expand Down
11 changes: 11 additions & 0 deletions cpp/ql/test/library-tests/controlflow/guards-ir/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,14 @@ int abort_test(int x) {
abort();
}
}

void boolean_flow(int x){
int b = x > 5;

if(!b){
}

if(b&&x<100){

}
}
Loading