Skip to content

Commit ea35f56

Browse files
MathiasVPMathiasVP
committed
C++: Add a query for detecting uses of expired stack pointers that escaped through global variables.
1 parent 31d214d commit ea35f56

File tree

9 files changed

+534
-0
lines changed

9 files changed

+534
-0
lines changed

‎cpp/config/suites/c/correctness

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
+ semmlecode-cpp-queries/Critical/NewArrayDeleteMismatch.ql: /Correctness/Common Errors
3232
+ semmlecode-cpp-queries/Critical/NewDeleteArrayMismatch.ql: /Correctness/Common Errors
3333
+ semmlecode-cpp-queries/Critical/NewFreeMismatch.ql: /Correctness/Common Errors
34+
+ semmlecode-cpp-queries/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql: /Correctness/Common Errors
3435
# Use of Libraries
3536
+ semmlecode-cpp-queries/Likely Bugs/Memory Management/SuspiciousCallToMemset.ql: /Correctness/Use of Libraries
3637
+ semmlecode-cpp-queries/Likely Bugs/Memory Management/SuspiciousSizeof.ql: /Correctness/Use of Libraries

‎cpp/config/suites/cpp/correctness

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
+ semmlecode-cpp-queries/Critical/NewArrayDeleteMismatch.ql: /Correctness/Common Errors
3535
+ semmlecode-cpp-queries/Critical/NewDeleteArrayMismatch.ql: /Correctness/Common Errors
3636
+ semmlecode-cpp-queries/Critical/NewFreeMismatch.ql: /Correctness/Common Errors
37+
+ semmlecode-cpp-queries/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql: /Correctness/Common Errors
3738
# Exceptions
3839
+ semmlecode-cpp-queries/Best Practices/Exceptions/AccidentalRethrow.ql: /Correctness/Exceptions
3940
+ semmlecode-cpp-queries/Best Practices/Exceptions/CatchingByValue.ql: /Correctness/Exceptions

‎cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.cpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
static const int* xptr;
2+
3+
void localAddressEscapes() {
4+
int x = 0;
5+
xptr = &x;
6+
}
7+
8+
void example1() {
9+
localAddressEscapes();
10+
const int* x = xptr; // BAD: This pointer points to expired stack allocated memory.
11+
}
12+
13+
void localAddressDoesNotEscape() {
14+
int x = 0;
15+
xptr = &x;
16+
// ...
17+
// use `xptr`
18+
// ...
19+
xptr = nullptr;
20+
}
21+
22+
void example2() {
23+
localAddressEscapes();
24+
const int* x = xptr; // GOOD: This pointer does not point to expired memory.
25+
}

‎cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.qhelp

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
This rule finds uses of pointers that likely point to local variables in
8+
expired stack frames. Such pointers to local variables is only valid
9+
until the function returns, after which it becomes a dangling pointer.
10+
</p>
11+
12+
</overview>
13+
<recommendation>
14+
15+
<ol>
16+
17+
<li>
18+
If it is necessary to take the address of a local variable, then make
19+
sure that the address is only stored in memory that does not outlive
20+
the local variable. For example, it is safe to store the address in
21+
another local variable. Similarly, it is also safe to pass the address
22+
of a local variable to another function provided that the other
23+
function only uses it locally and does not store it in non-local
24+
memory.
25+
</li>
26+
<li>
27+
If it is necessary to store an address which will outlive the
28+
current function scope, then it should be allocated on the heap. Care
29+
should be taken to make sure that the memory is deallocated when it is
30+
no longer needed, particularly when using low-level memory management
31+
routines such as <tt>malloc</tt>/<tt>free</tt> or
32+
<tt>new</tt>/<tt>delete</tt>. Modern C++ applications often use smart
33+
pointers, such as <tt>std::shared_ptr</tt>, to reduce the chance of
34+
a memory leak.
35+
</li>
36+
</ol>
37+
38+
</recommendation>
39+
<example>
40+
41+
<sample src="UsingExpiredStackAddress.cpp" />
42+
43+
</example>
44+
<references>
45+
46+
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Dangling_pointer">Dangling pointer</a>.</li>
47+
48+
</references>
49+
</qhelp>

‎cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql

