[analyzer] Check for dead/impossible status checks

[trixirt]

Summary

The c++ 'new' function has several variants.
Only the no-throw versions of these return a nullptr on failure.
Error handling based on tha nullptr check for the throw version
is dead code.

ex/

char *n = new char[x];
if (n == nullptr) // this is dead/impossible code

do_something();

[george.karpenkov]

@trixirt I wrote a bunch of comments inline, but then after looking at the test cases,
.... this would not work.

This is a dataflow, all-paths check (the variable can NEVER be a nullptr), yet symbolic execution runs on A given paths.

Currently in the analyzer there's no way to write dataflow checks using the checker API.

HOWEVER, in your case all comparisons to NULL are constants, so this should be just implemented as an AST matcher (a good introduction is https://eli.thegreenplace.net/2014/07/29/ast-matchers-and-clang-refactoring-tools).
The good thing it probably could be expressed in just a few lines of code.

[trixirt]

Convert to AST Visitor

Remove Ctx parameter from shouldNullCheckAllocation

Add a shouldNullCheckAllocation ASTMatcher

[trixirt]

cleanup of last patch

[george.karpenkov]

Getting there! Sorry for the delay. Would it be possible to rewrite the remaining imperative code using matchers?
👍 on defining your own matcher. However, that would probably need to go in a separate PR (with tests), @alexfh and @aaron.ballman can help with the review.

lib/StaticAnalyzer/Checkers/DeadStatusChecker.cpp

34

Could this be rewritten as a matcher instead? Surely there must be one comparing the literal to null?

[alexfh]

lib/StaticAnalyzer/Checkers/DeadStatusChecker.cpp

34

For this particular part a matchers-based solution might be a bit wordy:

binaryOperator(anyOf(
  allOf(hasLHS(ignoringParenCasts(nullPointerConstant())), hasRHS(expr().bind("other"))),
  allOf(hasRHS(ignoringParenCasts(nullPointerConstant())), hasLHS(expr().bind("other")))))

But maybe it's time to add a helper matcher similar to hasEitherOperand, for example:

inline internal::Matcher<BinaryOperator> hasOperands(
    const internal::Matcher<Expr> &One, const internal::Matcher<Expr> &Another) {
  return anyOf(allOf(hasLHS(One), hasRHS(Another)), allOf(hasRHS(One), hasLHS(Another)));
}

This can be done locally, but I wouldn't object adding it to ASTMatchers.h, since it could already be used a few times in the existing code (at least in clang-tidy/bugprone/IncorrectRoundingsCheck.cpp and clang-tidy/misc/RedundantExpressionCheck.cpp).

[NoQ]

Thanks for adding me!

Hmm, i think a matcher-based solution would be pretty limited. This is definitely your typical all-path data-flow problem because you want to make sure that you're looking at the last assignment to the variable.

For example:

int *m = new int;
m = nullptr;
if (m == nullptr) { ... } // no-warning

but

int *m = nullptr;
m = new int;
if (m == nullptr) { ... } // expected-warning{{}}

You might be able to fix false positives by adding a condition that the variable is not re-assigned within the function (with the help of assignment operator or due to taking a non-constant reference to it, etc). But you'll end up with a checker that finds a lot less bugs than a full-featured data flow analysis could have found. There's a canonical implementation of "the variable is not modified" check via ASTMatchers in LoopUnrolling.cpp.

If you'll ever want to find a full-featured data flow check, i'm not sure but you might be able to re-use LiveVariables analysis (the non-Relaxed one) to find the last assignment, and in this case you won't have to write data flow analysis yourself. DeadStores checker has an example of that.

[NoQ]

For now the tests that i proposed may in fact accidentally work because as far as i understand you're updating the "variable contains a value returned by operator new" flag when you encounter assignment operators. The real problems will arise when the order of the assignments in the AST doesn't correspond to the order of assignments during program execution.

So one more test i'd like to have is:

int *m = new int;
while (true) {
  if (m == nullptr) { ... } // no-warning
  m = nullptr;
}

[george.karpenkov]

@NoQ had some valid comments. I think this could be still useful without dataflow if we negate matches containing escaping and assignments.
Though of course this check really begs for a dataflow engine.

[trixirt]

Sorry for the delay, i will have something (hopefully) this weekend.

include/clang/StaticAnalyzer/Checkers/Checkers.td

 // Deadcode checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = DeadCode in {
 
 def DeadStoresChecker : Checker<"DeadStores">,
   HelpText<"Check for values stored to variables that are never read afterwards">,
   DescFile<"DeadStoresChecker.cpp">;
+
+def DeadStatusChecker : Checker<"DeadStatus">,
+  HelpText<"Check for impossible status">,
+  DescFile<"DeadStatusChecker.cpp">;
+
 } // end DeadCode
 
 let ParentPackage = DeadCodeAlpha in {
 
 def UnreachableCodeChecker : Checker<"UnreachableCode">,
   HelpText<"Check unreachable code">,
   DescFile<"UnreachableCodeChecker.cpp">;
 

lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   CheckSecuritySyntaxOnly.cpp
   CheckSizeofPointer.cpp
   CheckerDocumentation.cpp
   ChrootChecker.cpp
   ClangCheckers.cpp
   CloneChecker.cpp
   ConversionChecker.cpp
   CXXSelfAssignmentChecker.cpp
+  DeadStatusChecker.cpp
   DeadStoresChecker.cpp
   DebugCheckers.cpp
   DeleteWithNonVirtualDtorChecker.cpp
   DereferenceChecker.cpp
   DirectIvarAssignment.cpp
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp

lib/StaticAnalyzer/Checkers/DeadStatusChecker.cpp

+//==- DeadStatusChecker.cpp - Check for impossible status -*- C++ -*-==//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines DeadStatus, a checker that looks for impossible
+//  status checks.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class DeadStatusChecker : public Checker<check::PostStmt<CXXNewExpr>,
+                                         check::PreStmt<BinaryOperator>> {
+  std::unique_ptr<BugType> NullptrCheckBug;
+  void reportNullptrCheck(SymbolRef Sym, const BinaryOperator *B,
+                          CheckerContext &C) const;
+  bool isNullptr(const Expr *E, CheckerContext &C) const;
+
+public:
+  DeadStatusChecker();
+  void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const;
+  void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
+};
+} // namespace
+
+REGISTER_SET_WITH_PROGRAMSTATE(NewThrowSymbols, SymbolRef)
+
+void DeadStatusChecker::reportNullptrCheck(SymbolRef Sym,
+                                           const BinaryOperator *B,
+                                           CheckerContext &C) const {
+  ExplodedNode *N = C.generateErrorNode();
+  if (!N)
+    return;
+
+  auto report = llvm::make_unique<BugReport>(
+      *NullptrCheckBug, "This variable can never be a nullptr", N);
+  report->addRange(B->getSourceRange());
+  C.emitReport(std::move(report));
+}
+
+bool DeadStatusChecker::isNullptr(const Expr *E, CheckerContext &C) const {
+  bool ret = false;
+  if (E) {
+    E = E->IgnoreParenImpCasts();
+    SVal S = C.getSVal(E);
+    if (S.getAs<DefinedOrUnknownSVal>()) {
+      DefinedOrUnknownSVal V = S.castAs<DefinedOrUnknownSVal>();
+      if (V.isZeroConstant())
+        ret = true;
+    }
+  }
+  return ret;
+}
+
+DeadStatusChecker::DeadStatusChecker() {
+  NullptrCheckBug.reset(
+      new BugType(this, "Dead nullptr check", "C++ API Error"));
+}
+
+void DeadStatusChecker::checkPreStmt(const BinaryOperator *B,
+                                     CheckerContext &C) const {
+  if (!B->isEqualityOp())
+    return;
+
+  const Expr *lhs = B->getLHS()->IgnoreParenImpCasts();
+  const Expr *rhs = B->getRHS()->IgnoreParenImpCasts();
+  const DeclRefExpr *DRE = nullptr;
+  SVal V;
+
+  if (isNullptr(B->getLHS(), C)) {
+    if (rhs && isa<DeclRefExpr>(rhs)) {
+      DRE = dyn_cast<DeclRefExpr>(rhs);
+      V = C.getSVal(B->getRHS());
+    }
+  } else if (isNullptr(B->getRHS(), C)) {
+    if (lhs && isa<DeclRefExpr>(lhs)) {
+      DRE = dyn_cast<DeclRefExpr>(lhs);
+      V = C.getSVal(B->getLHS());
+    }
+  } else {
+    return;
+  }
+  if (DRE != nullptr) {
+    SymbolRef trySym = V.getAsLocSymbol();
+    if (trySym && C.getState()->contains<NewThrowSymbols>(trySym))
+      reportNullptrCheck(trySym, B, C);
+  }
+}
+
+void DeadStatusChecker::checkPostStmt(const CXXNewExpr *NE,
+                                      CheckerContext &C) const {
+  ASTContext &Ctx = C.getASTContext();
+  if (!NE->shouldNullCheckAllocation(Ctx)) {
+    ProgramStateRef State = C.getState();
+    SVal V = C.getSVal(NE);
+    SymbolRef Sym = V.getAsLocSymbol();
+    State = State->add<NewThrowSymbols>(Sym);
+    C.addTransition(State);
+  }
+}
+
+void ento::registerDeadStatusChecker(CheckerManager &mgr) {
+  mgr.registerChecker<DeadStatusChecker>();
+}

