From 591991c86fb5820dec76b0e30871212c3e17969a Mon Sep 17 00:00:00 2001 From: Devin Coughlin Date: Thu, 25 Feb 2016 23:36:52 +0000 Subject: [PATCH] [analyzer] Warn on use of 'self' after call to to [super dealloc]. 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 Differential Revision: http://reviews.llvm.org/D17528 llvm-svn: 261935 --- .../Checkers/ObjCSuperDeallocChecker.cpp | 143 +++++++++++++++--- .../test/Analysis/DeallocUseAfterFreeErrors.m | 100 +++++++++--- 2 files changed, 200 insertions(+), 43 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp index d1d6ef388db3..596ad70c423e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp @@ -8,7 +8,7 @@ //===----------------------------------------------------------------------===// // // 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. // //===----------------------------------------------------------------------===// @@ -25,7 +25,8 @@ using namespace ento; namespace { class ObjCSuperDeallocChecker - : public Checker { + : public Checker { mutable IdentifierInfo *IIdealloc, *IINSObject; mutable Selector SELdealloc; @@ -40,12 +41,24 @@ 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 @@ -71,40 +84,36 @@ public: 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(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 BR( - new BugReport(*DoubleSuperDeallocBugType, - "[super dealloc] should not be called multiple times", - ErrNode)); - BR->addRange(M.getOriginExpr()->getSourceRange()); - BR->addVisitor(llvm::make_unique(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. @@ -122,6 +131,92 @@ void ObjCSuperDeallocChecker::checkPostObjCMessage(const ObjCMethodCall &M, 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(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(R)) { + if (const SymbolicRegion *SymR = dyn_cast(SR)) { + BaseSym = SymR->getSymbol(); + break; + } else { + R = SR->getSuperRegion(); + PriorSubRegion = SR; + } + } + + StringRef Desc = StringRef(); + auto *IvarRegion = dyn_cast_or_null(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 BR( + new BugReport(*DoubleSuperDeallocBugType, Desc, ErrNode)); + BR->addRange(S->getSourceRange()); + BR->addVisitor(llvm::make_unique(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(Sym)) { + reportUseAfterDealloc(Sym, StringRef(), CE.getArgExpr(I), C); + return; + } + } +} + ObjCSuperDeallocChecker::ObjCSuperDeallocChecker() : IIdealloc(nullptr), IINSObject(nullptr) { diff --git a/clang/test/Analysis/DeallocUseAfterFreeErrors.m b/clang/test/Analysis/DeallocUseAfterFreeErrors.m index 75c2a1e573f5..9ae9a96718c3 100644 --- a/clang/test/Analysis/DeallocUseAfterFreeErrors.m +++ b/clang/test/Analysis/DeallocUseAfterFreeErrors.m @@ -35,8 +35,9 @@ typedef struct objc_selector *SEL; 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 @@ -54,8 +55,46 @@ typedef struct objc_selector *SEL; 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 @@ -72,8 +111,9 @@ typedef struct objc_selector *SEL; 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 @@ -90,8 +130,9 @@ typedef struct objc_selector *SEL; 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 @@ -103,8 +144,9 @@ typedef struct objc_selector *SEL; - (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 @@ -117,8 +159,23 @@ static void _invalidate(NSObject *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 @@ -132,13 +189,14 @@ static void _invalidate(NSObject *object) { - (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 @@ -244,11 +302,15 @@ static void _invalidate(NSObject *object) { // 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 @@ -291,8 +353,8 @@ static void _invalidate(NSObject *object) { - (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