Skip to content

Commit 604731b

Browse files
jbjjbj
authored
Merge pull request github#3171 from MathiasVP/init-dynamic-alloc-newexpr
C++: Emit InitializeDynamicAllocation instructions for NewExpr and NewArrayExpr
2 parents 4825774 + e2908ea commit 604731b

File tree

14 files changed

+809
-191
lines changed

14 files changed

+809
-191
lines changed

‎cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

Lines changed: 68 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,44 @@ abstract class TranslatedCall extends TranslatedExpr {
206206

207207
predicate hasPreciseSideEffect() { exists(getSideEffects()) }
208208

209-
TranslatedSideEffects getSideEffects() { result.getCall() = expr }
209+
final TranslatedSideEffects getSideEffects() { result.getExpr() = expr }
210+
}
211+
212+
abstract class TranslatedSideEffects extends TranslatedElement {
213+
abstract Expr getExpr();
214+
215+
final override Locatable getAST() { result = getExpr() }
216+
217+
final override Function getFunction() { result = getExpr().getEnclosingFunction() }
218+
219+
override TranslatedElement getChild(int i) {
220+
result =
221+
rank[i + 1](TranslatedSideEffect tse, int isWrite, int index |
222+
(
223+
tse.getCall() = getExpr() and
224+
tse.getArgumentIndex() = index and
225+
if tse.isWrite() then isWrite = 1 else isWrite = 0
226+
)
227+
|
228+
tse order by isWrite, index
229+
)
230+
}
231+
232+
final override Instruction getChildSuccessor(TranslatedElement te) {
233+
exists(int i |
234+
getChild(i) = te and
235+
if exists(getChild(i + 1))
236+
then result = getChild(i + 1).getFirstInstruction()
237+
else result = getParent().getChildSuccessor(this)
238+
)
239+
}
240+
241+
/**
242+
* Gets the `TranslatedFunction` containing this expression.
243+
*/
244+
final TranslatedFunction getEnclosingFunction() {
245+
result = getTranslatedFunction(getExpr().getEnclosingFunction())
246+
}
210247
}
211248

