In VirtualCallChecker, handle indirect calls

[sammccall]

Summary

In VirtualCallChecker, handle indirect calls.

getDirectCallee() can be nullptr, and dyn_cast(nullptr) is UB

[sammccall]

Oops, reverted noise :(

[sammccall]

I couldn't work out how to add a test for this, advice appreciated.

Closest I could get was adding something like this to test/Analysis/virtualcall.cpp:

class F { public: F(); void foo(); };
F::F() { void (F::* ptr) = &F::foo; (this->*ptr)(); }

which crashes, but only if I add extra logging :\

[bkramer]

Your test case is fine, it crashes with assertions enabled.

[sammccall]

Add regression test.

[bkramer]

lg

[zaks.anna]

Has this been cherry-picked into the clang 4.0 release branch? If not, we should definitely do that!

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp

+#include <iostream>
 //=======- VirtualCallChecker.cpp --------------------------------*- C++ -*-==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
class WalkAST : public StmtVisitor<WalkAST> {
   /// Controlled by the "Interprocedural" analyzer-config option.
   bool IsInterprocedural = false;
 
   /// Whether the checker should only warn for calls to pure virtual functions
   /// (which is undefined behavior) or for all virtual functions (which may
   /// may result in unexpected behavior).
   bool ReportPureOnly = false;
 
-  typedef const CallExpr * WorkListUnit;
+  typedef const CallExpr *WorkListUnit;
   typedef SmallVector<WorkListUnit, 20> DFSWorkList;
 
   /// A vector representing the worklist which has a chain of CallExprs.
   DFSWorkList WList;
 
   // PreVisited : A CallExpr to this FunctionDecl is in the worklist, but the
   // body has not been visited yet.
   // PostVisited : A CallExpr to this FunctionDecl is in the worklist, and the
   // body has been visited.
-  enum Kind { NotVisited,
-              PreVisited,  /**< A CallExpr to this FunctionDecl is in the
-                                worklist, but the body has not yet been
-                                visited. */
-              PostVisited  /**< A CallExpr to this FunctionDecl is in the
-                                worklist, and the body has been visited. */
+  enum Kind {
+    NotVisited,
+    PreVisited, /**< A CallExpr to this FunctionDecl is in the
+    PostVisited /**< A CallExpr to this FunctionDecl is in the
   };
 
   /// A DenseMap that records visited states of FunctionDecls.
   llvm::DenseMap<const FunctionDecl *, Kind> VisitedFunctions;
 
   /// The CallExpr whose body is currently being visited.  This is used for
   /// generating bug reports.  This is null while visiting the body of a
   /// constructor or destructor.
public:
 
   // Stmt visitor methods.
   void VisitCallExpr(CallExpr *CE);
   void VisitCXXMemberCallExpr(CallExpr *CE);
   void VisitStmt(Stmt *S) { VisitChildren(S); }
   void VisitChildren(Stmt *S);
 
   void ReportVirtualCall(const CallExpr *CE, bool isPure);
-
 };
 } // end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // AST walking.
 //===----------------------------------------------------------------------===//
 
 void WalkAST::VisitChildren(Stmt *S) {
if (Expr *base = CME->getBase()->IgnoreImpCasts()) {
       // If the most derived class is marked final, we know that now subclass
       // can override this member.
       if (base->getBestDynamicClassType()->hasAttr<FinalAttr>())
         callIsNonVirtual = true;
     }
   }
 
   // Get the callee.
-  const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(CE->getDirectCallee());
+  const CXXMethodDecl *MD =
+      dyn_cast_or_null<CXXMethodDecl>(CE->getDirectCallee());
   if (MD && MD->isVirtual() && !callIsNonVirtual && !MD->hasAttr<FinalAttr>() &&
       !MD->getParent()->hasAttr<FinalAttr>())
     ReportVirtualCall(CE, MD->isPure());
 
   if (IsInterprocedural)
     Enqueue(CE);
 }
 
if (IsInterprocedural) {
     // Name of current visiting CallExpr.
     os << *CE->getDirectCallee();
 
     // Name of the CallExpr whose body is current being walked.
     if (visitingCallExpr)
       os << " <-- " << *visitingCallExpr->getDirectCallee();
     // Names of FunctionDecls in worklist with state PostVisited.
     for (SmallVectorImpl<const CallExpr *>::iterator I = WList.end(),
-         E = WList.begin(); I != E; --I) {
-      const FunctionDecl *FD = (*(I-1))->getDirectCallee();
+                                                     E = WList.begin();
+         I != E; --I) {
+      const FunctionDecl *FD = (*(I - 1))->getDirectCallee();
       assert(FD);
       if (VisitedFunctions[FD] == PostVisited)
         os << " <-- " << *FD;
     }
 
     os << "\n";
   }
 
   PathDiagnosticLocation CELoc =
       PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC);
   SourceRange R = CE->getCallee()->getSourceRange();
 
   os << "Call to ";
   if (isPure)
     os << "pure ";
 
   os << "virtual function during ";
 
BR.EmitBasicReport(AC->getDecl(), Checker,
                      "C++ Object Lifecycle", os.str(), CELoc, R);
 }
 
 //===----------------------------------------------------------------------===//
 // VirtualCallChecker
 //===----------------------------------------------------------------------===//
 
 namespace {
-class VirtualCallChecker : public Checker<check::ASTDecl<CXXRecordDecl> > {
+class VirtualCallChecker : public Checker<check::ASTDecl<CXXRecordDecl>> {
 public:
   DefaultBool isInterprocedural;
   DefaultBool isPureOnly;
 
-  void checkASTDecl(const CXXRecordDecl *RD, AnalysisManager& mgr,
+  void checkASTDecl(const CXXRecordDecl *RD, AnalysisManager &mgr,
                     BugReporter &BR) const {
     AnalysisDeclContext *ADC = mgr.getAnalysisDeclContext(RD);
 
     // Check the constructors.
     for (const auto *I : RD->ctors()) {
       if (!I->isCopyOrMoveConstructor())
         if (Stmt *Body = I->getBody()) {
           WalkAST walker(this, BR, ADC, I, isInterprocedural, isPureOnly);
if (CXXDestructorDecl *DD = RD->getDestructor())
         walker.Execute();
       }
   }
 };
 }
 
 void ento::registerVirtualCallChecker(CheckerManager &mgr) {
   VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>();
-  checker->isInterprocedural =
-      mgr.getAnalyzerOptions().getBooleanOption("Interprocedural", false,
-                                                checker);
+  checker->isInterprocedural = mgr.getAnalyzerOptions().getBooleanOption(
+      "Interprocedural", false, checker);
 
   checker->isPureOnly =
-      mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false,
-                                                checker);
+      mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false, checker);
 }