diff --git a/cpp/config/suites/c/experimental b/cpp/config/suites/c/experimental new file mode 100644 index 000000000000..1842d710c6e1 --- /dev/null +++ b/cpp/config/suites/c/experimental @@ -0,0 +1 @@ ++ semmlecode-cpp-queries/Likely Bugs/RedundantNullCheckSimple.ql: /Correctness/Common Errors diff --git a/cpp/config/suites/cpp/experimental b/cpp/config/suites/cpp/experimental new file mode 100644 index 000000000000..1842d710c6e1 --- /dev/null +++ b/cpp/config/suites/cpp/experimental @@ -0,0 +1 @@ ++ semmlecode-cpp-queries/Likely Bugs/RedundantNullCheckSimple.ql: /Correctness/Common Errors diff --git a/cpp/ql/src/Likely Bugs/RedundantNullCheckSimple.ql b/cpp/ql/src/Likely Bugs/RedundantNullCheckSimple.ql new file mode 100644 index 000000000000..1f96bc691d05 --- /dev/null +++ b/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" diff --git a/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp new file mode 100644 index 000000000000..57173a06cc8d --- /dev/null +++ b/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; + } +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected new file mode 100644 index 000000000000..0fa4471ebee8 --- /dev/null +++ b/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 | diff --git a/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.qlref b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.qlref new file mode 100644 index 000000000000..2223e47c30d2 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.qlref @@ -0,0 +1 @@ +Likely Bugs/RedundantNullCheckSimple.ql