Skip to content

Commit ab3cad7

Browse files
MathiasVPMathiasVP
authored
Merge pull request #8173 from MathiasVP/add-using-expired-stack-address-query
C++: Add another `CWE-825` query
2 parents 8dfc0d2 + e4af342 commit ab3cad7

File tree

9 files changed

+553
-0
lines changed

9 files changed

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