Lines changed: 244 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,244 @@
1+
/**
2+
* @name Use of expired stack-address.
3+
* @description Accessing the stack-allocated memory of a function
4+
* after it has returned can lead to memory corruption.
5+
* @kind problem
6+
* @problem.severity warning
7+
* @security-severity 9.3
8+
* @precision high
9+
* @id cpp/using-expired-stack-address
10+
* @tags reliability
11+
* security
12+
* external/cwe/cwe-825
13+
*/
14+
15+
import cpp
16+
import semmle.code.cpp.ir.ValueNumbering
17+
import semmle.code.cpp.ir.IR
18+
19+
/**
20+
* Holds if `source` is the base address of an address computation whose
21+
* result is stored in `address`.
22+
*/
23+
predicate stackPointerFlowsToUse(Instruction address, VariableAddressInstruction source) {
24+
exists(VariableAddressInstruction var |
25+
var = address and
26+
var = source and
27+
var.getASTVariable() instanceof StackVariable and
28+
// Pointer-to-member types aren't properly handled in the dbscheme.
29+
not var.getResultType() instanceof PointerToMemberType and
30+
// Rule out FPs caused by extraction errors.
31+
not any(ErrorExpr e).getEnclosingFunction() = var.getEnclosingFunction()
32+
)
33+
or
34+
stackPointerFlowsToUse(address.(CopyInstruction).getSourceValue(), source)
35+
or
36+
stackPointerFlowsToUse(address.(ConvertInstruction).getUnary(), source)
37+
or
38+
stackPointerFlowsToUse(address.(CheckedConvertOrNullInstruction).getUnary(), source)
39+
or
40+
stackPointerFlowsToUse(address.(InheritanceConversionInstruction).getUnary(), source)
41+
or
42+
stackPointerFlowsToUse(address.(FieldAddressInstruction).getObjectAddress(), source)
43+
or
44+
stackPointerFlowsToUse(address.(PointerOffsetInstruction).getLeft(), source)
45+
}
46+
47+
/**
48+
* A HashCons-like table for comparing addresses that are
49+
* computed relative to some global variable.
50+
*/
51+
newtype TGlobalAddress =
52+
TGlobalVariable(GlobalOrNamespaceVariable v) {
53+
// Pointer-to-member types aren't properly handled in the dbscheme.
54+
not v.getUnspecifiedType() instanceof PointerToMemberType
55+
} or
56+
TLoad(TGlobalAddress address) {
57+
address = globalValueNumber(any(LoadInstruction load).getSourceAddress())
58+
} or
59+
TConversion(string kind, TGlobalAddress address, Type fromType, Type toType) {
60+
kind = "unchecked" and
61+
exists(ConvertInstruction convert |
62+
uncheckedConversionTypes(convert, fromType, toType) and
63+
address = globalValueNumber(convert.getUnary())
64+
)
65+
or
66+
kind = "checked" and
67+
exists(CheckedConvertOrNullInstruction convert |
68+
checkedConversionTypes(convert, fromType, toType) and
69+
address = globalValueNumber(convert.getUnary())
70+
)
71+
or
72+
kind = "inheritance" and
73+
exists(InheritanceConversionInstruction convert |
74+
inheritanceConversionTypes(convert, fromType, toType) and
75+
address = globalValueNumber(convert.getUnary())
76+
)
77+
} or
78+
TFieldAddress(TGlobalAddress address, Field f) {
79+
exists(FieldAddressInstruction fai |
80+
fai.getField() = f and
81+
address = globalValueNumber(fai.getObjectAddress())
82+
)
83+
}
84+
85+
pragma[noinline]
86+
predicate uncheckedConversionTypes(ConvertInstruction convert, Type fromType, Type toType) {
87+
fromType = convert.getUnary().getResultType() and
88+
toType = convert.getResultType()
89+
}
90+
91+
pragma[noinline]
92+
predicate checkedConversionTypes(CheckedConvertOrNullInstruction convert, Type fromType, Type toType) {
93+
fromType = convert.getUnary().getResultType() and
94+
toType = convert.getResultType()
95+
}
96+
97+
pragma[noinline]
98+
predicate inheritanceConversionTypes(
99+
InheritanceConversionInstruction convert, Type fromType, Type toType
100+
) {
101+
fromType = convert.getUnary().getResultType() and
102+
toType = convert.getResultType()
103+
}
104+
105+
/** Gets the HashCons value of an address computed by `instr`, if any. */
106+
TGlobalAddress globalValueNumber(Instruction instr) {
107+
result = TGlobalVariable(instr.(VariableAddressInstruction).getASTVariable())
108+
or
109+
not instr instanceof LoadInstruction and
110+
result = globalValueNumber(instr.(CopyInstruction).getSourceValue())
111+
or
112+
exists(LoadInstruction load | instr = load |
113+
result = TLoad(globalValueNumber(load.getSourceAddress()))
114+
)
115+
or
116+
exists(ConvertInstruction convert, Type fromType, Type toType | instr = convert |
117+
uncheckedConversionTypes(convert, fromType, toType) and
118+
result = TConversion("unchecked", globalValueNumber(convert.getUnary()), fromType, toType)
119+
)
120+
or
121+
exists(CheckedConvertOrNullInstruction convert, Type fromType, Type toType | instr = convert |
122+
checkedConversionTypes(convert, fromType, toType) and
123+
result = TConversion("checked", globalValueNumber(convert.getUnary()), fromType, toType)
124+
)
125+
or
126+
exists(InheritanceConversionInstruction convert, Type fromType, Type toType | instr = convert |
127+
inheritanceConversionTypes(convert, fromType, toType) and
128+
result = TConversion("inheritance", globalValueNumber(convert.getUnary()), fromType, toType)
129+
)
130+
or
131+
exists(FieldAddressInstruction fai | instr = fai |
132+
result = TFieldAddress(globalValueNumber(fai.getObjectAddress()), fai.getField())
133+
)
134+
or
135+
result = globalValueNumber(instr.(PointerOffsetInstruction).getLeft())
136+
}
137+
138+
/** Gets a `StoreInstruction` that may be executed after executing `store`. */
139+
pragma[inline]
140+
StoreInstruction getAStoreStrictlyAfter(StoreInstruction store) {
141+
exists(IRBlock block, int index1, int index2 |
142+
block.getInstruction(index1) = store and
143+
block.getInstruction(index2) = result and
144+
index2 > index1
145+
)
146+
or
147+
exists(IRBlock block1, IRBlock block2 |
148+
store.getBlock() = block1 and
149+
result.getBlock() = block2 and
150+
block1.getASuccessor+() = block2
151+
)
152+
}
153+
154+
/**
155+
* Holds if `store` copies the address of `f`'s local variable `var`
156+
* into th address `globalAddress`.
157+
*/
158+
predicate stackAddressEscapes(
159+
StoreInstruction store, StackVariable var, TGlobalAddress globalAddress, Function f
160+
) {
161+
exists(VariableAddressInstruction vai |
162+
stackPointerFlowsToUse(store.getSourceValue(), vai) and
163+
globalAddress = globalValueNumber(store.getDestinationAddress()) and
164+
f = vai.getEnclosingFunction() and
165+
var = vai.getASTVariable()
166+
) and
167+
// Ensure there's no subsequent store that overrides the global address.
168+
not globalAddress = globalValueNumber(getAStoreStrictlyAfter(store).getDestinationAddress())
169+
}
170+
171+
predicate blockStoresToAddress(
172+
IRBlock block, int index, StoreInstruction store, TGlobalAddress globalAddress
173+
) {
174+
block.getInstruction(index) = store and
175+
globalAddress = globalValueNumber(store.getDestinationAddress())
176+
}
177+
178+
predicate blockLoadsFromAddress(
179+
IRBlock block, int index, LoadInstruction load, TGlobalAddress globalAddress
180+
) {
181+
block.getInstruction(index) = load and
182+
globalAddress = globalValueNumber(load.getSourceAddress())
183+
}
184+
185+
predicate globalAddressPointsToStack(
186+
StoreInstruction store, StackVariable var, CallInstruction call, IRBlock block,
187+
TGlobalAddress globalAddress, boolean isCallBlock, boolean isStoreBlock
188+
) {
189+
(
190+
if blockStoresToAddress(block, _, _, globalAddress)
191+
then isStoreBlock = true
192+
else isStoreBlock = false
193+
) and
194+
(
195+
isCallBlock = true and
196+
exists(Function f |
197+
stackAddressEscapes(store, var, globalAddress, f) and
198+
call.getStaticCallTarget() = f and
199+
call.getBlock() = block
200+
)
201+
or
202+
isCallBlock = false and
203+
exists(IRBlock mid |
204+
mid.immediatelyDominates(block) and
205+
// Only recurse if there is no store to `globalAddress` in `mid`.
206+
globalAddressPointsToStack(store, var, call, mid, globalAddress, _, false)
207+
)
208+
)
209+
}
210+
211+
from
212+
StoreInstruction store, StackVariable var, LoadInstruction load, CallInstruction call,
213+
IRBlock block, boolean isCallBlock, TGlobalAddress address, boolean isStoreBlock
214+
where
215+
globalAddressPointsToStack(store, var, call, block, address, isCallBlock, isStoreBlock) and
216+
block.getAnInstruction() = load and
217+
globalValueNumber(load.getSourceAddress()) = address and
218+
(
219+
// We know that we have a sequence:
220+
// (1) store to `address` -> (2) return from `f` -> (3) load from `address`.
221+
// But if (2) and (3) happen in the sam block we need to check the
222+
// block indices to nsure that (3) happens after (2).
223+
if isCallBlock = true
224+
then
225+
// If so, the load must happen after the call.
226+
exists(int callIndex, int loadIndex |
227+
blockLoadsFromAddress(_, loadIndex, load, _) and
228+
block.getInstruction(callIndex) = call and
229+
callIndex < loadIndex
230+
)
231+
else any()
232+
) and
233+
// If there is a store to the address we need to make sure that the load we found was
234+
// before that store (So that the load doesn't read an overwritten value).
235+
if isStoreBlock = true
236+
then
237+
exists(int storeIndex, int loadIndex |
238+
blockStoresToAddress(block, storeIndex, _, address) and
239+
block.getInstruction(loadIndex) = load and
240+
loadIndex < storeIndex
241+
)
242+
else any()
243+
select load, "Stack variable $@ escapes $@ and is used after it has expired.", var, var.toString(),
244+
store, "here"

