Skip to content

Commit 56d7342

Browse files
MathiasVPMathiasVP
committed
C++: Improve the cpp/detect-and-handle-memory-allocation-errors query.
1 parent 58f3048 commit 56d7342

File tree

1 file changed

+160
-56
lines changed

1 file changed

+160
-56
lines changed

‎cpp/ql/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql

Lines changed: 160 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -11,76 +11,180 @@
1111
*/
1212

1313
import cpp
14+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
15+
import semmle.code.cpp.controlflow.Guards
1416

1517
/**
16-
* Lookup if condition compare with 0
18+
* A C++ `delete` or `delete[]` expression.
1719
*/
18-
class IfCompareWithZero extends IfStmt {
19-
IfCompareWithZero() {
20-
this.getCondition().(EQExpr).getAChild().getValue() = "0"
21-
or
22-
this.getCondition().(NEExpr).getAChild().getValue() = "0" and
23-
this.hasElse()
24-
or
25-
this.getCondition().(NEExpr).getAChild().getValue() = "0" and
26-
this.getThen().getAChild*() instanceof ReturnStmt
20+
class DeleteOrDeleteArrayExpr extends Expr {
21+
DeleteOrDeleteArrayExpr() { this instanceof DeleteExpr or this instanceof DeleteArrayExpr }
22+
23+
DeallocationFunction getDeallocator() {
24+
result = [this.(DeleteExpr).getDeallocator(), this.(DeleteArrayExpr).getDeallocator()]
2725
}
2826
}
2927

28+
/** Gets the `Constructor` invoked when `newExpr` allocates memory. */
29+
Constructor getConstructorForAllocation(NewOrNewArrayExpr newExpr) {
30+
result.getACallToThisFunction().getLocation() = newExpr.getLocation()
31+
}
32+
33+
/** Gets the `Destructor` invoked when `deleteExpr` deallocates memory. */
34+
Destructor getDestructorForDeallocation(DeleteOrDeleteArrayExpr deleteExpr) {
35+
result.getACallToThisFunction().getLocation() = deleteExpr.getLocation()
36+
}
37+
38+
/** Holds if the evaluation of `newExpr` may throw an exception. */
39+
predicate newMayThrow(NewOrNewArrayExpr newExpr) {
40+
functionMayThrow(newExpr.getAllocator()) or
41+
functionMayThrow(getConstructorForAllocation(newExpr))
42+
}
43+
44+
/** Holds if the evaluation of `deleteExpr` may throw an exception. */
45+
predicate deleteMayThrow(DeleteOrDeleteArrayExpr deleteExpr) {
46+
functionMayThrow(deleteExpr.getDeallocator()) or
47+
functionMayThrow(getDestructorForDeallocation(deleteExpr))
48+
}
49+
3050
/**
31-
* lookup for calls to `operator new`, with incorrect error handling.
51+
* Holds if the function may throw an exception when called. That is, if the body of the function looks
52+
* like it might throw an exception, and the function does not have a `noexcept` or `throw()` specifier.
3253
*/
33-
class WrongCheckErrorOperatorNew extends FunctionCall {
34-
Expr exp;
35-
36-
WrongCheckErrorOperatorNew() {
37-
this = exp.(NewOrNewArrayExpr).getAChild().(FunctionCall) and
38-
(
39-
this.getTarget().hasGlobalOrStdName("operator new")
40-
or
41-
this.getTarget().hasGlobalOrStdName("operator new[]")
54+
predicate functionMayThrow(Function f) {
55+
(not exists(f.getBlock()) or stmtMayThrow(f.getBlock())) and
56+
not f.isNoExcept() and
57+
not f.isNoThrow()
58+
}
59+
60+
/** Holds if the evaluation of `stmt` may throw an exception. */
61+
predicate stmtMayThrow(Stmt stmt) {
62+
stmtMayThrow(stmt.(BlockStmt).getAStmt())
63+
or
64+
convertedExprMayThrow(stmt.(ExprStmt).getExpr())
65+
or
66+
exists(IfStmt ifStmt | ifStmt = stmt |
67+
convertedExprMayThrow(ifStmt.getCondition()) or
68+
stmtMayThrow([ifStmt.getThen(), ifStmt.getElse()])
69+
)
70+
or
71+
exists(Loop loop | loop = stmt |
72+
convertedExprMayThrow(loop.getCondition()) or
73+
stmtMayThrow(loop.getStmt())
74+
)
75+
}
76+
77+
/** Holds if the evaluation of `e` (including conversions) may throw an exception. */
78+
predicate convertedExprMayThrow(Expr e) { exprMayThrow(e.getFullyConverted()) }
79+
80+
/** Holds if the evaluation of `e` may throw an exception. */
81+
predicate exprMayThrow(Expr e) {
82+
e instanceof DynamicCast
83+
or
84+
e instanceof TypeidOperator
85+
or
86+
e instanceof ThrowExpr
87+
or
88+
newMayThrow(e)
89+
or
90+
deleteMayThrow(e)
91+
or
92+
convertedExprMayThrow(e.(UnaryOperation).getOperand())
93+
or
94+
exists(BinaryOperation binOp | binOp = e |
95+
convertedExprMayThrow([binOp.getLeftOperand(), binOp.getRightOperand()])
96+
)
97+
or
98+
exists(CommaExpr comma | comma = e |
99+
convertedExprMayThrow([comma.getLeftOperand(), comma.getRightOperand()])
100+
)
101+
or
102+
exists(StmtExpr stmtExpr | stmtExpr = e |
103+
convertedExprMayThrow(stmtExpr.getResultExpr()) or
104+
stmtMayThrow(stmtExpr.getStmt())
105+
)
106+
or
107+
convertedExprMayThrow(e.(Conversion).getExpr())
108+
or
109+
exists(FunctionCall fc | fc = e |
110+
not exists(fc.getTarget()) or
111+
functionMayThrow(fc.getTarget()) or
112+
convertedExprMayThrow(fc.getAnArgument())
113+
)
114+
}
115+
116+
/** An allocator that will not throw an exception. */
117+
class NoThrowAllocator extends Function {
118+
NoThrowAllocator() {
119+
exists(NewOrNewArrayExpr newExpr |
120+
newExpr.getAllocator() = this and
121+
not functionMayThrow(this)
42122
)
43123
}
124+
}
44125

45-
/**
46-
* Holds if handler `try ... catch` exists.
47-
*/
48-
predicate isExistsTryCatchBlock() {
49-
exists(TryStmt ts | this.getEnclosingStmt() = ts.getStmt().getAChild*())
50-
}
126+
/** An allocator that might throw an exception. */
127+
class ThrowingAllocator extends Function {
128+
ThrowingAllocator() { not this instanceof NoThrowAllocator }
129+
}
51130

52-
/**
53-
* Holds if results call `operator new` check in `operator if`.
54-
*/
55-
predicate isExistsIfCondition() {
56-
exists(IfCompareWithZero ifc, AssignExpr aex, Initializer it |
57-
// call `operator new` directly from the condition of `operator if`.
58-
this = ifc.getCondition().getAChild*()
59-
or
60-
// check results call `operator new` with variable appropriation
61-
postDominates(ifc, this) and
62-
aex.getAChild() = exp and
63-
ifc.getCondition().getAChild().(VariableAccess).getTarget() =
64-
aex.getLValue().(VariableAccess).getTarget()
65-
or
66-
// check results call `operator new` with declaration variable
67-
postDominates(ifc, this) and
68-
exp = it.getExpr() and
69-
it.getDeclaration() = ifc.getCondition().getAChild().(VariableAccess).getTarget()
70-
)
131+
/** The `std::bad_alloc` exception and its `bsl` variant. */
132+
class BadAllocType extends Class {
133+
BadAllocType() { this.hasGlobalOrStdOrBslName("bad_alloc") }
134+
}
135+
136+
/**
137+
* A catch block that catches a `std::bad_alloc` (or any of its subclasses), or a catch
138+
* block that catches every exception (i.e., `catch(...)`).
139+
*/
140+
class BadAllocCatchBlock extends CatchBlock {
141+
BadAllocCatchBlock() {
142+
this.getParameter().getUnspecifiedType() = any(BadAllocType badAlloc).getADerivedClass*()
143+
or
144+
not exists(this.getParameter())
71145
}
146+
}
147+
148+
/**
149+
* Holds if `newExpr` will not throw an exception, but is embedded in a `try` statement
150+
* with a catch block `catchBlock` that catches an `std::bad_alloc` exception.
151+
*/
152+
predicate noThrowInTryBlock(NewOrNewArrayExpr newExpr, BadAllocCatchBlock catchBlock) {
153+
exists(TryStmt try |
154+
forall(Expr cand | cand.getEnclosingBlock().getEnclosingBlock*() = try.getStmt() |
155+
not convertedExprMayThrow(cand)
156+
) and
157+
try.getACatchClause() = catchBlock and
158+
newExpr.getEnclosingBlock().getEnclosingBlock*() = try.getStmt() and
159+
not newMayThrow(newExpr)
160+
)
161+
}
72162

73-
/**
74-
* Holds if `(std::nothrow)` or `(std::noexcept)` exists in call `operator new`.
75-
*/
76-
predicate isExistsNothrow() { getTarget().isNoExcept() or getTarget().isNoThrow() }
163+
/**
164+
* Holds if `newExpr` is handles allocation failures by throwing an exception, yet
165+
* the guard condition `guard` compares the result of `newExpr` to a null value.
166+
*/
167+
predicate nullCheckInThrowingNew(NewOrNewArrayExpr newExpr, GuardCondition guard) {
168+
newExpr.getAllocator() instanceof ThrowingAllocator and
169+
(
170+
// Handles null comparisons.
171+
guard.ensuresEq(globalValueNumber(newExpr).getAnExpr(), any(NullValue null), _, _, _)
172+
or
173+
// Handles `if(ptr)` and `if(!ptr)` cases.
174+
guard = globalValueNumber(newExpr).getAnExpr()
175+
)
77176
}
78177

79-
from WrongCheckErrorOperatorNew op
178+
from NewOrNewArrayExpr newExpr, Element element, string msg, string elementString
80179
where
81-
// use call `operator new` with `(std::nothrow)` and checking error using `try ... catch` block and not `operator if`
82-
op.isExistsNothrow() and not op.isExistsIfCondition() and op.isExistsTryCatchBlock()
83-
or
84-
// use call `operator new` without `(std::nothrow)` and checking error using `operator if` and not `try ... catch` block
85-
not op.isExistsNothrow() and not op.isExistsTryCatchBlock() and op.isExistsIfCondition()
86-
select op, "memory allocation error check is incorrect or missing"
180+
not newExpr.isFromUninstantiatedTemplate(_) and
181+
(
182+
noThrowInTryBlock(newExpr, element) and
183+
msg = "This allocation cannot throw. $@ is unnecessary." and
184+
elementString = "This catch block"
185+
or
186+
nullCheckInThrowingNew(newExpr, element) and
187+
msg = "This allocation cannot return null. $@ is unnecessary." and
188+
elementString = "This check"
189+
)
190+
select newExpr, msg, element, elementString

0 commit comments

Comments
 (0)