Merge pull request #10321 from MathiasVP/speedup-using-expired-stack-address-2

C++: Speedup 'cpp/using-expired-stack-address' by avoiding a large ne…

Author: Dave Bartolomeo

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(

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 |

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
 }