|
| 1 | +/** |
| 2 | + * @name Redundant null check due to previous dereference |
| 3 | + * @description Checking a pointer for nullness after dereferencing it is |
| 4 | + * likely to be a sign that either the check can be removed, or |
| 5 | + * it should be moved before the dereference. |
| 6 | + * @kind problem |
| 7 | + * @problem.severity error |
| 8 | + * @id cpp/redundant-null-check-simple |
| 9 | + * @tags reliability |
| 10 | + * correctness |
| 11 | + * external/cwe/cwe-476 |
| 12 | + */ |
| 13 | + |
| 14 | +/* |
| 15 | + * Note: this query is not assigned a precision yet because we don't want it on |
| 16 | + * LGTM until its performance is well understood. It's also lacking qhelp. |
| 17 | + */ |
| 18 | + |
| 19 | +import semmle.code.cpp.ir.IR |
| 20 | + |
| 21 | +class NullInstruction extends ConstantValueInstruction { |
| 22 | + NullInstruction() { |
| 23 | + this.getValue() = "0" and |
| 24 | + this.getResultType().getUnspecifiedType() instanceof PointerType |
| 25 | + } |
| 26 | +} |
| 27 | + |
| 28 | +/** |
| 29 | + * An instruction that will never have slicing on its result. |
| 30 | + */ |
| 31 | +class SingleValuedInstruction extends Instruction { |
| 32 | + SingleValuedInstruction() { |
| 33 | + this.getResultMemoryAccess() instanceof IndirectMemoryAccess |
| 34 | + or |
| 35 | + not this.hasMemoryResult() |
| 36 | + } |
| 37 | +} |
| 38 | + |
| 39 | +predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) { |
| 40 | + bool = any(CompareInstruction cmp | |
| 41 | + exists(NullInstruction null | |
| 42 | + cmp.getLeft() = null and cmp.getRight() = checked |
| 43 | + or |
| 44 | + cmp.getLeft() = checked and cmp.getRight() = null |
| 45 | + | |
| 46 | + cmp instanceof CompareEQInstruction |
| 47 | + or |
| 48 | + cmp instanceof CompareNEInstruction |
| 49 | + ) |
| 50 | + ) |
| 51 | + or |
| 52 | + bool = any(ConvertInstruction convert | |
| 53 | + checked = convert.getUnary() and |
| 54 | + convert.getResultType() instanceof BoolType and |
| 55 | + checked.getResultType() instanceof PointerType |
| 56 | + ) |
| 57 | +} |
| 58 | + |
| 59 | +from LoadInstruction checked, LoadInstruction deref, SingleValuedInstruction sourceValue |
| 60 | +where |
| 61 | + explicitNullTestOfInstruction(checked, _) and |
| 62 | + sourceValue = deref.getSourceAddress().(LoadInstruction).getSourceValue() and |
| 63 | + sourceValue = checked.getSourceValue() and |
| 64 | + // This also holds if the blocks are equal, meaning that the check could come |
| 65 | + // before the deref. That's still not okay because when they're in the same |
| 66 | + // basic block then the deref is unavoidable even if the check concluded that |
| 67 | + // the pointer was null. To follow this idea to its full generality, we |
| 68 | + // should also give an alert when `check` post-dominates `deref`. |
| 69 | + deref.getBlock().dominates(checked.getBlock()) and |
| 70 | + not checked.getAST().isInMacroExpansion() |
| 71 | +select checked, "This null check is redundant because the value is $@ in any case", deref, |
| 72 | + "dereferenced here" |
0 commit comments