Merge pull request #894 from jbj/ir-RedundantNullCheckSimple

C++: IR query for redundant null check

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

@@ -0,0 +1,72 @@
+/**
+ * @name Redundant null check due to previous dereference
+ * @description Checking a pointer for nullness after dereferencing it is
+ *              likely to be a sign that either the check can be removed, or
+ *              it should be moved before the dereference.
+ * @kind problem
+ * @problem.severity error
+ * @id cpp/redundant-null-check-simple
+ * @tags reliability
+ *       correctness
+ *       external/cwe/cwe-476
+ */
+
+/*
+ * 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.
+ */
+
+import semmle.code.cpp.ir.IR
+
+class NullInstruction extends ConstantValueInstruction {
+  NullInstruction() {
+    this.getValue() = "0" and
+    this.getResultType().getUnspecifiedType() instanceof PointerType
+  }
+}
+
+/**
+ * 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 |
+        cmp.getLeft() = null and cmp.getRight() = checked
+        or
+        cmp.getLeft() = checked and cmp.getRight() = null
+      |
+        cmp instanceof CompareEQInstruction
+        or
+        cmp instanceof CompareNEInstruction
+      )
+    )
+  or
+  bool = any(ConvertInstruction convert |
+      checked = convert.getUnary() and
+      convert.getResultType() instanceof BoolType and
+      checked.getResultType() instanceof PointerType
+    )
+}
+
+from LoadInstruction checked, LoadInstruction deref, SingleValuedInstruction sourceValue
+where
+  explicitNullTestOfInstruction(checked, _) and
+  sourceValue = deref.getSourceAddress().(LoadInstruction).getSourceValue() and
+  sourceValue = checked.getSourceValue() 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
+  // the pointer was null. To follow this idea to its full generality, we
+  // should also give an alert when `check` post-dominates `deref`.
+  deref.getBlock().dominates(checked.getBlock()) and
+  not checked.getAST().isInMacroExpansion()
+select checked, "This null check is redundant because the value is $@ in any case", deref,
+  "dereferenced here"

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

@@ -0,0 +1,71 @@
+void test_simple_bad(int *p) {
+  int x;
+  x = *p;
+  if (p == nullptr) { // BAD
+    return;
+  }
+}
+
+void test_not_same_basic_block(int *p) {
+  int x = *p;
+  if (x > 100)
+    return;
+  if (!p) // BAD
+    return;
+}
+
+void test_indirect(int **p) {
+  int x;
+  x = **p;
+  if (*p == nullptr) { // BAD [NOT DETECTED]
+    return;
+  }
+}
+
+struct ContainsIntPtr {
+  int **intPtr;
+};
+
+bool check_curslist(ContainsIntPtr *cip) {
+  // both the deref and the null check come from the same instruction, but it's
+  // an AliasedDefinition instruction.
+  return *cip->intPtr != nullptr; // GOOD
+}
+
+void test_no_single_dominator(int *p, bool b) {
+  int x;
+  if (b) {
+    x = *p;
+  } else {
+    x = *p;
+  }
+  if (p == nullptr) { // BAD [NOT DETECTED]
+    return;
+  }
+}
+
+int test_postdominator_same_bb(int *p) {
+  int b = (p == nullptr); // BAD
+  // This dereference is a postdominator of the null check, meaning that all
+  // paths from the check to the function exit will pass through it.
+  return *p + b;
+}
+
+int test_postdominator(int *p) {
+  int b = (p == nullptr); // BAD [NOT DETECTED]
+
+  if (b) b++; // This line breaks up the basic block
+
+  // This dereference is a postdominator of the null check, meaning that all
+  // paths from the check to the function exit will pass through it.
+  return *p + b;
+}
+
+int test_inverted_logic(int *p) {
+  if (p == nullptr) { // BAD [NOT DETECTED]
+    // The check above should probably have been `!=` instead of `==`.
+    return *p;
+  } else {
+    return 0;
+  }
+}

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

@@ -0,0 +1,3 @@
+| 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 |

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

@@ -0,0 +1 @@
+Likely Bugs/RedundantNullCheckSimple.ql