C++: Use the 'StoreInstruction' instead of the 'ReturnValueInstruction' when detecting return expressions.

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll

@@ -361,9 +361,30 @@ module GuardsInput implements SharedGuards::InputSig<Cpp::Location, Instruction,
 
     /** Gets an expression returned from this function. */
     GuardsInput::Expr getAReturnExpr() {
-      exists(ReturnValueInstruction ret |
-        ret.getEnclosingFunction() = this and
-        result = ret.getReturnValue()
+      exists(StoreInstruction store |
+        // We use the `Store` instruction that writes the return value instead of the
+        // `ReturnValue` instruction since the `ReturnValue` instruction is not always
+        // dominated by certain guards. For example:
+        // ```
+        // if(b) {
+        //   return true;
+        // } else {
+        //   return false;
+        // }
+        // ```
+        // this will be translated into IR like:
+        // ```
+        // if(b) {
+        //   x = true;
+        // } else {
+        //   x = false;
+        // }
+        // return x;
+        // ```
+        store.getDestinationAddress().(VariableAddressInstruction).getIRVariable() instanceof
+          IRReturnVariable and
+        store.getEnclosingFunction() = this and
+        result = store
       )
     }
   }

cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected

@@ -381,8 +381,11 @@
 | test.cpp:372:10:372:13 | flag | false | test.cpp:372:35:372:49 | FAILURE |
 | test.cpp:372:10:372:13 | flag | true | test.cpp:372:17:372:31 | SUCCESS |
 | test.cpp:375:25:375:25 | p | not null | test.cpp:376:24:377:7 | { ... } |
+| test.cpp:375:25:375:25 | p | not null | test.cpp:382:24:383:7 | { ... } |
 | test.cpp:375:25:375:25 | p | null | test.cpp:378:10:379:7 | { ... } |
+| test.cpp:375:25:375:25 | p | null | test.cpp:384:10:385:7 | { ... } |
 | test.cpp:375:33:375:33 | i | not null | test.cpp:390:10:391:7 | { ... } |
+| test.cpp:375:42:375:42 | s | not null | test.cpp:396:10:397:7 | { ... } |
 | test.cpp:375:50:375:50 | b | false | test.cpp:404:5:406:12 | case ...: |
 | test.cpp:375:50:375:50 | b | true | test.cpp:401:5:403:12 | case ...: |
 | test.cpp:376:7:376:18 | call to testNotNull1 | false | test.cpp:378:10:379:7 | { ... } |
@@ -391,6 +394,8 @@
 | test.cpp:376:20:376:20 | p | null | test.cpp:378:10:379:7 | { ... } |
 | test.cpp:382:7:382:18 | call to testNotNull2 | false | test.cpp:384:10:385:7 | { ... } |
 | test.cpp:382:7:382:18 | call to testNotNull2 | true | test.cpp:382:24:383:7 | { ... } |
+| test.cpp:382:20:382:20 | p | not null | test.cpp:382:24:383:7 | { ... } |
+| test.cpp:382:20:382:20 | p | null | test.cpp:384:10:385:7 | { ... } |
 | test.cpp:388:7:388:29 | ... == ... | false | test.cpp:390:10:391:7 | { ... } |
 | test.cpp:388:7:388:29 | ... == ... | true | test.cpp:388:32:389:7 | { ... } |
 | test.cpp:388:12:388:26 | call to getNumOrDefault | 0 | test.cpp:388:32:389:7 | { ... } |
@@ -400,6 +405,8 @@
 | test.cpp:394:7:394:47 | ... == ... | true | test.cpp:394:50:395:7 | { ... } |
 | test.cpp:394:15:394:34 | call to returnAIfNoneAreNull | 0 | test.cpp:394:50:395:7 | { ... } |
 | test.cpp:394:15:394:34 | call to returnAIfNoneAreNull | not 0 | test.cpp:396:10:397:7 | { ... } |
+| test.cpp:394:36:394:36 | s | not null | test.cpp:396:10:397:7 | { ... } |
+| test.cpp:394:39:394:46 | suffix | not null | test.cpp:396:10:397:7 | { ... } |
 | test.cpp:400:11:400:25 | call to testEnumWrapper | 1 | test.cpp:401:5:403:12 | case ...: |
 | test.cpp:400:11:400:25 | call to testEnumWrapper | 2 | test.cpp:404:5:406:12 | case ...: |
 | test.cpp:400:27:400:27 | b | false | test.cpp:404:5:406:12 | case ...: |

cpp/ql/test/library-tests/controlflow/guards/GuardsInline.expected