‎cpp/ql/src/change-notes/2022-02-22-using-expired-stack-address.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: newQuery
3+
---
4+
5+
- A new query titled "Use of expired stack-address" (`cpp/using-expired-stack-address`) has been added.
6+
This query finds accesses to expired stack-allocated memory that escaped via a global variable.

‎cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/UsingExpiredStackAddress.expected

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
| test.cpp:15:16:15:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:9:7:9:7 | x | x | test.cpp:10:3:10:13 | Store: ... = ... | here |
2+
| test.cpp:58:16:58:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:51:36:51:36 | x | x | test.cpp:52:3:52:13 | Store: ... = ... | here |
3+
| test.cpp:73:16:73:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:62:7:62:7 | x | x | test.cpp:68:3:68:13 | Store: ... = ... | here |
4+
| test.cpp:98:15:98:15 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:92:8:92:8 | s | s | test.cpp:93:3:93:15 | Store: ... = ... | here |
5+
| test.cpp:111:16:111:16 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:102:7:102:7 | x | x | test.cpp:106:3:106:14 | Store: ... = ... | here |
6+
| test.cpp:161:16:161:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:136:3:136:12 | Store: ... = ... | here |
7+
| test.cpp:162:16:162:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:137:3:137:16 | Store: ... = ... | here |
8+
| test.cpp:164:16:164:17 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:139:3:139:12 | Store: ... = ... | here |
9+
| test.cpp:165:16:165:17 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:139:3:139:12 | Store: ... = ... | here |
10+
| test.cpp:166:17:166:18 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:140:3:140:16 | Store: ... = ... | here |
11+
| test.cpp:167:16:167:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:141:3:141:15 | Store: ... = ... | here |
12+
| test.cpp:168:17:168:18 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:142:3:142:19 | Store: ... = ... | here |
13+
| test.cpp:170:16:170:17 | Load: p3 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:144:3:144:12 | Store: ... = ... | here |
14+
| test.cpp:171:17:171:18 | Load: p3 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:145:3:145:16 | Store: ... = ... | here |
15+
| test.cpp:172:18:172:19 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:146:3:146:15 | Store: ... = ... | here |
16+
| test.cpp:173:18:173:19 | Load: p2 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:147:3:147:19 | Store: ... = ... | here |
17+
| test.cpp:174:18:174:19 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:142:3:142:19 | Store: ... = ... | here |
18+
| test.cpp:175:16:175:17 | Load: p1 | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:148:3:148:18 | Store: ... = ... | here |
19+
| test.cpp:177:14:177:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:151:3:151:15 | Store: ... = ... | here |
20+
| test.cpp:178:14:178:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:132:7:132:8 | b1 | b1 | test.cpp:152:3:152:19 | Store: ... = ... | here |
21+
| test.cpp:179:14:179:21 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:153:3:153:18 | Store: ... = ... | here |
22+
| test.cpp:180:14:180:19 | Load: * ... | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:133:7:133:8 | b2 | b2 | test.cpp:154:3:154:22 | Store: ... = ... | here |
23+
| test.cpp:181:13:181:20 | Load: access to array | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:155:3:155:21 | Store: ... = ... | here |
24+
| test.cpp:182:14:182:19 | Load: * ... | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:134:7:134:8 | b3 | b3 | test.cpp:156:3:156:25 | Store: ... = ... | here |

‎cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/UsingExpiredStackAddress.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Likely Bugs/Memory Management/UsingExpiredStackAddress.ql

0 commit comments

Comments
 (0)