[analyzer] Detect duplicate [super dealloc] calls

[dcoughlin]

Summary

Implement a path checker that warns about duplicate calls to [super dealloc]. This is the first step before re-implementing the use-after-free checks using a path checker (see D5042).

[ddkilzer]

FIXME: This path checker needs to discriminate between different 'self' symbols when recursing into the -dealloc method in the same class (for different objects). See test case.

So I need to test the implicit 'self' argument (for the current method) whenever I encounter a '[super dealloc]' call so I can differentiate the same '[super dealloc]' call for different objects. I'm not sure whether I should use an SVal or a SymbolRef (or maybe an ImplicitParamDecl) to store the 'self' symbol, though. Any tips?

I can probably switch to REGISTER_MAP_WITH_PROGRAMSTATE() and store the 'self' symbol in there as well.

Note that I posted this patch for review before fixing that bug to address any (early) feedback.

[ddkilzer]

FIXME: This path checker needs to discriminate between different 'self' symbols when recursing into the -dealloc method in the same class (for different objects). See test case.

Fixed. I needed a SymbolRef (as expected after I thought about it some more). Wow, that was easy:

SymbolRef SelfSymbol = M.getSelfSVal().getAsSymbol();

[ddkilzer]

• Updated help text in Checkers.td to be more generic.
• Fix false positive with classes using custom retain counting and containing an ivar that's the same type as the class. Added test case for this condition.
• Extracted duplicate code into isSuperDeallocMessage().
• Small tweaks to variable names.
• Add section headers for [super dealloc] test cases to explain what each one is testing.

[zaks.anna]

David,

I've listed some small concerns throughout and have not looked at the tests in a lot of detail yet.

However, the main problem is that you store too much state in SuperDeallocState. (We try to keep the states as lean as possible since they are one of the major bottlenecks and have considerable impact on performance.) Most of what you store is only consumed by BugReporterVisitor. You might be able to work around storing this info by lazily reconstructing it at bug reporting time.

When bug is reported, the path is visited bottom up; this is when your visitor will get called. You can teach the visitor to detect the first node in which the SuperDeallocState for the given SymRef has been added (as the path is visited bottom up). At that point, you look up the statement associated with the ProgramPoint and if it's a message call, you found the node to which the diagnostic should be attached. (Take a look at other checkers such as MallocChecker; they are all based on this lazy approach.)

You might also want to play with Stack Hints in error reporting since your reports are likely to span multiple functions. These are the notes that get attached to the call site of the functions that contain the diagnostic. You should use -analyzer-output=text (and -analyzer-output=plist) when testing the full path experience. (Ex: See ./test/Analysis/keychainAPI-diagnostic-visitor.m)

lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp

157

Is this done for performance purposes? Can [super dealloc] be called from functions? isSuperDeallocMessage check seems faster.

162

It would be better to do an early return here as well, unless you plan to expand this for handling other calls.
if (!isSuperDeallocMessage(M))

return;

183

These should be guarded on isSuperDeallocMessage. Otherwise, we would do a lookup every time we see a method call.

[ddkilzer]

Thanks for the feedback!

I will look at reducing the state stored by SuperDeallocState.

I will also add stack hints like "[super dealloc] was called here first" for the first call, and "[super dealloc] was called a second time here", or similar.

lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp

157

This check was modeled after ObjCSelfInitChecker::checkPostObjCMessage() in ObjCSelfInitChecker.cpp.

Heh, I see. The checks in shouldRunOnFunctionOrMethod() are redundant because we're already passed a reference to an ObjCMethodCall.

I don't believe [super dealloc] is valid inside a plain C function context.

162

Sure, will fix.

183

Got it! Will fix.

[dcoughlin]

Updated this to address Anna's comments.

• I've made the state smaller. It is just now a set of SymbolRefs for methods instances that have been dealloc'd.
• I've hoisted isSuperDeallocMessage() to early return when possible.
• I've added tests with -analyzer-output=text.
• I've moved the lazy initialization of the dealloc selector identifier to isSuperDeallocMessage()

