[analyzer] Warn on use of 'self' after call to to [super dealloc].

[dcoughlin]

Summary

Referring to 'self' after a call to [super dealloc] is a use-after-free in Objective-C because NSObject's -dealloc frees the memory pointed to by self. This patch extends the ObjCSuperDeallocChecker to catch this error.

rdar://problem/6953275

[zaks.anna]

lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp

165 ↗

(On Diff #48747)

Does "has been freed" sound better?

test/Analysis/DeallocUseAfterFreeErrors.m

59 ↗

(On Diff #48747)

Would it be possible to provide a more presice error message here? Specifically, we refer to 'self' here but the reference is implicit. I guess we could say something like "accessing an instance variable after the object has been freed". Would that require extra checking to ensure that we are indeed accessing a member in checkLocation?

[dcoughlin]

lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp

165 ↗

(On Diff #48747)

Yes, changed it.

test/Analysis/DeallocUseAfterFreeErrors.m

59 ↗

(On Diff #48747)

I've updated this to be "use of instance variable '_ivar' after the instance has been freed with call to [super dealloc]".

cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp

 //===- ObjCSuperDeallocChecker.cpp - Check correct use of [super dealloc] -===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This defines ObjCSuperDeallocChecker, a builtin check that warns when
-// [super dealloc] is called twice on the same instance in MRR mode.
+// self is used after a call to [super dealloc] in MRR mode.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class ObjCSuperDeallocChecker
-    : public Checker<check::PostObjCMessage, check::PreObjCMessage> {
+    : public Checker<check::PostObjCMessage, check::PreObjCMessage,
+                     check::PreCall, check::Location> {
 
   mutable IdentifierInfo *IIdealloc, *IINSObject;
   mutable Selector SELdealloc;
 
   std::unique_ptr<BugType> DoubleSuperDeallocBugType;
 
   void initIdentifierInfoAndSelectors(ASTContext &Ctx) const;
 
   bool isSuperDeallocMessage(const ObjCMethodCall &M) const;
 
 public:
   ObjCSuperDeallocChecker();
   void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
   void checkPreObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
+
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+
+  void checkLocation(SVal l, bool isLoad, const Stmt *S,
+                     CheckerContext &C) const;
+
+private:
+
+  void diagnoseCallArguments(const CallEvent &CE, CheckerContext &C) const;
+
+  void reportUseAfterDealloc(SymbolRef Sym, StringRef Desc, const Stmt *S,
+                             CheckerContext &C) const;
 };
 
 } // End anonymous namespace.
 
 // Remember whether [super dealloc] has previously been called on the
-// a SymbolRef for the receiver.
+// SymbolRef for the receiver.
 REGISTER_SET_WITH_PROGRAMSTATE(CalledSuperDealloc, SymbolRef)
 
 class SuperDeallocBRVisitor final
     : public BugReporterVisitorImpl<SuperDeallocBRVisitor> {
 
   SymbolRef ReceiverSymbol;
   bool Satisfied;
 
public:
 
   void Profile(llvm::FoldingSetNodeID &ID) const override {
     ID.Add(ReceiverSymbol);
   }
 };
 
 void ObjCSuperDeallocChecker::checkPreObjCMessage(const ObjCMethodCall &M,
                                                   CheckerContext &C) const {
-  if (!isSuperDeallocMessage(M))
-    return;
 
   ProgramStateRef State = C.getState();
   SymbolRef ReceiverSymbol = M.getReceiverSVal().getAsSymbol();
-  assert(ReceiverSymbol && "No receiver symbol at call to [super dealloc]?");
+  if (!ReceiverSymbol) {
+    diagnoseCallArguments(M, C);
+    return;
+  }
 
   bool AlreadyCalled = State->contains<CalledSuperDealloc>(ReceiverSymbol);
-
-  // If [super dealloc] has not been called, there is nothing to do. We'll
-  // note the fact that [super dealloc] was called in checkPostObjCMessage.
   if (!AlreadyCalled)
     return;
 
-  // We have a duplicate [super dealloc] method call.
-  // This likely causes a crash, so stop exploring the
-  // path by generating a sink.
-  ExplodedNode *ErrNode = C.generateErrorNode();
-  // If we've already reached this node on another path, return.
-  if (!ErrNode)
-    return;
+  StringRef Desc;
 
-  // Generate the report.
-  std::unique_ptr<BugReport> BR(
-      new BugReport(*DoubleSuperDeallocBugType,
-                    "[super dealloc] should not be called multiple times",
-                    ErrNode));
-  BR->addRange(M.getOriginExpr()->getSourceRange());
-  BR->addVisitor(llvm::make_unique<SuperDeallocBRVisitor>(ReceiverSymbol));
-  C.emitReport(std::move(BR));
+  if (isSuperDeallocMessage(M)) {
+    Desc = "[super dealloc] should not be called multiple times";
+  } else {
+    Desc = StringRef();
+  }
+
+  reportUseAfterDealloc(ReceiverSymbol, Desc, M.getOriginExpr(), C);
 
   return;
 }
 
+void ObjCSuperDeallocChecker::checkPreCall(const CallEvent &Call,
+                                           CheckerContext &C) const {
+  diagnoseCallArguments(Call, C);
+}
+
 void ObjCSuperDeallocChecker::checkPostObjCMessage(const ObjCMethodCall &M,
                                                    CheckerContext &C) const {
   // Check for [super dealloc] method call.
   if (!isSuperDeallocMessage(M))
     return;
 
   ProgramStateRef State = C.getState();
   SymbolRef ReceiverSymbol = M.getSelfSVal().getAsSymbol();
   assert(ReceiverSymbol && "No receiver symbol at call to [super dealloc]?");
 
   // We add this transition in checkPostObjCMessage to avoid warning when
   // we inline a call to [super dealloc] where the inlined call itself
   // calls [super dealloc].
   State = State->add<CalledSuperDealloc>(ReceiverSymbol);
   C.addTransition(State);
 }
 
+void ObjCSuperDeallocChecker::checkLocation(SVal L, bool IsLoad, const Stmt *S,
+                                  CheckerContext &C) const {
+  SymbolRef BaseSym = L.getLocSymbolInBase();
+  if (!BaseSym)
+    return;
+
+  ProgramStateRef State = C.getState();
+
+  if (!State->contains<CalledSuperDealloc>(BaseSym))
+    return;
+
+  const MemRegion *R = L.getAsRegion();
+  if (!R)
+    return;
+
+  // Climb the super regions to find the base symbol while recording
+  // the second-to-last region for error reporting.
+  const MemRegion *PriorSubRegion = nullptr;
+  while (const SubRegion *SR = dyn_cast<SubRegion>(R)) {
+    if (const SymbolicRegion *SymR = dyn_cast<SymbolicRegion>(SR)) {
+      BaseSym = SymR->getSymbol();
+      break;
+    } else {
+      R = SR->getSuperRegion();
+      PriorSubRegion = SR;
+    }
+  }
+
+  StringRef Desc = StringRef();
+  auto *IvarRegion = dyn_cast_or_null<ObjCIvarRegion>(PriorSubRegion);
+
+  if (IvarRegion) {
+    std::string Buf;
+    llvm::raw_string_ostream OS(Buf);
+    OS << "use of instance variable '" << *IvarRegion->getDecl() <<
+          "' after the instance has been freed with call to [super dealloc]";
+    Desc = OS.str();
+  }
+
+  reportUseAfterDealloc(BaseSym, Desc, S, C);
+}
+
+/// Report a use-after-dealloc on \param Sym. If not empty,
+/// \param Desc will be used to describe the error; otherwise,
+/// a default warning will be used.
+void ObjCSuperDeallocChecker::reportUseAfterDealloc(SymbolRef Sym,
+                                                    StringRef Desc,
+                                                    const Stmt *S,
+                                                    CheckerContext &C) const {
+  // We have a use of self after free.
+  // This likely causes a crash, so stop exploring the
+  // path by generating a sink.
+  ExplodedNode *ErrNode = C.generateErrorNode();
+  // If we've already reached this node on another path, return.
+  if (!ErrNode)
+    return;
+
+  if (Desc.empty())
+    Desc = "use of 'self' after it has been freed with call to [super dealloc]";
+
+  // Generate the report.
+  std::unique_ptr<BugReport> BR(
+      new BugReport(*DoubleSuperDeallocBugType, Desc, ErrNode));
+  BR->addRange(S->getSourceRange());
+  BR->addVisitor(llvm::make_unique<SuperDeallocBRVisitor>(Sym));
+  C.emitReport(std::move(BR));
+}
+
+/// Diagnose if any of the arguments to \param CE have already been
+/// dealloc'd.
+void ObjCSuperDeallocChecker::diagnoseCallArguments(const CallEvent &CE,
+                                                    CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  unsigned ArgCount = CE.getNumArgs();
+  for (unsigned I = 0; I < ArgCount; I++) {
+    SymbolRef Sym = CE.getArgSVal(I).getAsSymbol();
+    if (!Sym)
+      continue;
+
+    if (State->contains<CalledSuperDealloc>(Sym)) {
+      reportUseAfterDealloc(Sym, StringRef(), CE.getArgExpr(I), C);
+      return;
+    }
+  }
+}
+
 ObjCSuperDeallocChecker::ObjCSuperDeallocChecker()
     : IIdealloc(nullptr), IINSObject(nullptr) {
 
   DoubleSuperDeallocBugType.reset(
       new BugType(this, "[super dealloc] should not be called more than once",
                   categories::CoreFoundationObjectiveC));
 }
 

cfe/trunk/test/Analysis/DeallocUseAfterFreeErrors.m

 - (instancetype)initWithIvar:(NSObject *)ivar {
   self = [super init];
   if (!self)
     return nil;
   _ivar = [ivar retain];
   return self;
 }
 - (void)dealloc {
-  [super dealloc];
-  [_ivar release];
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  [_ivar release]; // expected-warning {{use of instance variable '_ivar' after the instance has been freed with call to [super dealloc]}}
+  // expected-note@-1 {{use of instance variable '_ivar' after the instance has been freed with call to [super dealloc]}}
 }
 @end
 
 @interface SuperDeallocThenAssignNilToIvarClass : NSObject {
   NSObject *_delegate;
 }
 @end
 
 @implementation SuperDeallocThenAssignNilToIvarClass
 - (instancetype)initWithDelegate:(NSObject *)delegate {
   self = [super init];
   if (!self)
     return nil;
   _delegate = delegate;
   return self;
 }
 - (void)dealloc {
-  [super dealloc];
-  _delegate = nil;
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  _delegate = nil; // expected-warning {{use of instance variable '_delegate' after the instance has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of instance variable '_delegate' after the instance has been freed with call to [super dealloc]}}
+}
+@end
+
+
+struct SomeStruct {
+  int f;
+};
+
+@interface SuperDeallocThenAssignIvarField : NSObject {
+  struct SomeStruct _s;
+}
+@end
+
+@implementation SuperDeallocThenAssignIvarField
+- (void)dealloc {
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  _s.f = 7; // expected-warning {{use of instance variable '_s' after the instance has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of instance variable '_s' after the instance has been freed with call to [super dealloc]}}
+}
+@end
+
+@interface OtherClassWithIvar {
+@public
+  int _otherIvar;
+}
+@end;
+
+@interface SuperDeallocThenAssignIvarIvar : NSObject {
+  OtherClassWithIvar *_ivar;
+}
+@end
+
+@implementation SuperDeallocThenAssignIvarIvar
+- (void)dealloc {
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  _ivar->_otherIvar = 7; // expected-warning {{use of instance variable '_ivar' after the instance has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of instance variable '_ivar' after the instance has been freed with call to [super dealloc]}}
 }
 @end
 
 @interface SuperDeallocThenReleasePropertyClass : NSObject { }
 @property (retain) NSObject *ivar;
 @end
 
 @implementation SuperDeallocThenReleasePropertyClass
 - (instancetype)initWithProperty:(NSObject *)ivar {
   self = [super init];
   if (!self)
     return nil;
   self.ivar = ivar;
   return self;
 }
 - (void)dealloc {
-  [super dealloc];
-  self.ivar = nil;
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  self.ivar = nil; // expected-warning {{use of 'self' after it has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of 'self' after it has been freed with call to [super dealloc]}}
 }
 @end
 
 @interface SuperDeallocThenAssignNilToPropertyClass : NSObject { }
 @property (assign) NSObject *delegate;
 @end
 
 @implementation SuperDeallocThenAssignNilToPropertyClass
 - (instancetype)initWithDelegate:(NSObject *)delegate {
   self = [super init];
   if (!self)
     return nil;
   self.delegate = delegate;
   return self;
 }
 - (void)dealloc {
-  [super dealloc];
-  self.delegate = nil;
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  self.delegate = nil; // expected-warning {{use of 'self' after it has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of 'self' after it has been freed with call to [super dealloc]}}
 }
 @end
 
 @interface SuperDeallocThenCallInstanceMethodClass : NSObject { }
 - (void)_invalidate;
 @end
 
 @implementation SuperDeallocThenCallInstanceMethodClass
 - (void)_invalidate {
 }
 - (void)dealloc {
-  [super dealloc];
-  [self _invalidate];
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  [self _invalidate]; // expected-warning {{use of 'self' after it has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of 'self' after it has been freed with call to [super dealloc]}}
 }
 @end
 
 @interface SuperDeallocThenCallNonObjectiveCMethodClass : NSObject { }
 @end
 
 static void _invalidate(NSObject *object) {
   (void)object;
 }
 
 @implementation SuperDeallocThenCallNonObjectiveCMethodClass
 - (void)dealloc {
-  [super dealloc];
-  _invalidate(self);
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  _invalidate(self); // expected-warning {{use of 'self' after it has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of 'self' after it has been freed with call to [super dealloc]}}
+}
+@end
+
+@interface SuperDeallocThenCallObjectiveClassMethodClass : NSObject { }
+@end
+
+@implementation SuperDeallocThenCallObjectiveClassMethodClass
++ (void) invalidate:(id)arg; {
+}
+
+- (void)dealloc {
+  [super dealloc]; // expected-note {{[super dealloc] called here}}
+  [SuperDeallocThenCallObjectiveClassMethodClass invalidate:self]; // expected-warning {{use of 'self' after it has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of 'self' after it has been freed with call to [super dealloc]}}
 }
 @end
 
 @interface TwoSuperDeallocCallsClass : NSObject {
   NSObject *_ivar;
 }
 - (void)_invalidate;
 @end
 
 @implementation TwoSuperDeallocCallsClass
 - (void)_invalidate {
 }
 - (void)dealloc {
-  if (_ivar) {
+  if (_ivar) { // expected-note {{Taking false branch}}
     [_ivar release];
     [super dealloc];
     return;
   }
-  [super dealloc];
-  [self _invalidate];
+  [super dealloc];    // expected-note {{[super dealloc] called here}}
+  [self _invalidate]; // expected-warning {{use of 'self' after it has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of 'self' after it has been freed with call to [super dealloc]}}
 }
 @end
 
 //===------------------------------------------------------------------------===
 // Warn about calling [super dealloc] twice due to missing return statement.
 
 @interface MissingReturnCausesDoubleSuperDeallocClass : NSObject {
   NSObject *_ivar;
+ (void)dealloc {
   [super dealloc]; // no warning: class method
 }
 @end
 
 //===------------------------------------------------------------------------===
 // Do not warn about calling [super dealloc] twice if when the analyzer has
 // inlined the call to its super deallocator.
 
-@interface SuperClassCallingSuperDealloc : NSObject
+@interface SuperClassCallingSuperDealloc : NSObject {
+  NSObject *_ivar;
+}
 @end
 
 @implementation SuperClassCallingSuperDealloc
 - (void)dealloc; {
+  [_ivar release]; // no-warning
+
   [super dealloc];
 }
 @end
 
 @interface SubclassCallingSuperDealloc : SuperClassCallingSuperDealloc
 @end
 
 @implementation SubclassCallingSuperDealloc

 @end
 
 @implementation InterveningMethodCallOnSelf
 - (void)anotherMethod {
 }
 
 - (void)dealloc; {
   [super dealloc]; // expected-note {{[super dealloc] called here}}
-  [self anotherMethod];
-  [super dealloc]; // expected-warning {{[super dealloc] should not be called multiple times}}
-  // expected-note@-1 {{[super dealloc] should not be called multiple times}}
+  [self anotherMethod]; // expected-warning {{use of 'self' after it has been freed with call to [super dealloc]}}
+      // expected-note@-1 {{use of 'self' after it has been freed with call to [super dealloc]}}
+  [super dealloc];
 }
 @end