From 4efbc6ea9beebfe21544fee894c2e0dcf1cdc11f Mon Sep 17 00:00:00 2001 From: Jeroen Ketema Date: Mon, 16 Feb 2026 19:04:23 +0100 Subject: [PATCH] C++: Handle `allowInterproceduralFlow` correctly in case of recursive functions --- .../semmle/code/cpp/ir/dataflow/MustFlow.qll | 27 +++++-------------- .../ReturnStackAllocatedMemory.expected | 8 ------ .../ReturnStackAllocatedMemory/test.cpp | 2 +- 3 files changed, 8 insertions(+), 29 deletions(-) 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 5a0f34c6dc1..a8adb16849c 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll @@ -87,24 +87,12 @@ module MustFlow { ) } - /** - * 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 IRFunction getEnclosingCallable(Instruction n) { - pragma[only_bind_into](result) = pragma[only_bind_out](n).getEnclosingIRFunction() - } - /** Holds if `nodeFrom` flows to `nodeTo`. */ private predicate step(Instruction nodeFrom, Instruction nodeTo) { - Cached::step(pragma[only_bind_into](nodeFrom), pragma[only_bind_into](nodeTo)) and - ( - allowInterproceduralFlow() - or - getEnclosingCallable(nodeFrom) = getEnclosingCallable(nodeTo) - ) + Cached::localStep(pragma[only_bind_into](nodeFrom), pragma[only_bind_into](nodeTo)) + or + allowInterproceduralFlow() and + Cached::flowThroughCallable(pragma[only_bind_into](nodeFrom), pragma[only_bind_into](nodeTo)) or isAdditionalFlowStep(nodeFrom.getAUse(), nodeTo) } @@ -230,7 +218,8 @@ module MustFlow { * Holds if `argument` is an argument (or argument indirection) to a call, and * `parameter` is the corresponding initialization instruction in the call target. */ - private predicate flowThroughCallable(Instruction argument, Instruction parameter) { + cached + predicate flowThroughCallable(Instruction argument, Instruction parameter) { // Flow from an argument to a parameter exists(CallInstruction call, InitializeParameterInstruction init | init = parameter | isPositionalArgumentInitParam(call, argument, init, call.getStaticCallTarget()) @@ -279,13 +268,11 @@ module MustFlow { } cached - predicate step(Instruction nodeFrom, Instruction nodeTo) { + predicate localStep(Instruction nodeFrom, Instruction nodeTo) { exists(Operand mid | instructionToOperandStep(nodeFrom, mid) and operandToInstructionStep(mid, nodeTo) ) - or - flowThroughCallable(nodeFrom, nodeTo) } } } 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 23b23dc4a3b..6aa457b1e8a 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 @@ -48,9 +48,6 @@ edges | 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 *)... | -| test.cpp:253:17:253:17 | p | test.cpp:256:10:256:10 | p | -| test.cpp:255:19:255:20 | & ... | test.cpp:253:17:253:17 | p | -| test.cpp:255:20:255:20 | x | test.cpp:255:19:255:20 | & ... | nodes | test.cpp:17:9:17:11 | & ... | semmle.label | & ... | | test.cpp:17:10:17:11 | mc | semmle.label | mc | @@ -117,10 +114,6 @@ nodes | 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 | -| test.cpp:253:17:253:17 | p | semmle.label | p | -| test.cpp:255:19:255:20 | & ... | semmle.label | & ... | -| test.cpp:255:20:255:20 | x | semmle.label | x | -| test.cpp:256:10:256:10 | p | semmle.label | p | #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 | @@ -138,4 +131,3 @@ nodes | 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 | -| test.cpp:256:10:256:10 | Load: p | test.cpp:255:20:255:20 | x | test.cpp:256:10:256:10 | p | May return stack-allocated memory from $@. | test.cpp:255:20:255:20 | x | x | diff --git a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp index ab1a626a4b0..abde10eb6e7 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Memory Management/ReturnStackAllocatedMemory/test.cpp @@ -252,6 +252,6 @@ void* test_strndupa(const char* s, size_t size) { int* f_rec(int *p, bool b) { int x; - int* px = f_rec(&x, b); // GOOD [FALSE POSITIVE] + int* px = f_rec(&x, b); // GOOD return p; }