Merge pull request #14794 from MathiasVP/catch-more-return-stack-allocated-memory

C++: Catch more returns of stack-allocated memory

Author: MathiasVP

cpp/ql/lib/change-notes/2023-11-15-return-stack-allocated-memory.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Returning stack-allocated memory" (`cpp/return-stack-allocated-memory`) query now also detects returning stack-allocated memory allocated by calls to `alloca`, `strdupa`, and `strndupa`.

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

@@ -27,16 +27,26 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
   ReturnStackAllocatedMemoryConfig() { this = "ReturnStackAllocatedMemoryConfig" }
 
   override predicate isSource(Instruction source) {
-    // Holds if `source` is a node that represents the use of a stack variable
-    exists(VariableAddressInstruction var, Function func |
-      var = source and
-      func = source.getEnclosingFunction() and
-      var.getAstVariable() instanceof StackVariable and
-      // Pointer-to-member types aren't properly handled in the dbscheme.
-      not var.getResultType() instanceof PointerToMemberType and
+    exists(Function func |
       // Rule out FPs caused by extraction errors.
       not any(ErrorExpr e).getEnclosingFunction() = func and
-      not intentionallyReturnsStackPointer(func)
+      not intentionallyReturnsStackPointer(func) and
+      func = source.getEnclosingFunction()
+    |
+      // `source` is an instruction that represents the use of a stack variable
+      exists(VariableAddressInstruction var |
+        var = source and
+        var.getAstVariable() instanceof StackVariable and
+        // Pointer-to-member types aren't properly handled in the dbscheme.
+        not var.getResultType() instanceof PointerToMemberType
+      )
+      or
+      // `source` is an instruction that represents the return value of a
+      // function that is known to return stack-allocated memory.
+      exists(Call call |
+        call.getTarget().hasGlobalName(["alloca", "strdupa", "strndupa", "_alloca", "_malloca"]) and
+        source.getUnconvertedResultExpression() = call
+      )
     )
   }
 
@@ -85,10 +95,10 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration {
 }
 
 from
-  MustFlowPathNode source, MustFlowPathNode sink, VariableAddressInstruction var,
+  MustFlowPathNode source, MustFlowPathNode sink, Instruction instr,
   ReturnStackAllocatedMemoryConfig conf
 where
   conf.hasFlowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and
-  source.getInstruction() = var
+  source.getInstruction() = instr
 select sink.getInstruction(), source, sink, "May return stack-allocated memory from $@.",
-  var.getAst(), var.getAst().toString()
+  instr.getAst(), instr.getAst().toString()

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

@@ -43,6 +43,11 @@ edges
 | test.cpp:189:16:189:16 | p | test.cpp:189:16:189:16 | (reference to) |
 | test.cpp:190:10:190:13 | (reference dereference) | test.cpp:190:10:190:13 | (reference to) |
 | test.cpp:190:10:190:13 | pRef | test.cpp:190:10:190:13 | (reference dereference) |
+| test.cpp:237:12:237:17 | call to alloca | test.cpp:237:12:237:17 | call to alloca |
+| test.cpp:237:12:237:17 | call to alloca | test.cpp:238:9:238:9 | p |
+| test.cpp:249:13:249:20 | call to strndupa | test.cpp:249:13:249:20 | call to strndupa |
+| test.cpp:249:13:249:20 | call to strndupa | test.cpp:250:9:250:10 | s2 |
+| test.cpp:250:9:250:10 | s2 | test.cpp:250:9:250:10 | (void *)... |
 nodes
 | test.cpp:17:9:17:11 | & ... | semmle.label | & ... |
 | test.cpp:17:10:17:11 | mc | semmle.label | mc |
@@ -101,6 +106,14 @@ nodes
 | test.cpp:190:10:190:13 | (reference dereference) | semmle.label | (reference dereference) |
 | test.cpp:190:10:190:13 | (reference to) | semmle.label | (reference to) |
 | test.cpp:190:10:190:13 | pRef | semmle.label | pRef |
+| test.cpp:237:12:237:17 | call to alloca | semmle.label | call to alloca |
+| test.cpp:237:12:237:17 | call to alloca | semmle.label | call to alloca |
+| test.cpp:238:9:238:9 | p | semmle.label | p |
+| test.cpp:245:9:245:15 | call to strdupa | semmle.label | call to strdupa |
+| test.cpp:249:13:249:20 | call to strndupa | semmle.label | call to strndupa |
+| test.cpp:249:13:249:20 | call to strndupa | semmle.label | call to strndupa |
+| test.cpp:250:9:250:10 | (void *)... | semmle.label | (void *)... |
+| test.cpp:250:9:250:10 | s2 | semmle.label | s2 |
 #select
 | test.cpp:17:9:17:11 | CopyValue: & ... | test.cpp:17:10:17:11 | mc | test.cpp:17:9:17:11 | & ... | May return stack-allocated memory from $@. | test.cpp:17:10:17:11 | mc | mc |
 | test.cpp:25:9:25:11 | Load: ptr | test.cpp:23:18:23:19 | mc | test.cpp:25:9:25:11 | ptr | May return stack-allocated memory from $@. | test.cpp:23:18:23:19 | mc | mc |
@@ -115,3 +128,6 @@ nodes
 | test.cpp:177:10:177:23 | Convert: (void *)... | test.cpp:176:25:176:34 | localArray | test.cpp:177:10:177:23 | (void *)... | May return stack-allocated memory from $@. | test.cpp:176:25:176:34 | localArray | localArray |
 | test.cpp:183:10:183:19 | CopyValue: (reference to) | test.cpp:182:21:182:27 | myLocal | test.cpp:183:10:183:19 | (reference to) | May return stack-allocated memory from $@. | test.cpp:182:21:182:27 | myLocal | myLocal |
 | test.cpp:190:10:190:13 | CopyValue: (reference to) | test.cpp:189:16:189:16 | p | test.cpp:190:10:190:13 | (reference to) | May return stack-allocated memory from $@. | test.cpp:189:16:189:16 | p | p |
+| test.cpp:238:9:238:9 | Load: p | test.cpp:237:12:237:17 | call to alloca | test.cpp:238:9:238:9 | p | May return stack-allocated memory from $@. | test.cpp:237:12:237:17 | call to alloca | call to alloca |
+| test.cpp:245:9:245:15 | Call: call to strdupa | test.cpp:245:9:245:15 | call to strdupa | test.cpp:245:9:245:15 | call to strdupa | May return stack-allocated memory from $@. | test.cpp:245:9:245:15 | call to strdupa | call to strdupa |
+| test.cpp:250:9:250:10 | Convert: (void *)... | test.cpp:249:13:249:20 | call to strndupa | test.cpp:250:9:250:10 | (void *)... | May return stack-allocated memory from $@. | test.cpp:249:13:249:20 | call to strndupa | call to strndupa |

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

@@ -229,4 +229,23 @@ int* id(int* px) {
 void f() {
   int x;
   int* px = id(&x); // GOOD
+}
+
+void *alloca(size_t);
+
+void* test_alloca() {
+	void* p = alloca(10);
+	return p; // BAD
+}
+
+char *strdupa(const char *);
+char *strndupa(const char *, size_t);
+
+char* test_strdupa(const char* s) {
+	return strdupa(s); // BAD
+}
+
+void* test_strndupa(const char* s, size_t size) {
+	char* s2 = strndupa(s, size);
+	return s2; // BAD
 }