C++: Use GVN in RedundantNullCheckSimple

Author: jbj

cpp/ql/src/Likely Bugs/RedundantNullCheckSimple.ql

@@ -13,10 +13,11 @@
 
 /*
  * Note: this query is not assigned a precision yet because we don't want it on
- * LGTM until its performance is well understood. It's also lacking qhelp.
+ * LGTM until its performance is well understood.
  */
 
 import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.ValueNumbering
 
 class NullInstruction extends ConstantValueInstruction {
   NullInstruction() {
@@ -25,17 +26,6 @@ class NullInstruction extends ConstantValueInstruction {
   }
 }
 
-/**
- * An instruction that will never have slicing on its result.
- */
-class SingleValuedInstruction extends Instruction {
-  SingleValuedInstruction() {
-    this.getResultMemoryAccess() instanceof IndirectMemoryAccess
-    or
-    not this.hasMemoryResult()
-  }
-}
-
 predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) {
   bool = any(CompareInstruction cmp |
       exists(NullInstruction null |
@@ -56,17 +46,17 @@ predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) {
     )
 }
 
-predicate candidateResult(LoadInstruction checked, SingleValuedInstruction sourceValue)
+predicate candidateResult(LoadInstruction checked, ValueNumber value)
 {
   explicitNullTestOfInstruction(checked, _) and
   not checked.getAST().isInMacroExpansion() and
-  sourceValue = checked.getSourceValue()
+  value.getAnInstruction() = checked
 }
 
-from LoadInstruction checked, LoadInstruction deref, SingleValuedInstruction sourceValue
+from LoadInstruction checked, LoadInstruction deref, ValueNumber sourceValue
 where
   candidateResult(checked, sourceValue) and
-  sourceValue = deref.getSourceAddress().(LoadInstruction).getSourceValue() and
+  sourceValue.getAnInstruction() = deref.getSourceAddress() and
   // This also holds if the blocks are equal, meaning that the check could come
   // before the deref. That's still not okay because when they're in the same
   // basic block then the deref is unavoidable even if the check concluded that

cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp

@@ -69,3 +69,34 @@ int test_inverted_logic(int *p) {
     return 0;
   }
 }
+
+void test_indirect_local() {
+  int a = 0;
+  int *p = &a;
+  int **pp = &p;
+  int x;
+  x = **pp;
+  if (*pp == nullptr) { // BAD
+    return;
+  }
+}
+
+void test_field_local(bool boolvar) {
+  int a = 0;
+  struct {
+    int *p;
+  } s = { &a };
+  auto sp = &s;
+
+  if (boolvar) {
+    int x = *sp->p;
+    if (sp->p == nullptr) { // BAD
+      return;
+    }
+  } else {
+    int *x = sp->p;
+    if (sp == nullptr) { // BAD [NOT DETECTED]
+      return;
+    }
+  }
+}

cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected

@@ -1,3 +1,5 @@
 | RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:3:7:3:8 | Load: * ... | dereferenced here |
 | RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:10:11:10:12 | Load: * ... | dereferenced here |
 | RedundantNullCheckSimple.cpp:48:12:48:12 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:51:10:51:11 | Load: * ... | dereferenced here |
+| RedundantNullCheckSimple.cpp:79:7:79:9 | Load: * ... | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:78:7:78:10 | Load: * ... | dereferenced here |
+| RedundantNullCheckSimple.cpp:93:13:93:13 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:92:13:92:18 | Load: * ... | dereferenced here |