@@ -9,7 +9,9 @@
 | test.cpp:379:5:379:7 | Call: call to chk | 'call to testNotNull1:false' |
 | test.cpp:379:5:379:7 | Call: call to chk | p:null |
 | test.cpp:383:5:383:7 | Call: call to chk | 'call to testNotNull2:true' |
+| test.cpp:383:5:383:7 | Call: call to chk | 'p:not null' |
 | test.cpp:385:5:385:7 | Call: call to chk | 'call to testNotNull2:false' |
+| test.cpp:385:5:385:7 | Call: call to chk | p:null |
 | test.cpp:389:5:389:7 | Call: call to chk | '0 == call to getNumOrDefault:true' |
 | test.cpp:389:5:389:7 | Call: call to chk | 'call to getNumOrDefault:0' |
 | test.cpp:391:5:391:7 | Call: call to chk | '0 == call to getNumOrDefault:false' |
@@ -19,6 +21,8 @@
 | test.cpp:395:5:395:7 | Call: call to chk | 'call to returnAIfNoneAreNull:0' |
 | test.cpp:397:5:397:7 | Call: call to chk | '0 == call to returnAIfNoneAreNull:false' |
 | test.cpp:397:5:397:7 | Call: call to chk | 'call to returnAIfNoneAreNull:not 0' |
+| test.cpp:397:5:397:7 | Call: call to chk | 's:not null' |
+| test.cpp:397:5:397:7 | Call: call to chk | 'suffix:not null' |
 | test.cpp:402:7:402:9 | Call: call to chk | 'call to testEnumWrapper:1' |
 | test.cpp:402:7:402:9 | Call: call to chk | 'call to testEnumWrapper=SUCCESS:true' |
 | test.cpp:402:7:402:9 | Call: call to chk | b:true |

cpp/ql/test/library-tests/controlflow/guards/test.cpp

@@ -380,9 +380,9 @@ void testWrappers(void* p, int* i, char* s, bool b) {
   }
 
   if (testNotNull2(p)) {
-    chk(); // $ guarded='call to testNotNull2:true' MISSING: guarded='p:not null'
+    chk(); // $ guarded='call to testNotNull2:true' guarded='p:not null'
   } else {
-    chk(); // $ guarded='call to testNotNull2:false'
+    chk(); // $ guarded='call to testNotNull2:false' guarded=p:null
   }
 
   if (0 == getNumOrDefault(i)) {
@@ -394,7 +394,7 @@ void testWrappers(void* p, int* i, char* s, bool b) {
   if ('\0' == returnAIfNoneAreNull(s, "suffix")) {
     chk(); // $ guarded='0 == call to returnAIfNoneAreNull:true' guarded='call to returnAIfNoneAreNull:0'
   } else {
-    chk(); // $ guarded='0 == call to returnAIfNoneAreNull:false' guarded='call to returnAIfNoneAreNull:not 0' MISSING: guarded='s:not null'
+    chk(); // $ guarded='0 == call to returnAIfNoneAreNull:false' guarded='call to returnAIfNoneAreNull:not 0' guarded='s:not null' guarded='suffix:not null'
   }
 
   switch (testEnumWrapper(b)) {

cpp/ql/test/library-tests/dataflow/dataflow-tests/BarrierGuard.cpp

@@ -139,7 +139,7 @@ void test_guarded_wrapper() {
   int x = source();
 
   if(guarded_wrapper(x)) {
-    sink(x); // $ SPURIOUS: ast,ir
+    sink(x); // $ SPURIOUS: ast
   } else {
     sink(x); // $ ast,ir
   }

cpp/ql/test/library-tests/dataflow/dataflow-tests/test-source-sink.expected

@@ -158,7 +158,6 @@ irFlow
 | BarrierGuard.cpp:49:10:49:15 | call to source | BarrierGuard.cpp:55:13:55:13 | x |
 | BarrierGuard.cpp:60:11:60:16 | call to source | BarrierGuard.cpp:64:14:64:14 | x |
 | BarrierGuard.cpp:60:11:60:16 | call to source | BarrierGuard.cpp:66:14:66:14 | x |
-| BarrierGuard.cpp:139:11:139:16 | call to source | BarrierGuard.cpp:142:10:142:10 | x |
 | BarrierGuard.cpp:139:11:139:16 | call to source | BarrierGuard.cpp:144:10:144:10 | x |
 | acrossLinkTargets.cpp:19:27:19:32 | call to source | acrossLinkTargets.cpp:12:8:12:8 | x |
 | clang.cpp:12:9:12:20 | sourceArray1 | clang.cpp:18:8:18:19 | sourceArray1 |