From 9ac8d60636f6183ed56daab7b41a28d255a7a9ab Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Mon, 4 Feb 2019 10:19:20 +0100 Subject: [PATCH 1/3] C++: IR query for redundant null check This new query is not written because it's the most interesting query we could write but because it's an IR-based query whose results are easy to verify. --- .../Likely Bugs/RedundantNullCheckSimple.ql | 72 +++++++++++++++++++ .../RedundantNullCheckSimple.cpp | 33 +++++++++ .../RedundantNullCheckSimple.expected | 2 + .../RedundantNullCheckSimple.qlref | 1 + 4 files changed, 108 insertions(+) create mode 100644 cpp/ql/src/Likely Bugs/RedundantNullCheckSimple.ql create mode 100644 cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp create mode 100644 cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected create mode 100644 cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.qlref 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..d693e50ac598 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp @@ -0,0 +1,33 @@ +void test1(int *p) { + int x; + x = *p; + if (p == nullptr) { // BAD + return; + } +} + +void test2(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 +} 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..c0ece0bfa815 --- /dev/null +++ b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected @@ -0,0 +1,2 @@ +| 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 | 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 From 12084fc90439d565495d91e43afabcecbb6d03e9 Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Thu, 7 Feb 2019 10:39:29 +0100 Subject: [PATCH 2/3] C++: Add new query to new `experimental` suite This suite isn't referenced from anywhere yet, but it'll be included in a standard ODASA dist because the dist includes all files in the `c` and `cpp` directories. We can modify the nightly test jobs to include the experimental suite. --- cpp/config/suites/c/experimental | 1 + cpp/config/suites/cpp/experimental | 1 + 2 files changed, 2 insertions(+) create mode 100644 cpp/config/suites/c/experimental create mode 100644 cpp/config/suites/cpp/experimental 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 From 9f2fdbbc1daa45e2e49200902a53f0ba5eede4fc Mon Sep 17 00:00:00 2001 From: Jonas Jensen Date: Tue, 19 Feb 2019 17:12:33 +0100 Subject: [PATCH 3/3] C++: More tests for RedundantNullCheckSimple --- .../RedundantNullCheckSimple.cpp | 42 ++++++++++++++++++- .../RedundantNullCheckSimple.expected | 1 + 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp index d693e50ac598..57173a06cc8d 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp @@ -1,4 +1,4 @@ -void test1(int *p) { +void test_simple_bad(int *p) { int x; x = *p; if (p == nullptr) { // BAD @@ -6,7 +6,7 @@ void test1(int *p) { } } -void test2(int *p) { +void test_not_same_basic_block(int *p) { int x = *p; if (x > 100) return; @@ -31,3 +31,41 @@ bool check_curslist(ContainsIntPtr *cip) { // 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 index c0ece0bfa815..0fa4471ebee8 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected +++ b/cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected @@ -1,2 +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 |