212249
/**
@@ -308,66 +345,27 @@ class TranslatedStructorCall extends TranslatedFunctionCall {
308345
override predicate hasQualifier() { any() }
309346
}
310347

311-
class TranslatedSideEffects extends TranslatedElement, TTranslatedSideEffects {
312-
Call expr;
313-
314-
TranslatedSideEffects() { this = TTranslatedSideEffects(expr) }
348+
class TranslatedAllocationSideEffects extends TranslatedSideEffects,
349+
TTranslatedAllocationSideEffects {
350+
AllocationExpr expr;
315351

316-
override string toString() { result = "(side effects for " + expr.toString() + ")" }
352+
TranslatedAllocationSideEffects() { this = TTranslatedAllocationSideEffects(expr) }
317353

318-
override Locatable getAST() { result = expr }
354+
final override AllocationExpr getExpr() { result = expr }
319355

320-
Call getCall() { result = expr }
356+
override string toString() { result = "(allocation side effects for " + expr.toString() + ")" }
321357

322-
override TranslatedElement getChild(int i) {
323-
result =
324-
rank[i + 1](TranslatedSideEffect tse, int isWrite, int index |
325-
(
326-
tse.getCall() = getCall() and
327-
tse.getArgumentIndex() = index and
328-
if tse.isWrite() then isWrite = 1 else isWrite = 0
329-
)
330-
|
331-
tse order by isWrite, index
332-
)
333-
}
334-
335-
override Instruction getChildSuccessor(TranslatedElement te) {
336-
exists(int i |
337-
getChild(i) = te and
338-
if exists(getChild(i + 1))
339-
then result = getChild(i + 1).getFirstInstruction()
340-
else result = getParent().getChildSuccessor(this)
341-
)
342-
}
358+
override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
343359

344360
override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) {
345-
expr.getTarget() instanceof AllocationFunction and
346-
not exists(NewOrNewArrayExpr newExpr |
347-
// we synthesize allocator calls for `new` and `new[]`, so don't add instructions to
348-
// the existing allocator call when it exists.
349-
newExpr.getAllocatorCall() = expr
350-
) and
351361
opcode instanceof Opcode::InitializeDynamicAllocation and
352362
tag = OnlyInstructionTag() and
353363
type = getUnknownType()
354364
}
355365

356-
override Instruction getFirstInstruction() {
357-
if expr.getTarget() instanceof AllocationFunction
358-
then result = getInstruction(OnlyInstructionTag())
359-
else result = getChild(0).getFirstInstruction()
360-
}
361-
362366
override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
363367
tag = OnlyInstructionTag() and
364368
kind = gotoEdge() and
365-
expr.getTarget() instanceof AllocationFunction and
366-
not exists(NewOrNewArrayExpr newExpr |
367-
// we synthesize allocator calls for `new` and `new[]`, so don't add instructions to
368-
// the existing allocator call when it exists.
369-
newExpr.getAllocatorCall() = expr
370-
) and
371369
if exists(getChild(0))
372370
then result = getChild(0).getFirstInstruction()
373371
else result = getParent().getChildSuccessor(this)
@@ -381,23 +379,34 @@ class TranslatedSideEffects extends TranslatedElement, TTranslatedSideEffects {
381379

382380
override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
383381
tag = OnlyInstructionTag() and
384-
result = getTranslatedExpr(expr).getInstruction(CallTag())
382+
if expr instanceof NewOrNewArrayExpr
383+
then result = getTranslatedAllocatorCall(expr).getInstruction(CallTag())
384+
else result = getTranslatedExpr(expr).getInstruction(CallTag())
385385
}
386+
}
386387

387-
/**
388-
* Gets the `TranslatedFunction` containing this expression.
389-
*/
390-
final TranslatedFunction getEnclosingFunction() {
391-
result = getTranslatedFunction(expr.getEnclosingFunction())
392-
}
388+
class TranslatedCallSideEffects extends TranslatedSideEffects, TTranslatedCallSideEffects {
389+
Call expr;
393390

394-
/**
395-
* Gets the `Function` containing this expression.
396-
*/
397-
override Function getFunction() { result = expr.getEnclosingFunction() }
391+
TranslatedCallSideEffects() { this = TTranslatedCallSideEffects(expr) }
392+
393+
override string toString() { result = "(side effects for " + expr.toString() + ")" }
394+
395+
override Call getExpr() { result = expr }
396+
397+
override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) { none() }
398+
399+
override Instruction getFirstInstruction() { result = getChild(0).getFirstInstruction() }
400+
401+
override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) { none() }
402+
403+
override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
404+
tag = OnlyInstructionTag() and
405+
result = getTranslatedExpr(expr).getInstruction(CallTag())
406+
}
398407
}
399408