[zaks.anna]

Looks good, below are some comments which are mostly nits.

What's the plan for bringing this out of alpha? Is it pending evaluation on real code?

lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp

12

Please, be more specific about the properties we are checking for.

92

Not sure if the extra check buys us much (right?), but it's not on a hot path, so we could keep it.

103

maybe remove "originally"?

113

"should never be" -> "should not be"?
"twice" -> "more than once"

127

So we are actually storing the symbol that refers to 'super', right? The receiver of 'dealloc', not the 'self' of the object whose methods are being analyzed. This was confusing reading the code top down; for example, in the bug visitor.

156

Maybe we should say "more than once" or "again" in case we've missed a call to [super] dealloc.

165

You should consider extending the helper method that Gabor added for checking if a specific function has been called in
http://reviews.llvm.org/D15921

test/Analysis/DeallocUseAfterFreeErrors.m

272

you can put expected-note on the next line using "expected-note@-1"

[dcoughlin]

Address more of Anna's comments.

• Add a more explicit comment about checker in header comment
• Changed the checker to always use the receiver symbol rather than the self symbol for clarity.
• Rework SuperDeallocBRVisitor::VisitNode() to add a note on the node where state transitioned from not having CalledSuperDealloc for the receiver symbol to having it set.
• Reworded diagnostic text.

I didn't extend Gabor's function helper to Objective-C. I'll do that in a later patch.

[dcoughlin]

In D5238#348199, @zaks.anna wrote:

Looks good, below are some comments which are mostly nits.

What's the plan for bringing this out of alpha? Is it pending evaluation on real code?

I will first extend this checker to check for uses of self after dealloc and then evaluate.

[dcoughlin]

Addressed additional comments from Anna offline:

• "[super dealloc] called again" is OK as a path note but not good as an error message. I've changed it to "[super dealloc] should not be called multiple times".
• Added a comment about why we need both PreObjCMessage and PostObjCMessage to avoid warning when inling a call to [super dealloc]. (The SubclassCallingSuperDealloc test covers this.)

