[analyzer] Be more careful with destructors of non-regions.

[NoQ]

Summary

It turns out that we crash all over the place when we try to evaluate destructors over concrete-int-but-not-null locations.

Add some defensive code.

[baloghadamsoftware]

Yes, it crashes! :-) I tried it because I did not believe it, but it does. Is there any real-world use-case for casting concrete integers to class instances? How did you find this crashing case?

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

1055 ↗

(On Diff #212004)

Why do we rename it from FieldVal to FieldLoc? Are we sure that this is a Loc? If so, then we should apply castAs<Loc>() and change the type to Loc as well. (Or auto since castAs<Loc>() is enough according to the LLVM coding standards.) If we are not sure, then I think it is better to leave it FieldVal.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

627 ↗

(On Diff #212004)

Could you please add some lines of comments to this code block?

[NoQ]

In D65349#1604363, @baloghadamsoftware wrote:

Is there any real-world use-case for casting concrete integers to class instances? How did you find this crashing case?

I think in original code this value was produced by doing pointer arithmetic over a null pointer. Which is kinda weird because we normally mis-model such arithmetic as resulting in a null pointer, so that to treat dereferences of such pointers as null dereferences (and abort the analysis immediately, never reaching the destructor). See also D37478.

Also it seems that this bug has just been independently reported as https://bugs.llvm.org/show_bug.cgi?id=42816.

[xazax.hun]

LG!

[NoQ]

Fxd!

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

void VisitCXXThisExpr(const CXXThisExpr *TE, ExplodedNode *Pred,
                         ExplodedNodeSet & Dst);
 
   void VisitCXXConstructExpr(const CXXConstructExpr *E, ExplodedNode *Pred,
                              ExplodedNodeSet &Dst);
 
   void VisitCXXDestructor(QualType ObjectType, const MemRegion *Dest,
                           const Stmt *S, bool IsBaseDtor,
                           ExplodedNode *Pred, ExplodedNodeSet &Dst,
-                          const EvalCallOptions &Options);
+                          EvalCallOptions &Options);
 
   void VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
                                 ExplodedNode *Pred,
                                 ExplodedNodeSet &Dst);
 
   void VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
                        ExplodedNodeSet &Dst);
 

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp

