diff --git a/cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql b/cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql index 663f2e990b50..51d0bc71da77 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/UsingExpiredStackAddress.ql @@ -163,19 +163,46 @@ TGlobalAddress globalAddress(Instruction instr) { result = globalAddress(instr.(PointerOffsetInstruction).getLeft()) } -/** Gets a `StoreInstruction` that may be executed after executing `store`. */ -pragma[inline] -StoreInstruction getAStoreStrictlyAfter(StoreInstruction store) { - exists(IRBlock block, int index1, int index2 | - block.getInstruction(index1) = store and - block.getInstruction(index2) = result and - index2 > index1 +/** + * Gets a first `StoreInstruction` that writes to address `globalAddress` reachable + * from `block`. + */ +StoreInstruction getFirstStore(IRBlock block, TGlobalAddress globalAddress) { + 1 = getStoreRank(result, block, globalAddress) + or + not exists(getStoreRank(_, block, globalAddress)) and + result = getFirstStore(block.getASuccessor(), globalAddress) +} + +/** + * Gets the rank of `store` in block `block` (i.e., a rank of `1` means that it is the + * first `store` to write to `globalAddress`, a rank of `2` means it's the second, etc.) + */ +int getStoreRank(StoreInstruction store, IRBlock block, TGlobalAddress globalAddress) { + blockStoresToAddress(block, _, store, globalAddress) and + store = + rank[result](StoreInstruction anotherStore, int i | + blockStoresToAddress(_, i, anotherStore, globalAddress) + | + anotherStore order by i + ) +} + +/** + * Gets a next subsequent `StoreInstruction` to write to `globalAddress` + * after `store` has done so. + */ +StoreInstruction getANextStoreTo(StoreInstruction store, TGlobalAddress globalAddress) { + exists(IRBlock block, int rnk | + rnk = getStoreRank(store, block, globalAddress) and + rnk + 1 = getStoreRank(result, block, globalAddress) ) or - exists(IRBlock block1, IRBlock block2 | - store.getBlock() = block1 and - result.getBlock() = block2 and - block1.getASuccessor+() = block2 + exists(IRBlock block, int rnk, IRBlock succ | + rnk = getStoreRank(store, block, globalAddress) and + not rnk + 1 = getStoreRank(_, block, globalAddress) and + succ = block.getASuccessor() and + result = getFirstStore(succ, globalAddress) ) } @@ -192,7 +219,7 @@ predicate stackAddressEscapes( stackPointerFlowsToUse(store.getSourceValue(), vai) ) and // Ensure there's no subsequent store that overrides the global address. - not globalAddress = globalAddress(getAStoreStrictlyAfter(store).getDestinationAddress()) + not exists(getANextStoreTo(store, globalAddress)) } predicate blockStoresToAddress( diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/UsingExpiredStackAddress.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/UsingExpiredStackAddress.expected index 80d098583389..955668b4e7a0 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/UsingExpiredStackAddress.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/UsingExpiredStackAddress.expected @@ -64,6 +64,10 @@ edges | test.cpp:201:5:201:17 | EnterFunction: maybe_deref_p | test.cpp:201:5:201:17 | VariableAddress: maybe_deref_p | | test.cpp:210:3:210:9 | Call: call to escape1 | test.cpp:201:5:201:17 | EnterFunction: maybe_deref_p | | test.cpp:210:3:210:9 | Call: call to escape1 | test.cpp:201:5:201:17 | VariableAddress: maybe_deref_p | +| test.cpp:234:3:234:13 | Store: ... = ... | test.cpp:238:3:238:9 | Call: call to escape2 | +| test.cpp:238:3:238:9 | Call: call to escape2 | test.cpp:239:17:239:17 | Load: p | +| test.cpp:263:3:263:13 | Store: ... = ... | test.cpp:267:3:267:9 | Call: call to escape3 | +| test.cpp:267:3:267:9 | Call: call to escape3 | test.cpp:268:17:268:17 | Load: p | #select | test.cpp:15:16:15:16 | Load: p | test.cpp:10:3:10:13 | Store: ... = ... | 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 | | test.cpp:24:16:24:16 | Load: p | test.cpp:10:3:10:13 | Store: ... = ... | test.cpp:24:16:24: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 | @@ -90,3 +94,5 @@ edges | test.cpp:180:14:180:19 | Load: * ... | test.cpp:154:3:154:22 | Store: ... = ... | 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 | | test.cpp:181:13:181:20 | Load: access to array | test.cpp:155:3:155:21 | Store: ... = ... | 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 | | test.cpp:182:14:182:19 | Load: * ... | test.cpp:156:3:156:25 | Store: ... = ... | 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 | +| test.cpp:239:17:239:17 | Load: p | test.cpp:234:3:234:13 | Store: ... = ... | test.cpp:239:17:239:17 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:232:7:232:7 | x | x | test.cpp:234:3:234:13 | Store: ... = ... | here | +| test.cpp:268:17:268:17 | Load: p | test.cpp:263:3:263:13 | Store: ... = ... | test.cpp:268:17:268:17 | Load: p | Stack variable $@ escapes $@ and is used after it has expired. | test.cpp:260:7:260:7 | x | x | test.cpp:263:3:263:13 | Store: ... = ... | here | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/test.cpp index 3e8a7e90b848..616305a8174d 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/UsingExpiredStackAddress/test.cpp @@ -209,4 +209,61 @@ int maybe_deref_p(bool b) { int field_indirect_maybe_bad(bool b) { escape1(); return maybe_deref_p(b); +} + +// These next tests cover subsequent stores to the same address in the same basic block. + +static struct S100 s102; + +void not_escape1() { + int x; + s102.p = &x; + s102.p = nullptr; +} + +void calls_not_escape1() { + not_escape1(); + int x = *s102.p; // GOOD +} + +static struct S100 s103; + +void escape2() { + int x; + s103.p = nullptr; + s103.p = &x; +} + +void calls_escape2() { + escape2(); + int x = *s103.p; // BAD +} + +bool unknown(); +static struct S100 s104; + +void not_escape2() { + int x; + s104.p = &x; + if(unknown()) { } + s104.p = nullptr; +} + +void calls_not_escape2() { + not_escape2(); + int x = *s104.p; // GOOD +} + +static struct S100 s105; + +void escape3() { + int x; + s105.p = nullptr; + if(unknown()) { } + s105.p = &x; +} + +void calls_escape3() { + escape3(); + int x = *s105.p; // BAD } \ No newline at end of file