Skip to content

Commit 5f6d07d

Browse files
committed
C++: Fix performance of UnsignedGEZero.ql
This query used two fastTC operations that were already somewhat inefficient on their own but could send the evaluator into an OOM loop when run in parallel without enough RAM. The fix is to recurse manually, starting just from the expressions that are potential candidates for alerts.
1 parent bd94e80 commit 5f6d07d

File tree

1 file changed

+24
-15
lines changed

1 file changed

+24
-15
lines changed

cpp/ql/src/Likely Bugs/Arithmetic/UnsignedGEZero.qll

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,24 +15,33 @@ class ConstantZero extends Expr {
1515
}
1616
}
1717

18+
/**
19+
* Holds if `candidate` is an expression such that if it's unsigned then we
20+
* want an alert at `ge`.
21+
*/
22+
private predicate lookForUnsignedAt(GEExpr ge, Expr candidate) {
23+
// Base case: `candidate >= 0`
24+
ge.getRightOperand() instanceof ConstantZero and
25+
candidate = ge.getLeftOperand().getFullyConverted() and
26+
// left operand was a signed or unsigned IntegralType before conversions
27+
// (not a pointer, checking a pointer >= 0 is an entirely different mistake)
28+
// (not an enum, as the fully converted type of an enum is compiler dependent
29+
// so checking an enum >= 0 is always reasonable)
30+
ge.getLeftOperand().getUnderlyingType() instanceof IntegralType
31+
or
32+
// Recursive case: `...(largerType)candidate >= 0`
33+
exists(Conversion conversion |
34+
lookForUnsignedAt(ge, conversion) and
35+
candidate = conversion.getExpr() and
36+
conversion.getType().getSize() > candidate.getType().getSize()
37+
)
38+
}
39+
1840
class UnsignedGEZero extends GEExpr {
1941
UnsignedGEZero() {
20-
this.getRightOperand() instanceof ConstantZero and
21-
// left operand was a signed or unsigned IntegralType before conversions
22-
// (not a pointer, checking a pointer >= 0 is an entirely different mistake)
23-
// (not an enum, as the fully converted type of an enum is compiler dependent
24-
// so checking an enum >= 0 is always reasonable)
25-
getLeftOperand().getUnderlyingType() instanceof IntegralType and
2642
exists(Expr ue |
27-
// ue is some conversion of the left operand
28-
ue = getLeftOperand().getConversion*() and
29-
// ue is unsigned
30-
ue.getUnderlyingType().(IntegralType).isUnsigned() and
31-
// ue may be converted to zero or more strictly larger possibly signed types
32-
// before it is fully converted
33-
forall(Expr following | following = ue.getConversion+() |
34-
following.getType().getSize() > ue.getType().getSize()
35-
)
43+
lookForUnsignedAt(this, ue) and
44+
ue.getUnderlyingType().(IntegralType).isUnsigned()
3645
)
3746
}
3847
}

0 commit comments

Comments
 (0)