C++: Handle allowInterproceduralFlow correctly in case of recursive functions

Author: jketema

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)
     }
   }
 }

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 |

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;
 }