Skip to content

Brodes/guard flow parsing #17907

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

Closed
bdrodes 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
56 changes: 48 additions & 8 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 @@ -9,22 +9,48 @@ 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) {
// The instruction itself
result = instr
or
// or the GVN definition for the current instruction
result = valueNumber(instr).getAnInstruction().(StoreInstruction).getSourceValue()
or
// Special handling for conversions
result =
getDerivedInstruction(valueNumber(instr).getAnInstruction().(CompareNEInstruction).getLeft())
result = derivedThroughConversion(valueNumber(instr))
}

private Instruction derivedThroughConversion(ValueNumber vn) {
// GVN for the ConvertInstruction conversion
result = getDerivedInstruction(vn.getAnInstruction().(ConvertInstruction).getUnary())
or
result =
getDerivedInstruction(valueNumber(instr).getAnInstruction().(ConvertInstruction).getUnary())
// 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
)
}

/**
Expand Down Expand Up @@ -832,7 +858,7 @@ private predicate unary_compares_eq(
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() = test
unary_simple_comparison_eq(derived, k, inNonZeroCase, v) and op.getDef() = derived
|
areEqual = true and value = v
or
Expand Down Expand Up @@ -897,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

@MathiasVP MathiasVP Nov 14, 2024

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 @@ -941,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