[cfe-commits] r75570 - in /cfe/trunk: lib/Analysis/BasicObjCFoundationChecks.cpp lib/Analysis/BasicObjCFoundationChecks.h test/Analysis/retain-release.m

[Ted Kremenek]

Author: kremenek
Date: Mon Jul 13 19:43:42 2009
New Revision: 75570

URL: http://llvm.org/viewvc/llvm-project?rev=75570&view=rev
Log:
Add basic checking for passing NULL to CFRetain/CFRelease, since those functions
are not explicitly marked as not accepting NULL pointers. This check illustrates
how we need more refactoring in the custom-check logic.

Modified:
    cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.cpp
    cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.h
    cfe/trunk/test/Analysis/retain-release.m

Modified: cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.cpp?rev=75570&r1=75569&r2=75570&view=diff

==============================================================================
--- cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.cpp (original)
+++ cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.cpp Mon Jul 13 19:43:42 2009
@@ -458,7 +458,84 @@
 }
 
 //===----------------------------------------------------------------------===//
+// CFRetain/CFRelease auditing for null arguments.
+//===----------------------------------------------------------------------===//
+
+namespace {
+class VISIBILITY_HIDDEN AuditCFRetainRelease : public GRSimpleAPICheck {
+  APIMisuse *BT;
+  
+  // FIXME: Either this should be refactored into GRSimpleAPICheck, or
+  //   it should always be passed with a call to Audit.  The latter
+  //   approach makes this class more stateless.
+  ASTContext& Ctx;
+  IdentifierInfo *Retain, *Release;
+  BugReporter& BR;
+  
+public:
+  AuditCFRetainRelease(ASTContext& ctx, BugReporter& br) 
+  : BT(0), Ctx(ctx),
+    Retain(&Ctx.Idents.get("CFRetain")), Release(&Ctx.Idents.get("CFRelease")),
+    BR(br){}
+  
+  ~AuditCFRetainRelease() {}
+  
+  bool Audit(ExplodedNode<GRState>* N, GRStateManager&);
+};
+} // end anonymous namespace
+
+
+bool AuditCFRetainRelease::Audit(ExplodedNode<GRState>* N, GRStateManager&) {
+  CallExpr* CE = cast<CallExpr>(cast<PostStmt>(N->getLocation()).getStmt());
+  
+  // If the CallExpr doesn't have exactly 1 argument just give up checking.
+  if (CE->getNumArgs() != 1)
+    return false;
+  
+  // Check if we called CFRetain/CFRelease.
+  const GRState* state = N->getState();
+  SVal X = state->getSVal(CE->getCallee());
+  const FunctionDecl* FD = X.getAsFunctionDecl();
+  
+  if (!FD)
+    return false;
+  
+  const IdentifierInfo *FuncII = FD->getIdentifier();  
+  if (!(FuncII == Retain || FuncII == Release))
+    return false;
+  
+  // Finally, check if the argument is NULL.
+  // FIXME: We should be able to bifurcate the state here, as a successful
+  // check will result in the value not being NULL afterwards.
+  // FIXME: Need a way to register vistors for the BugReporter.  Would like
+  // to benefit from the same diagnostics that regular null dereference
+  // reporting has.
+  if (state->getStateManager().isEqual(state, CE->getArg(0), 0)) {
+    if (!BT)
+      BT = new APIMisuse("null passed to CFRetain/CFRelease");
+    
+    const char *description = (FuncII == Retain)
+                            ? "Null pointer argument in call to CFRetain"
+                            : "Null pointer argument in call to CFRelease";
+
+    RangedBugReport *report = new RangedBugReport(*BT, description, N);
+    report->addRange(CE->getArg(0)->getSourceRange());
+    BR.EmitReport(report);
+    return true;
+  }
+
+  return false;
+}
+    
+    
+GRSimpleAPICheck*
+clang::CreateAuditCFRetainRelease(ASTContext& Ctx, BugReporter& BR) {  
+  return new AuditCFRetainRelease(Ctx, BR);
+}
+
+//===----------------------------------------------------------------------===//
 // Check registration.
+//===----------------------------------------------------------------------===//
 
 void clang::RegisterAppleChecks(GRExprEngine& Eng) {
   ASTContext& Ctx = Eng.getContext();
@@ -466,9 +543,8 @@
 
   Eng.AddCheck(CreateBasicObjCFoundationChecks(Ctx, BR),
                Stmt::ObjCMessageExprClass);
-
-  Eng.AddCheck(CreateAuditCFNumberCreate(Ctx, BR),
-               Stmt::CallExprClass);
+  Eng.AddCheck(CreateAuditCFNumberCreate(Ctx, BR), Stmt::CallExprClass);  
+  Eng.AddCheck(CreateAuditCFRetainRelease(Ctx, BR), Stmt::CallExprClass);
   
   RegisterNSErrorChecks(BR, Eng);
 }

Modified: cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.h?rev=75570&r1=75569&r2=75570&view=diff

==============================================================================
--- cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.h (original)
+++ cfe/trunk/lib/Analysis/BasicObjCFoundationChecks.h Mon Jul 13 19:43:42 2009
@@ -32,12 +32,15 @@
 class BugReporter;
 class GRExprEngine;
   
-GRSimpleAPICheck* CreateBasicObjCFoundationChecks(ASTContext& Ctx,
+GRSimpleAPICheck *CreateBasicObjCFoundationChecks(ASTContext& Ctx,
                                                   BugReporter& BR);
   
-GRSimpleAPICheck* CreateAuditCFNumberCreate(ASTContext& Ctx,
+GRSimpleAPICheck *CreateAuditCFNumberCreate(ASTContext& Ctx,
                                             BugReporter& BR);
   
+GRSimpleAPICheck *CreateAuditCFRetainRelease(ASTContext& Ctx,
+                                             BugReporter& BR);
+  
 void RegisterNSErrorChecks(BugReporter& BR, GRExprEngine &Eng);
   
 } // end clang namespace

Modified: cfe/trunk/test/Analysis/retain-release.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/retain-release.m?rev=75570&r1=75569&r2=75570&view=diff

==============================================================================
--- cfe/trunk/test/Analysis/retain-release.m (original)
+++ cfe/trunk/test/Analysis/retain-release.m Mon Jul 13 19:43:42 2009
@@ -390,6 +390,18 @@
   CFRelease(*B);  // no-warning
 }
 
+// Test when we pass NULL to CFRetain/CFRelease.
+void f16(int x, CFTypeRef p) {
+  if (p)
+    return;
+
+  if (x) {
+    CFRelease(p); // expected-warning{{Null pointer argument in call to CFRelease}}
+  }
+  else {
+    CFRetain(p); // expected-warning{{Null pointer argument in call to CFRetain}}
+  }
+}
 
 // Test basic tracking of ivars associated with 'self'.  For the retain/release
 // checker we currently do not want to flag leaks associated with stores