400-
class TranslatedStructorCallSideEffects extends TranslatedSideEffects {
409+
class TranslatedStructorCallSideEffects extends TranslatedCallSideEffects {
401410
TranslatedStructorCallSideEffects() { getParent().(TranslatedStructorCall).hasQualifier() }
402411

403412
override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType t) {

‎cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -442,11 +442,22 @@ newtype TTranslatedElement =
442442
// The declaration/initialization part of a `ConditionDeclExpr`
443443
TTranslatedConditionDecl(ConditionDeclExpr expr) { not ignoreExpr(expr) } or
444444
// The side effects of a `Call`
445-
TTranslatedSideEffects(Call expr) {
446-
exists(TTranslatedArgumentSideEffect(expr, _, _, _)) or
447-
expr instanceof ConstructorCall or
448-
expr.getTarget() instanceof AllocationFunction
449-
} or // A precise side effect of an argument to a `Call`
445+
TTranslatedCallSideEffects(Call expr) {
446+
// Exclude allocations such as `malloc` (which happen to also be function calls).
447+
// Both `TranslatedCallSideEffects` and `TranslatedAllocationSideEffects` generate
448+
// the same side effects for its children as they both extend the `TranslatedSideEffects`
449+
// class.
450+
// Note: We can separate allocation side effects and call side effects into two
451+
// translated elements as no call can be both a `ConstructorCall` and an `AllocationExpr`.
452+
not expr instanceof AllocationExpr and
453+
(
454+
exists(TTranslatedArgumentSideEffect(expr, _, _, _)) or
455+
expr instanceof ConstructorCall
456+
)
457+
} or
458+
// The side effects of an allocation, i.e. `new`, `new[]` or `malloc`
459+
TTranslatedAllocationSideEffects(AllocationExpr expr) or
460+
// A precise side effect of an argument to a `Call`
450461
TTranslatedArgumentSideEffect(Call call, Expr expr, int n, boolean isWrite) {
451462
(
452463
expr = call.getArgument(n).getFullyConverted()

‎cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1649,6 +1649,11 @@ class TranslatedAllocatorCall extends TTranslatedAllocatorCall, TranslatedDirect
16491649

16501650
final override int getNumberOfArguments() {
16511651
result = expr.getAllocatorCall().getNumberOfArguments()
1652+
or
1653+
// Make sure there's a result even when there is no allocator, as otherwise
1654+
// TranslatedCall::getChild() will not return the side effects for this call.
1655+
not exists(expr.getAllocatorCall()) and
1656+
result = 0
16521657
}
16531658

16541659
final override TranslatedExpr getArgument(int index) {

‎cpp/ql/test/library-tests/dataflow/security-taint/tainted.expected

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,3 +73,18 @@
7373
| test.cpp:100:17:100:22 | buffer | test.cpp:93:18:93:18 | s | |
7474
| test.cpp:100:17:100:22 | buffer | test.cpp:97:7:97:12 | buffer | |
7575
| test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | buffer | |
76+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:8:24:8:25 | s1 | |
77+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:11:20:11:21 | s1 | |
78+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:11:36:11:37 | s2 | |
79+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:17:106:24 | userName | |
80+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:28:106:33 | call to getenv | |
81+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:28:106:46 | (const char *)... | |
82+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:108:8:108:11 | copy | |
83+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:2:109:7 | call to strcpy | |
84+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:9:109:12 | copy | |
85+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:15:109:22 | userName | |
86+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:6:111:27 | ! ... | |
87+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:7:111:12 | call to strcmp | |
88+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:7:111:27 | (bool)... | |
89+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:14:111:17 | (const char *)... | |
90+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:14:111:17 | copy | |

‎cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,6 @@
1414
| test.cpp:100:12:100:15 | call to gets | test.cpp:100:2:100:8 | pointer | AST only |
1515
| test.cpp:100:17:100:22 | buffer | test.cpp:97:7:97:12 | buffer | AST only |
1616
| test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | array to pointer conversion | IR only |
17+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:11:20:11:21 | s1 | AST only |
18+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:108:8:108:11 | copy | AST only |
19+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:9:109:12 | copy | AST only |

‎cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,3 +57,15 @@
5757
| test.cpp:100:17:100:22 | buffer | test.cpp:93:18:93:18 | s | |
5858
| test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | array to pointer conversion | |
5959
| test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | buffer | |
60+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:8:24:8:25 | s1 | |
61+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:11:36:11:37 | s2 | |
62+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:17:106:24 | userName | |
63+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:28:106:33 | call to getenv | |
64+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:28:106:46 | (const char *)... | |
65+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:2:109:7 | call to strcpy | |
66+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:15:109:22 | userName | |
67+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:6:111:27 | ! ... | |
68+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:7:111:12 | call to strcmp | |
69+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:7:111:27 | (bool)... | |
70+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:14:111:17 | (const char *)... | |
71+
| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:14:111:17 | copy | |

‎cpp/ql/test/library-tests/dataflow/security-taint/test.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,16 @@ void test_gets()
9999

100100
pointer = gets(buffer);
101101
}
102+
103+
const char *alias_global_new;
104+
105+
void newBuffer() {
106+
const char *userName = getenv("USER_NAME");
107+
char *alias = new char[4096];
108+
char *copy = new char[4096];
109+
strcpy(copy, userName);
110+
alias_global_new = alias; // to force a Chi node on all aliased memory
111+
if (!strcmp(copy, "admin")) { // copy should be tainted
112+
isAdmin = true;
113+
}
114+
}

0 commit comments

Comments
 (0)