lib/StaticAnalyzer/Checkers/Checkers.td

 } // end "osx.cocoa"
 
 let ParentPackage = CocoaAlpha in {
 
 def ObjCDeallocChecker : Checker<"Dealloc">,
   HelpText<"Warn about Objective-C classes that lack a correct implementation of -dealloc">,
   DescFile<"CheckObjCDealloc.cpp">;
 
+def ObjCSuperDeallocChecker : Checker<"SuperDealloc">,
+  HelpText<"Warn about use of 'self' after '[super dealloc]' in Objective-C "
+           "-dealloc methods">,
+  DescFile<"ObjCSuperDeallocChecker.cpp">;
+
 def InstanceVariableInvalidation : Checker<"InstanceVariableInvalidation">,
   HelpText<"Check that the invalidatable instance variables are invalidated in the methods annotated with objc_instance_variable_invalidator">,
   DescFile<"IvarInvalidationChecker.cpp">;
 
 def MissingInvalidationMethod : Checker<"MissingInvalidationMethod">,
   HelpText<"Check that the invalidation methods are present in classes that contain invalidatable instance variables">,
   DescFile<"IvarInvalidationChecker.cpp">;
 

lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp

+//===- ObjCSuperDeallocChecker.cpp - Check use of [super dealloc] message -===//
+//
+//                     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 checks for the
+// correct use of the [super dealloc] message in -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 SuperDeallocState {
+
+  SymbolRef SuperDeallocSymbol;
+  unsigned BlockID;
+  const StackFrameContext *SFC;
+
+public:
+  SuperDeallocState(SymbolRef S, unsigned B, const StackFrameContext *SFC)
+      : SuperDeallocSymbol(S), BlockID(B), SFC(SFC) {}
+
+  SymbolRef getSuperDeallocSymbol() const { return SuperDeallocSymbol; }
+  const StackFrameContext *getStackFrameContext() const { return SFC; }
+
+  bool operator==(const SuperDeallocState &X) const {
+    return BlockID == X.BlockID && SFC == X.SFC &&
+           SuperDeallocSymbol == X.SuperDeallocSymbol;
+  }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddPointer(SuperDeallocSymbol);
+    ID.AddInteger(BlockID);
+    ID.AddPointer(SFC);
+  }
+};
+
+class SuperDeallocBRVisitor
+    : public BugReporterVisitorImpl<SuperDeallocBRVisitor> {
+
+  SymbolRef SuperDeallocSymbol;
+  const StackFrameContext *SFC;
+  bool Satisfied;
+
+public:
+  SuperDeallocBRVisitor(SymbolRef SuperDeallocSymbol,
+                        const StackFrameContext *SFC)
+      : SuperDeallocSymbol(SuperDeallocSymbol), SFC(SFC), Satisfied(false) {}
+
+  PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ,
+                                 const ExplodedNode *Pred,
+                                 BugReporterContext &BRC,
+                                 BugReport &BR) override;
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+    ID.Add(SuperDeallocSymbol);
+    ID.Add(SFC);
+  }
+};
+
+class ObjCSuperDeallocChecker
+    : public Checker<check::PostObjCMessage, check::PreObjCMessage> {
+
+  mutable IdentifierInfo *IIdealloc, *IINSObject;
+  mutable Selector SELdealloc;
+
+  std::unique_ptr<BugType> DoubleSuperDeallocBugType;
+
+  void initIdentifierInfoAndSelectors(ASTContext &Ctx) const;
+
+  bool shouldRunOnFunctionOrMethod(const NamedDecl *ND) const;
+
+public:
+  ObjCSuperDeallocChecker();
+  void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
+  void checkPreObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
+};
+
+} // end anonymous namespace
+
+// Remember if we called [super dealloc] previously.
+REGISTER_LIST_WITH_PROGRAMSTATE(CalledSuperDealloc, SuperDeallocState)
+
+PathDiagnosticPiece *SuperDeallocBRVisitor::VisitNode(const ExplodedNode *Succ,
+                                                      const ExplodedNode *Pred,
+                                                      BugReporterContext &BRC,
+                                                      BugReport &BR) {
+  if (Satisfied)
+    return nullptr;
+
+  const Expr *E = nullptr;
+
+  if (Optional<PostStmt> P = Succ->getLocationAs<PostStmt>())
+    E = P->getStmtAs<ObjCMessageExpr>();
+
+  if (!E)
+    return nullptr;
+
+  ProgramStateRef State = Succ->getState();
+  SVal S = State->getSVal(E, Succ->getLocationContext());
+  if (SuperDeallocSymbol == S.getAsSymbol() && SFC == Succ->getStackFrame()) {
+    Satisfied = true;
+
+    // Construct a new PathDiagnosticPiece.
+    ProgramPoint P = Succ->getLocation();
+    PathDiagnosticLocation L =
+        PathDiagnosticLocation::create(P, BRC.getSourceManager());
+
+    if (!L.isValid() || !L.asLocation().isValid())
+      return nullptr;
+
+    return new PathDiagnosticEventPiece(
+        L, "[super dealloc] originally called here");
+  }
+
+  return nullptr;
+}
+
+ObjCSuperDeallocChecker::ObjCSuperDeallocChecker()
+    : IIdealloc(nullptr), IINSObject(nullptr) {
+
+  DoubleSuperDeallocBugType.reset(
+      new BugType(this, "[super dealloc] should never be called twice",
+                  categories::CoreFoundationObjectiveC));
+}
+
+void ObjCSuperDeallocChecker::checkPostObjCMessage(const ObjCMethodCall &M,
+                                                   CheckerContext &C) const {
+  initIdentifierInfoAndSelectors(C.getASTContext());
+
+  // FIXME: A callback should disable checkers at the start of functions.
+  if (!shouldRunOnFunctionOrMethod(
+          dyn_cast<NamedDecl>(C.getCurrentAnalysisDeclContext()->getDecl())))
+    return;
+
+  // Check for [super dealloc] method call.
+  if (M.getOriginExpr()->getReceiverKind() == ObjCMessageExpr::SuperInstance &&
+      M.getSelector() == SELdealloc) {
+    ProgramStateRef State = C.getState();
+    SVal V = State->getSVal(M.getOriginExpr(), C.getLocationContext());
+    State = State->add<CalledSuperDealloc>(
+        SuperDeallocState(V.getAsSymbol(), C.getBlockID(), C.getStackFrame()));
+    C.addTransition(State);
+  }
+}
+
+void ObjCSuperDeallocChecker::checkPreObjCMessage(const ObjCMethodCall &M,
+                                                  CheckerContext &C) const {
+  initIdentifierInfoAndSelectors(C.getASTContext());
+
+  // If [super dealloc] has not been called, there is nothing to do.
+  ProgramStateRef State = C.getState();
+  CalledSuperDeallocTy SDStateList = State->get<CalledSuperDealloc>();
+  if (SDStateList.isEmpty())
+    return;
+
+  assert(++SDStateList.begin() == SDStateList.end() &&
+         "There should only ever be one SuperDeallocState in the list "
+         "because a second one causes a sink node to be created.");
+
+  // Check for a duplicate [super dealloc] method call.
+  if (M.getOriginExpr()->getReceiverKind() == ObjCMessageExpr::SuperInstance &&
+      M.getSelector() == SELdealloc) {
+
+    // We have reached a bug that likely causes a crash, so stop exploring the
+    // path by generating a sink.
+    ExplodedNode *ErrNode = C.generateSink();
+    // If we've already reached this node on another path, return.
+    if (!ErrNode)
+      return;
+
+    // Generate the report.
+    std::unique_ptr<BugReport> R(
+        new BugReport(*DoubleSuperDeallocBugType,
+                      "[super dealloc] called a second time", ErrNode));
+    R->addRange(M.getOriginExpr()->getSourceRange());
+    const SuperDeallocState &SDState = SDStateList.getHead();
+    R->addVisitor(new SuperDeallocBRVisitor(SDState.getSuperDeallocSymbol(),
+                                            SDState.getStackFrameContext()));
+    C.emitReport(R.release());
+
+    return;
+  }
+}
+
+void
+ObjCSuperDeallocChecker::initIdentifierInfoAndSelectors(ASTContext &Ctx) const {
+
+  if (IIdealloc)
+    return;
+
+  IIdealloc = &Ctx.Idents.get("dealloc");
+  IINSObject = &Ctx.Idents.get("NSObject");
+
+  SELdealloc = Ctx.Selectors.getSelector(0, &IIdealloc);
+}
+
+// FIXME: A callback should disable checkers at the start of functions.
+bool ObjCSuperDeallocChecker::shouldRunOnFunctionOrMethod(
+    const NamedDecl *ND) const {
+
+  if (!ND)
+    return false;
+
+  const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(ND);
+  if (!MD)
+    return false;
+
+  // [super dealloc] only applies to NSObject subclasses.
+  for (ObjCInterfaceDecl *ID = MD->getClassInterface()->getSuperClass(); ID;
+       ID = ID->getSuperClass()) {
+
+    IdentifierInfo *II = ID->getIdentifier();
+
+    if (II == IINSObject)
+      return true;
+  }
+
+  return false;
+}
+
+//===----------------------------------------------------------------------===//
+// Checker Registration.
+//===----------------------------------------------------------------------===//
+
+void ento::registerObjCSuperDeallocChecker(CheckerManager &Mgr) {
+  const LangOptions &LangOpts = Mgr.getLangOpts();
+  if (LangOpts.getGC() == LangOptions::GCOnly || LangOpts.ObjCAutoRefCount)
+    return;
+  Mgr.registerChecker<ObjCSuperDeallocChecker>();
+}