void ExprEngine::ProcessAutomaticObjDtor(const CFGAutomaticObjDtor Dtor,
 
   // FIXME: We need to run the same destructor on every element of the array.
   // This workaround will just run the first destructor (which will still
   // invalidate the entire array).
   EvalCallOptions CallOpts;
   Region = makeZeroElementRegion(state, loc::MemRegionVal(Region), varType,
                                  CallOpts.IsArrayCtorOrDtor).getAsRegion();
 
-  VisitCXXDestructor(varType, Region, Dtor.getTriggerStmt(), /*IsBase=*/ false,
-                     Pred, Dst, CallOpts);
+  VisitCXXDestructor(varType, Region, Dtor.getTriggerStmt(),
+                     /*IsBase=*/false, Pred, Dst, CallOpts);
 }
 
 void ExprEngine::ProcessDeleteDtor(const CFGDeleteDtor Dtor,
                                    ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst) {
   ProgramStateRef State = Pred->getState();
   const LocationContext *LCtx = Pred->getLocationContext();
   const CXXDeleteExpr *DE = Dtor.getDeleteExpr();
void ExprEngine::ProcessBaseDtor(const CFGBaseDtor D,
   SVal ThisVal = Pred->getState()->getSVal(ThisPtr);
 
   // Create the base object region.
   const CXXBaseSpecifier *Base = D.getBaseSpecifier();
   QualType BaseTy = Base->getType();
   SVal BaseVal = getStoreManager().evalDerivedToBase(ThisVal, BaseTy,
                                                      Base->isVirtual());
 
-  VisitCXXDestructor(BaseTy, BaseVal.castAs<loc::MemRegionVal>().getRegion(),
-                     CurDtor->getBody(), /*IsBase=*/ true, Pred, Dst, {});
+  EvalCallOptions CallOpts;
+  VisitCXXDestructor(BaseTy, BaseVal.getAsRegion(), CurDtor->getBody(),
+                     /*IsBase=*/true, Pred, Dst, CallOpts);
 }
 
 void ExprEngine::ProcessMemberDtor(const CFGMemberDtor D,
                                    ExplodedNode *Pred, ExplodedNodeSet &Dst) {
   const FieldDecl *Member = D.getFieldDecl();
   QualType T = Member->getType();
   ProgramStateRef State = Pred->getState();
   const LocationContext *LCtx = Pred->getLocationContext();
 
   const auto *CurDtor = cast<CXXDestructorDecl>(LCtx->getDecl());
-  Loc ThisVal = getSValBuilder().getCXXThis(CurDtor,
-                                            LCtx->getStackFrame());
-  SVal FieldVal =
-      State->getLValue(Member, State->getSVal(ThisVal).castAs<Loc>());
+  Loc ThisStorageLoc =
+      getSValBuilder().getCXXThis(CurDtor, LCtx->getStackFrame());
+  Loc ThisLoc = State->getSVal(ThisStorageLoc).castAs<Loc>();
+  SVal FieldVal = State->getLValue(Member, ThisLoc);
 
   // FIXME: We need to run the same destructor on every element of the array.
   // This workaround will just run the first destructor (which will still
   // invalidate the entire array).
   EvalCallOptions CallOpts;
   FieldVal = makeZeroElementRegion(State, FieldVal, T,
                                    CallOpts.IsArrayCtorOrDtor);
 
-  VisitCXXDestructor(T, FieldVal.castAs<loc::MemRegionVal>().getRegion(),
-                     CurDtor->getBody(), /*IsBase=*/false, Pred, Dst, CallOpts);
+  VisitCXXDestructor(T, FieldVal.getAsRegion(), CurDtor->getBody(),
+                     /*IsBase=*/false, Pred, Dst, CallOpts);
 }
 
 void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,
                                       ExplodedNode *Pred,
                                       ExplodedNodeSet &Dst) {
   const CXXBindTemporaryExpr *BTE = D.getBindTemporaryExpr();
   ProgramStateRef State = Pred->getState();
   const LocationContext *LC = Pred->getLocationContext();
void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,
   // bound to default parameters.
   assert(CleanDtorState.size() <= 1);
   ExplodedNode *CleanPred =
       CleanDtorState.empty() ? Pred : *CleanDtorState.begin();
 
   EvalCallOptions CallOpts;
   CallOpts.IsTemporaryCtorOrDtor = true;
   if (!MR) {
-    CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
-
     // If we have no MR, we still need to unwrap the array to avoid destroying
     // the whole array at once. Regardless, we'd eventually need to model array
     // destructors properly, element-by-element.
     while (const ArrayType *AT = getContext().getAsArrayType(T)) {
       T = AT->getElementType();
       CallOpts.IsArrayCtorOrDtor = true;
     }
   } else {

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

 }
 
 void ExprEngine::VisitCXXDestructor(QualType ObjectType,
                                     const MemRegion *Dest,
                                     const Stmt *S,
                                     bool IsBaseDtor,
                                     ExplodedNode *Pred,
                                     ExplodedNodeSet &Dst,
-                                    const EvalCallOptions &CallOpts) {
+                                    EvalCallOptions &CallOpts) {
   assert(S && "A destructor without a trigger!");
   const LocationContext *LCtx = Pred->getLocationContext();
   ProgramStateRef State = Pred->getState();
 
   const CXXRecordDecl *RecordDecl = ObjectType->getAsCXXRecordDecl();
   assert(RecordDecl && "Only CXXRecordDecls should have destructors");
   const CXXDestructorDecl *DtorDecl = RecordDecl->getDestructor();
-
   // FIXME: There should always be a Decl, otherwise the destructor call
   // shouldn't have been added to the CFG in the first place.
   if (!DtorDecl) {
     // Skip the invalid destructor. We cannot simply return because
     // it would interrupt the analysis instead.
     static SimpleProgramPointTag T("ExprEngine", "SkipInvalidDestructor");
     // FIXME: PostImplicitCall with a null decl may crash elsewhere anyway.
     PostImplicitCall PP(/*Decl=*/nullptr, S->getEndLoc(), LCtx, &T);
     NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
     Bldr.generateNode(PP, Pred->getState(), Pred);
     return;
   }
 
+  if (!Dest) {
+    // We're trying to destroy something that is not a region. This may happen
+    // for a variety of reasons (unknown target region, concrete integer instead
+    // of target region, etc.). The current code makes an attempt to recover.
+    // FIXME: We probably don't really need to recover when we're dealing
+    // with concrete integers specifically.
+    CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion = true;
+    if (const Expr *E = dyn_cast_or_null<Expr>(S)) {
+      Dest = MRMgr.getCXXTempObjectRegion(E, Pred->getLocationContext());
+    } else {
+      static SimpleProgramPointTag T("ExprEngine", "SkipInvalidDestructor");
+      NodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+      Bldr.generateSink(Pred->getLocation().withTag(&T),
+                        Pred->getState(), Pred);
+      return;
+    }
+  }
+
   CallEventManager &CEMgr = getStateManager().getCallEventManager();
   CallEventRef<CXXDestructorCall> Call =
       CEMgr.getCXXDestructorCall(DtorDecl, S, Dest, IsBaseDtor, State, LCtx);
 
   PrettyStackTraceLoc CrashInfo(getContext().getSourceManager(),
                                 Call->getSourceRange().getBegin(),
                                 "Error evaluating destructor");
 
   ExplodedNodeSet DstPreCall;
   getCheckerManager().runCheckersForPreCall(DstPreCall, Pred,
                                             *Call, *this);

cfe/trunk/test/Analysis/dtor.cpp

 void f() {
   NonTrivial nt1;
   NonTrivial nt2(nt1);
   nt1 = nt2;
   clang_analyzer_eval(__is_trivial(NonTrivial)); // expected-warning{{FALSE}}
   clang_analyzer_eval(__alignof(NonTrivial) > 0); // expected-warning{{TRUE}}
 }
 }
+
+namespace dtor_over_loc_concrete_int {
+struct A {
+  ~A() {}
+};
+
+struct B {
+  A a;
+  ~B() {}
+};
+
+struct C : A {
+  ~C() {}
+};
+
+void testB() {
+  B *b = (B *)-1;
+  b->~B(); // no-crash
+}
+
+void testC() {
+  C *c = (C *)-1;
+  c->~C(); // no-crash
+}
+
+void testAutoDtor() {
+  const A &a = *(A *)-1;
+  // no-crash
+}
+} // namespace dtor_over_loc_concrete_int