hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-17 01:03:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #1173 from jbj/alloca-enable

Browse Source 
C++: Enable cpp/alloca-in-loop on LGTM
This commit is contained in:
Geoffrey White
2019-05-01 12:48:59 +01:00
committed by GitHub
parent 1203c7305a 40aea2f76d
commit d4e1bae5d5
4 changed files with 514 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
change-notes/1.21/analysis-cpp.md
View File

@@ -8,6 +8,7 @@
|-----------------------------|-----------|--------------------------------------------------------------------|
| `()`-declared function called with too few arguments (`cpp/too-few-arguments`) | Correctness | Find all cases where the number of arguments is less than the number of parameters of the function, provided the function is also properly declared/defined elsewhere. |
| `()`-declared function called with mismatched arguments (`cpp/mismatched-function-arguments`) | Correctness | Find all cases where the types of arguments do not match the types of parameters of the function, provided the function is also properly declared/defined elsewhere. |
| Call to alloca in a loop (`cpp/alloca-in-loop`) | reliability, correctness, external/cwe/cwe-770 | Finds calls to `alloca` in loops, which can lead to stack overflow if the number of iterations is large. Newly displayed on LGTM. |
## Changes to existing queries

328
cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoop.ql
View File

@@ -1,36 +1,330 @@
/**
* @name alloca in a loop
* @name Call to alloca in a loop
* @description Using alloca in a loop can lead to a stack overflow
* @kind problem
* @problem.severity warning
* @precision high
* @id cpp/alloca-in-loop
* @tags reliability
* correctness
* security
* external/cwe/cwe-770
*/
import cpp
import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
import semmle.code.cpp.dataflow.DataFlow
Loop getAnEnclosingLoopOfExpr(Expr e) {
result = e.getEnclosingStmt().getParent*() or
result = getAnEnclosingLoopOfStmt(e.getEnclosingStmt())
}
/** Gets a loop that contains `e`. */
Loop getAnEnclosingLoopOfExpr(Expr e) { result = getAnEnclosingLoopOfStmt(e.getEnclosingStmt()) }
/** Gets a loop that contains `s`. */
Loop getAnEnclosingLoopOfStmt(Stmt s) {
result = s.getParent*() or
result = s.getParent*() and
not s = result.(ForStmt).getInitialization()
or
result = getAnEnclosingLoopOfExpr(s.getParent*())
}
from Loop l, FunctionCall fc
where
getAnEnclosingLoopOfExpr(fc) = l and
(
fc.getTarget().getName() = "__builtin_alloca"
/** A call to `alloca` in one of its forms. */
class AllocaCall extends FunctionCall {
AllocaCall() {
this.getTarget().getName() = "__builtin_alloca"
or
(
(fc.getTarget().getName() = "_alloca" or fc.getTarget().getName() = "_malloca") and
fc.getTarget().getADeclarationEntry().getFile().getBaseName() = "malloc.h"
(this.getTarget().getName() = "_alloca" or this.getTarget().getName() = "_malloca") and
this.getTarget().getADeclarationEntry().getFile().getBaseName() = "malloc.h"
}
}
/**
* A loop that contains an `alloca` call.
*/
class LoopWithAlloca extends Stmt {
LoopWithAlloca() { this = getAnEnclosingLoopOfExpr(any(AllocaCall ac)) }
/** Get an `alloca` call inside this loop. It may be in a nested loop. */
AllocaCall getAnAllocaCall() { this = getAnEnclosingLoopOfExpr(result) }
/**
* Holds if the condition of this loop will only be true if `e` is `truth`.
* For example, if the loop condition is `a == 0 && b`, then
* `conditionRequires(a, false)` and `conditionRequires(b, true)`.
*/
private predicate conditionRequires(Expr e, boolean truth) {
e = this.(Loop).getCondition() and
truth = true
or
// `e == 0`
exists(EQExpr eq |
conditionRequires(eq, truth.booleanNot()) and
eq.getAnOperand().getValue().toInt() = 0 and
e = eq.getAnOperand() and
not exists(e.getValue())
)
) and
not l.(DoStmt).getCondition().getValue() = "0"
select fc, "Stack allocation is inside a $@ and could lead to stack overflow.", l, l.toString()
or
// `e != 0`
exists(NEExpr eq |
conditionRequires(eq, truth) and
eq.getAnOperand().getValue().toInt() = 0 and
e = eq.getAnOperand() and
not exists(e.getValue())
)
or
// `(bool)e == true`
exists(EQExpr eq |
conditionRequires(eq, truth) and
eq.getAnOperand().getValue().toInt() = 1 and
e = eq.getAnOperand() and
e.getType().getUnspecifiedType() instanceof BoolType and
not exists(e.getValue())
)
or
// `(bool)e != true`
exists(NEExpr eq |
conditionRequires(eq, truth.booleanNot()) and
eq.getAnOperand().getValue().toInt() = 1 and
e = eq.getAnOperand() and
e.getType().getUnspecifiedType() instanceof BoolType and
not exists(e.getValue())
)
or
exists(NotExpr notExpr |
conditionRequires(notExpr, truth.booleanNot()) and
e = notExpr.getOperand()
)
or
// If the e of `this` requires `andExpr` to be true, then it
// requires both of its operand to be true as well.
exists(LogicalAndExpr andExpr |
truth = true and
conditionRequires(andExpr, truth) and
e = andExpr.getAnOperand()
)
or
// Dually, if the e of `this` requires `orExpr` to be false, then
// it requires both of its operand to be false as well.
exists(LogicalOrExpr orExpr |
truth = false and
conditionRequires(orExpr, truth) and
e = orExpr.getAnOperand()
)
}
/**
* Holds if the condition of this loop will only be true if `e` relates to
* `value` as `dir`. We don't keep track of whether the equality is strict
* since this predicate is only used to heuristically determine whether
* there's a reasonably tight upper bound on the number of loop iterations.
*
* For example, if the loop condition is `a < 2 && b`, then
* `conditionRequiresInequality(a, 2, Lesser())`.
*/
private predicate conditionRequiresInequality(Expr e, int value, RelationDirection dir) {
exists(RelationalOperation rel, Expr constant, boolean branch |
this.conditionRequires(rel, branch) and
relOpWithSwapAndNegate(rel, e.getFullyConverted(), constant, dir, _, branch) and
value = constant.getValue().toInt() and
not exists(e.getValue())
)
or
// Because we're not worried about off-by-one, it's not important whether
// the `CrementOperation` is a {pre,post}-{inc,dec}rement.
exists(CrementOperation inc |
this.conditionRequiresInequality(inc, value, dir) and
e = inc.getOperand()
)
}
/**
* Gets a variable that's restricted by `conditionRequires` or
* `conditionRequiresInequality`.
*/
private Variable getAControllingVariable() {
conditionRequires(result.getAnAccess(), _)
or
conditionRequiresInequality(result.getAnAccess(), _, _)
}
/**
* Gets a `VariableAccess` that changes `var` inside the loop body, where
* `var` is a controlling variable of this loop.
*/
private VariableAccess getAControllingVariableUpdate(Variable var) {
var = result.getTarget() and
var = this.getAControllingVariable() and
this = getAnEnclosingLoopOfExpr(result) and
result.isUsedAsLValue()
}
/**
* Holds if there is a control-flow path from the condition of this loop to
* `node` that doesn't update `var`, where `var` is a controlling variable of
* this loop. The path has to stay within the loop. The path will start at
* the successor of the loop condition. If the path reaches all the way back
* to the loop condition, then it's possible to go around the loop without
* updating `var`.
*/
private predicate conditionReachesWithoutUpdate(Variable var, ControlFlowNode node) {
// Don't leave the loop. It might cause us to leave the scope of `var`
(node instanceof Stmt implies this = getAnEnclosingLoopOfStmt(node)) and
(
node = this.(Loop).getCondition().getASuccessor() and
var = this.getAControllingVariable()
or
this.conditionReachesWithoutUpdate(var, node.getAPredecessor()) and
not node = this.getAControllingVariableUpdate(var)
)
}
/**
* Holds if all paths around the loop will update `var`, where `var` is a
* controlling variable of this loop.
*/
private predicate hasMandatoryUpdate(Variable var) {
not this.conditionReachesWithoutUpdate(var, this.(Loop).getCondition())
}
/**
* Gets a definition that may be the most recent definition of the
* controlling variable `var` before this loop.
*/
private DataFlow::Node getAPrecedingDef(Variable var) {
exists(VariableAccess va |
va = var.getAnAccess() and
this.conditionRequiresInequality(va, _, _) and
DataFlow::localFlow(result, DataFlow::exprNode(va)) and
// A source is outside the loop if it's not inside the loop
not exists(Expr e |
e = result.asExpr()
or
e = result.asDefiningArgument()
|
this = getAnEnclosingLoopOfExpr(e)
)
)
}
/**
* Gets a number that may be the most recent value assigned to the
* controlling variable `var` before this loop.
*/
private int getAControllingVarInitialValue(Variable var, DataFlow::Node source) {
source = this.getAPrecedingDef(var) and
result = source.asExpr().getValue().toInt()
}
/**
* Holds if the most recent definition of `var` before this loop may assign a
* value that is not a compile-time constant.
*/
private predicate controllingVarHasUnknownInitialValue(Variable var) {
// A definition without a constant value was reached
exists(DataFlow::Node source |
source = this.getAPrecedingDef(var) and
not exists(this.getAControllingVarInitialValue(var, source))
)
}
/**
* Gets the least possible value that the controlling variable `var` may have
* before this loop, if such a value can be deduced.
*/
private int getMinPrecedingDef(Variable var) {
not this.controllingVarHasUnknownInitialValue(var) and
result = min(this.getAControllingVarInitialValue(var, _))
or
this.controllingVarHasUnknownInitialValue(var) and
var.getType().(IntegralType).isUnsigned() and
result = 0
}
/**
* Gets the greatest possible value that the controlling variable `var` may
* have before this loop, if such a value can be deduced.
*/
private int getMaxPrecedingDef(Variable var) {
not this.controllingVarHasUnknownInitialValue(var) and
result = max(this.getAControllingVarInitialValue(var, _))
}
/**
* Holds if this loop has a "small" number of iterations. The meaning of
* "small" should be such that the loop wouldn't be unreasonably large if
* manually unrolled.
*/
predicate isTightlyBounded() {
exists(Variable var | this.hasMandatoryUpdate(var) |
this.conditionRequires(var.getAnAccess(), false) and
forall(VariableAccess update | update = this.getAControllingVariableUpdate(var) |
exists(AssignExpr assign |
assign.getLValue() = update and
assign.getRValue().getValue().toInt() != 0
)
)
or
this.conditionRequires(var.getAnAccess(), true) and
forall(VariableAccess update | update = this.getAControllingVariableUpdate(var) |
exists(AssignExpr assign |
assign.getLValue() = update and
assign.getRValue().getValue().toInt() = 0
)
)
or
exists(int bound |
this.conditionRequiresInequality(var.getAnAccess(), bound, Lesser()) and
bound - this.getMinPrecedingDef(var) <= 16 and
forall(VariableAccess update | update = this.getAControllingVariableUpdate(var) |
// var++;
// ++var;
exists(IncrementOperation inc | inc.getOperand() = update)
or
// var += positive_number;
exists(AssignAddExpr aa |
aa.getLValue() = update and
aa.getRValue().getValue().toInt() > 0
)
or
// var = var + positive_number;
// var = positive_number + var;
exists(AssignExpr assign, AddExpr add |
assign.getLValue() = update and
assign.getRValue() = add and
add.getAnOperand() = var.getAnAccess() and
add.getAnOperand().getValue().toInt() > 0
)
)
)
or
exists(int bound |
this.conditionRequiresInequality(var.getAnAccess(), bound, Greater()) and
this.getMaxPrecedingDef(var) - bound <= 16 and
forall(VariableAccess update | update = this.getAControllingVariableUpdate(var) |
// var--;
// --var;
exists(DecrementOperation inc | inc.getOperand() = update)
or
// var -= positive_number;
exists(AssignSubExpr aa |
aa.getLValue() = update and
aa.getRValue().getValue().toInt() > 0
)
or
// var = var - positive_number;
exists(AssignExpr assign, SubExpr add |
assign.getLValue() = update and
assign.getRValue() = add and
add.getLeftOperand() = var.getAnAccess() and
add.getRightOperand().getValue().toInt() > 0
)
)
)
)
}
}
from LoopWithAlloca l
where
not l.(DoStmt).getCondition().getValue() = "0" and
not l.isTightlyBounded()
select l.getAnAllocaCall(), "Stack allocation is inside a $@ loop.", l,
l.toString()

25
cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop.expected
View File

@@ -1,8 +1,17 @@
| AllocaInLoop1.cpp:31:18:31:23 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1.cpp:22:2:39:2 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1.cpp:55:19:55:24 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1.cpp:45:2:64:2 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1.cpp:80:19:80:24 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1.cpp:71:3:88:3 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1ms.cpp:28:18:28:24 | call to _alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1ms.cpp:19:2:36:2 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1ms.cpp:52:19:52:26 | call to _malloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1ms.cpp:42:2:63:2 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1ms.cpp:79:19:79:25 | call to _alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop1ms.cpp:70:3:87:3 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop2.c:39:30:39:35 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop2.c:29:5:48:19 | do (...) ... | do (...) ... |
| AllocaInLoop3.cpp:45:23:45:28 | call to __builtin_alloca | Stack allocation is inside a $@ and could lead to stack overflow. | AllocaInLoop3.cpp:43:2:49:19 | do (...) ... | do (...) ... |
| AllocaInLoop1.cpp:31:18:31:23 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1.cpp:22:2:39:2 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1.cpp:55:19:55:24 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1.cpp:45:2:64:2 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1.cpp:80:19:80:24 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1.cpp:71:3:88:3 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1ms.cpp:28:18:28:24 | call to _alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1ms.cpp:19:2:36:2 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1ms.cpp:52:19:52:26 | call to _malloca | Stack allocation is inside a $@ loop. | AllocaInLoop1ms.cpp:42:2:63:2 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop1ms.cpp:79:19:79:25 | call to _alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1ms.cpp:70:3:87:3 | for(...;...;...) ... | for(...;...;...) ... |
| AllocaInLoop2.c:39:30:39:35 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop2.c:29:5:48:19 | do (...) ... | do (...) ... |
| AllocaInLoop3.cpp:45:23:45:28 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop3.cpp:43:2:49:19 | do (...) ... | do (...) ... |
| BoundedLoop.cpp:25:5:25:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:24:3:26:3 | for(...;...;...) ... | for(...;...;...) ... |
| BoundedLoop.cpp:38:5:38:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:37:3:39:3 | for(...;...;...) ... | for(...;...;...) ... |
| BoundedLoop.cpp:55:5:55:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:54:3:59:3 | while (...) ... | while (...) ... |
| BoundedLoop.cpp:64:5:64:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:63:3:68:3 | for(...;...;...) ... | for(...;...;...) ... |
| BoundedLoop.cpp:73:5:73:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:72:3:74:3 | for(...;...;...) ... | for(...;...;...) ... |
| BoundedLoop.cpp:97:5:97:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:96:3:98:3 | for(...;...;...) ... | for(...;...;...) ... |
| BoundedLoop.cpp:105:5:105:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:104:3:106:3 | for(...;...;...) ... | for(...;...;...) ... |
| BoundedLoop.cpp:138:5:138:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:137:3:139:3 | for(...;...;...) ... | for(...;...;...) ... |
| BoundedLoop.cpp:176:5:176:10 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | BoundedLoop.cpp:175:3:177:3 | for(...;...;...) ... | for(...;...;...) ... |

185
cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/BoundedLoop.cpp Normal file
View File

@@ -0,0 +1,185 @@
void *__builtin_alloca(unsigned long sz);
#define alloca __builtin_alloca
void forOnce() {
for (struct { bool stop; } state = { 0 }; !state.stop; state.stop = 1) {
alloca(100); // GOOD
}
}
void forOnce2() {
bool stop;
for (stop = 0; !stop; stop = 1) {
alloca(100); // GOOD
}
}
void forTwice() {
for (int i = 0; i < 2; i++) {
alloca(100); // GOOD
}
}
void forEver() {
for (;;) {
alloca(100); // BAD
}
}
void doTwice() {
int i = 0;
do {
alloca(100); // GOOD
} while (++i < 2);
}
void unknownStartingPoint(int i) {
for (; i < 2; i++) {
alloca(100); // BAD
}
}
int getInt();
void atMostTwice() {
int i = 0;
while (!(i >= 2 || getInt())) {
i++;
alloca(100); // GOOD
}
}
void sometimesIncrement() {
int i = 0;
while (i < 2) {
alloca(100); // BAD
if (getInt()) {
i++;
}
}
}
void upAndDown() {
for (int i = 0; i < 2; i++) {
alloca(100); // BAD
if (getInt()) {
i--;
}
}
}
void largeBound() {
for (int i = 0; i < 10000; i++) {
alloca(100); // BAD
}
}
void largeOffset() {
int i;
if (getInt()) {
i = 9998;
} else {
i = 9997;
}
for (; i < 10000; i++) {
alloca(100); // GOOD
}
}
void maybeSmallOffset() {
int i;
if (getInt()) {
i = 0;
} else {
i = 9997;
}
for (; i < 10000; i++) {
alloca(100); // BAD
}
}
void incBefore() {
int i = -1;
i++; // not understood by data flow
for (; i < 2; i++) {
alloca(100); // GOOD [FALSE POSITIVE]
}
}
void nestedAddsUp() {
for (int i = 0; i < 16; i++) {
for (int j = 0; j < 16; j++) {
alloca(100); // BAD [NOT DETECTED]
}
}
}
void nestedWithReset() {
bool stop = 0;
for (int i = 0; i < 1; i++) {
stop = 0;
do {
stop = 1;
alloca(100); // GOOD
} while (!stop);
stop = 0;
}
}
void eqFalse() {
for (int stop = 0; stop == 0; stop = 5) {
alloca(100); // GOOD
}
}
void eqFalseFlipped() {
for (int stop = 0; stop == 0; stop = 0) {
alloca(100); // BAD
}
}
void neFalse() {
for (bool go_on = true; go_on != 0; go_on = false) {
alloca(100); // GOOD
}
}
void eqTrue() {
for (bool go_on = false; go_on == true; go_on = false) {
alloca(100); // GOOD
}
}
void countDownFor() {
for (int i = 2; i >= 0; i--) {
alloca(100); // GOOD
}
}
void countDownWhile() {
int i;
i = 2;
while (--i >= 0) {
alloca(100); // GOOD
}
}
void countDownAssignAdd() {
for (int i = 2; i >= 0; i = i - 1) {
alloca(100); // GOOD
}
}
void countDownWrong() {
for (int i = 2-1; i >= 0; i++) {
alloca(100); // BAD
}
}
void countUpFromUnsigned(unsigned long i) {
while (i < 10) {
i = i + 1;
alloca(100); // GOOD
}
}