test/Analysis/DeallocUseAfterFreeErrors.m

+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.osx.cocoa.SuperDealloc -verify %s
+
+#define nil ((id)0)
+
+typedef unsigned long NSUInteger;
+@protocol NSObject
+- (instancetype)retain;
+- (oneway void)release;
+@end
+
+@interface NSObject <NSObject> { }
+- (void)dealloc;
+- (instancetype)init;
+@end
+
+typedef struct objc_selector *SEL;
+
+//===------------------------------------------------------------------------===
+//  <rdar://problem/6953275>
+//  Check that 'self' is not referenced after calling '[super dealloc]'.
+
+@interface SuperDeallocThenReleaseIvarClass : NSObject {
+  NSObject *_ivar;
+}
+@end
+
+@implementation SuperDeallocThenReleaseIvarClass
+- (instancetype)initWithIvar:(NSObject *)ivar {
+  self = [super init];
+  if (!self)
+    return nil;
+  _ivar = [ivar retain];
+  return self;
+}
+- (void)dealloc {
+  [super dealloc];
+  [_ivar release];
+}
+@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;
+}
+@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;
+}
+@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;
+}
+@end
+
+@interface SuperDeallocThenCallInstanceMethodClass : NSObject { }
+- (void)_invalidate;
+@end
+
+@implementation SuperDeallocThenCallInstanceMethodClass
+- (void)_invalidate {
+}
+- (void)dealloc {
+  [super dealloc];
+  [self _invalidate];
+}
+@end
+
+@interface SuperDeallocThenCallNonObjectiveCMethodClass : NSObject { }
+@end
+
+static void _invalidate(NSObject *object) {
+  (void)object;
+}
+
+@implementation SuperDeallocThenCallNonObjectiveCMethodClass
+- (void)dealloc {
+  [super dealloc];
+  _invalidate(self);
+}
+@end
+
+@interface TwoSuperDeallocCallsClass : NSObject {
+  NSObject *_ivar;
+}
+- (void)_invalidate;
+@end
+
+@implementation TwoSuperDeallocCallsClass
+- (void)_invalidate {
+}
+- (void)dealloc {
+  if (_ivar) {
+    [_ivar release];
+    [super dealloc];
+    return;
+  }
+  [super dealloc];
+  [self _invalidate];
+}
+@end
+
+@interface MissingReturnCausesDoubleSuperDeallocClass : NSObject {
+  NSObject *_ivar;
+}
+@end
+
+@implementation MissingReturnCausesDoubleSuperDeallocClass
+- (void)dealloc {
+  if (_ivar) {
+    [_ivar release];
+    [super dealloc];
+    //return;
+  }
+  [super dealloc]; // expected-warning{{[super dealloc] called a second time}}
+}
+@end
+
+@interface SuperDeallocInOtherMethodClass : NSObject {
+  NSObject *_ivar;
+}
+- (void)_cleanup;
+@end
+
+@implementation SuperDeallocInOtherMethodClass
+- (void)_cleanup {
+  [_ivar release];
+  [super dealloc];
+}
+- (void)dealloc {
+  [self _cleanup];
+  [super dealloc]; // expected-warning{{[super dealloc] called a second time}}
+}
+@end
+
+// A class that contains an ivar of itself can generate a false positive that
+// [super dealloc] is called twice if each object instance is not tracked
+// separately by the checker, and if custom reference counting is implemented,
+// as happens when _OBJC_SUPPORTED_INLINE_REFCNT_WITH_DEALLOC2MAIN is used.
+// This is just a simple approximation to trigger the bug.
+@class ClassWithOwnIvarInstanceClass;
+@interface ClassWithOwnIvarInstanceClass : NSObject {
+  ClassWithOwnIvarInstanceClass *_ivar;
+  NSUInteger _retainCount;
+}
+@end
+
+@implementation ClassWithOwnIvarInstanceClass
+- (instancetype)retain {
+  ++_retainCount;
+  return self;
+}
+- (oneway void)release {
+  --_retainCount;
+  if (!_retainCount)
+    [self dealloc];
+}
+- (void)dealloc {
+  [_ivar release];
+  [super dealloc]; // expected-warning{{[super dealloc] called a second time}}
+  // FIXME: This is a false-positive since the checker does not track 'self' values for different objects separately.
+}
+@end