diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll index 1f3ea2a4d3d..08ee06acdda 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll @@ -38,6 +38,9 @@ abstract class MustFlowConfiguration extends string { */ predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { none() } + /** Holds if this configuration allows flow from arguments to parameters. */ + predicate allowInterproceduralFlow() { any() } + /** * Holds if data must flow from `source` to `sink` for this configuration. * @@ -204,10 +207,25 @@ private module Cached { } } +/** + * Gets the enclosing callable of `n`. Unlike `n.getEnclosingCallable()`, this + * predicate ensures that joins go from `n` to the result instead of the other + * way around. + */ +pragma[inline] +private Declaration getEnclosingCallable(DataFlow::Node n) { + pragma[only_bind_into](result) = pragma[only_bind_out](n).getEnclosingCallable() +} + /** Holds if `nodeFrom` flows to `nodeTo`. */ private predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo, MustFlowConfiguration config) { exists(config) and - Cached::step(nodeFrom, nodeTo) + Cached::step(pragma[only_bind_into](nodeFrom), pragma[only_bind_into](nodeTo)) and + ( + config.allowInterproceduralFlow() + or + getEnclosingCallable(nodeFrom) = getEnclosingCallable(nodeTo) + ) or config.isAdditionalFlowStep(nodeFrom, nodeTo) } diff --git a/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql b/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql index 7eab1bd03c8..ed1d4084993 100644 --- a/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql +++ b/cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql @@ -52,6 +52,18 @@ class ReturnStackAllocatedMemoryConfig extends MustFlowConfiguration { ) } + // We disable flow into callables in this query as we'd otherwise get a result on this piece of code: + // ```cpp + // int* id(int* px) { + // return px; // this returns the local variable `x`, but it's fine as the local variable isn't declared in this scope. + // } + // void f() { + // int x; + // int* px = id(&x); + // } + // ``` + override predicate allowInterproceduralFlow() { none() } + /** * This configuration intentionally conflates addresses of fields and their object, and pointer offsets * with their base pointer as this allows us to detect cases where an object's address flows to a @@ -77,9 +89,6 @@ from ReturnStackAllocatedMemoryConfig conf where conf.hasFlowPath(pragma[only_bind_into](source), pragma[only_bind_into](sink)) and - source.getNode().asInstruction() = var and - // Only raise an alert if we're returning from the _same_ callable as the on that - // declared the stack variable. - var.getEnclosingFunction() = sink.getNode().getEnclosingCallable() + source.getNode().asInstruction() = var select sink.getNode(), source, sink, "May return stack-allocated memory from $@.", var.getAst(), var.getAst().toString() diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected index 6b8a59793a3..8f9d91fc1ad 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/ReturnStackAllocatedMemory.expected @@ -100,12 +100,6 @@ edges | test.cpp:190:10:190:13 | Unary | test.cpp:190:10:190:13 | (reference dereference) | | test.cpp:190:10:190:13 | Unary | test.cpp:190:10:190:13 | (reference to) | | test.cpp:190:10:190:13 | pRef | test.cpp:190:10:190:13 | Unary | -| test.cpp:225:14:225:15 | px | test.cpp:226:10:226:11 | Load | -| test.cpp:226:10:226:11 | Load | test.cpp:226:10:226:11 | px | -| test.cpp:226:10:226:11 | px | test.cpp:226:10:226:11 | StoreValue | -| test.cpp:231:16:231:17 | & ... | test.cpp:225:14:225:15 | px | -| test.cpp:231:17:231:17 | Unary | test.cpp:231:16:231:17 | & ... | -| test.cpp:231:17:231:17 | x | test.cpp:231:17:231:17 | Unary | nodes | test.cpp:17:9:17:11 | & ... | semmle.label | & ... | | test.cpp:17:9:17:11 | StoreValue | semmle.label | StoreValue | @@ -221,13 +215,6 @@ nodes | test.cpp:190:10:190:13 | Unary | semmle.label | Unary | | test.cpp:190:10:190:13 | Unary | semmle.label | Unary | | test.cpp:190:10:190:13 | pRef | semmle.label | pRef | -| test.cpp:225:14:225:15 | px | semmle.label | px | -| test.cpp:226:10:226:11 | Load | semmle.label | Load | -| test.cpp:226:10:226:11 | StoreValue | semmle.label | StoreValue | -| test.cpp:226:10:226:11 | px | semmle.label | px | -| test.cpp:231:16:231:17 | & ... | semmle.label | & ... | -| test.cpp:231:17:231:17 | Unary | semmle.label | Unary | -| test.cpp:231:17:231:17 | x | semmle.label | x | #select | test.cpp:17:9:17:11 | StoreValue | test.cpp:17:10:17:11 | mc | test.cpp:17:9:17:11 | StoreValue | May return stack-allocated memory from $@. | test.cpp:17:10:17:11 | mc | mc | | test.cpp:25:9:25:11 | StoreValue | test.cpp:23:18:23:19 | mc | test.cpp:25:9:25:11 | StoreValue | May return stack-allocated memory from $@. | test.cpp:23:18:23:19 | mc | mc |