Skip to content

Commit 7f2d4e4

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

8 files changed

Lines changed: 529 additions & 0 deletions

File tree

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

‎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)