test/Analysis/dead-status-new-nullptr.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=deadcode.DeadStatus -verify %s
+
+void *t1(unsigned n, bool *fail);
+void *t1(unsigned n, bool *fail) {
+  char *m = new char[n];
+  if (nullptr == m) // expected-warning{{This variable can never be a nullptr}}
+    *fail = true;
+  else
+    *fail = false;
+  return m;
+}
+
+void *t2(unsigned n, bool *fail);
+void *t2(unsigned n, bool *fail) {
+  char *m = new char[n];
+  if (m == nullptr) // expected-warning{{This variable can never be a nullptr}}
+    *fail = true;
+  else
+    *fail = false;
+  return m;
+}
+
+// a variant of nullptr
+void *t3(unsigned n, bool *fail);
+void *t3(unsigned n, bool *fail) {
+  char *m = new char[n];
+  if (m == 0) // expected-warning{{This variable can never be a nullptr}}
+    *fail = true;
+  else
+    *fail = false;
+  return m;
+}
+
+// a variant of nullptr
+#define NULL __null
+void *t4(unsigned n, bool *fail);
+void *t4(unsigned n, bool *fail) {
+  char *m = new char[n];
+  if (m == NULL) // expected-warning{{This variable can never be a nullptr}}
+    *fail = true;
+  else
+    *fail = false;
+  return m;
+}