Merge pull request #1593 from geoffw0/stackforreturn

CPP: Fix FP in AllocaInLoop.ql

Author: zlaski-semmle

change-notes/1.22/analysis-cpp.md

@@ -11,6 +11,7 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Call to alloca in a loop (`cpp/alloca-in-loop`) | Fewer false positive results | Fixed false positives where the stack allocation could not be reached multiple times in the loop, typically due to a `break` or `return` statement. |
 | Continue statement that does not continue (`cpp/continue-in-false-loop`) | Fewer false positive results | Analysis is now restricted to `do`-`while` loops. This query is now run and displayed by default on LGTM. |
 | Expression has no effect (`cpp/useless-expression`) | Fewer false positive results | Calls to functions with the `weak` attribute are no longer considered to be side effect free, because they could be overridden with a different implementation at link time. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | False positives involving strings that are not null-terminated have been excluded. |

cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoop.ql

@@ -322,9 +322,11 @@ class LoopWithAlloca extends Stmt {
   }
 }
 
-from LoopWithAlloca l
+from LoopWithAlloca l, AllocaCall alloc
 where
   not l.(DoStmt).getCondition().getValue() = "0" and
-  not l.isTightlyBounded()
-select l.getAnAllocaCall(), "Stack allocation is inside a $@ loop.", l,
+  not l.isTightlyBounded() and
+  alloc = l.getAnAllocaCall() and
+  alloc.getASuccessor*() = l.(Loop).getStmt()
+select alloc, "Stack allocation is inside a $@ loop.", l,
   l.toString()

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

@@ -1,6 +1,7 @@
 | AllocaInLoop1.cpp:31:18:31:23 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1.cpp:22:2:39:2 | for(...;...;...) ... | for(...;...;...) ... |
 | AllocaInLoop1.cpp:55:19:55:24 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1.cpp:45:2:64:2 | for(...;...;...) ... | for(...;...;...) ... |
 | AllocaInLoop1.cpp:80:19:80:24 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1.cpp:71:3:88:3 | for(...;...;...) ... | for(...;...;...) ... |
+| AllocaInLoop1.cpp:110:19:110:24 | call to __builtin_alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1.cpp:109:2:113:13 | do (...) ... | do (...) ... |
 | AllocaInLoop1ms.cpp:28:18:28:24 | call to _alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1ms.cpp:19:2:36:2 | for(...;...;...) ... | for(...;...;...) ... |
 | AllocaInLoop1ms.cpp:52:19:52:26 | call to _malloca | Stack allocation is inside a $@ loop. | AllocaInLoop1ms.cpp:42:2:63:2 | for(...;...;...) ... | for(...;...;...) ... |
 | AllocaInLoop1ms.cpp:79:19:79:25 | call to _alloca | Stack allocation is inside a $@ loop. | AllocaInLoop1ms.cpp:70:3:87:3 | for(...;...;...) ... | for(...;...;...) ... |

cpp/ql/test/query-tests/Likely Bugs/Memory Management/AllocaInLoop/AllocaInLoop1.cpp

@@ -88,3 +88,40 @@ void baz(const struct vtype* vec, int count) {
 		}
 	} while (0);
 }
+
+// case 4: alloca  contained in an unbounded loop, followed by break.
+void case4() {
+	char *buffer;
+
+	do {
+		buffer = (char*)alloca(1024); // GOOD
+
+		break;
+	} while (1);
+
+	delete [] buffer;
+}
+
+// case 5: alloca  contained in an unbounded loop, followed by continue.
+void case5() {
+	char *buffer;
+
+	do {
+		buffer = (char*)alloca(1024); // BAD
+
+		continue;
+	} while (1);
+
+	delete [] buffer;
+}
+
+// case 6: alloca  contained in an unbounded loop, followed by return.
+char *case6() {
+	char *buffer;
+
+	do {
+		buffer = (char*)alloca(1024); // GOOD
+
+		return buffer;
+	} while (1);
+}