From 40c5c24c06a1696b92e9bd98721ee6d0fa1d0182 Mon Sep 17 00:00:00 2001 From: Jordy Rose Date: Tue, 6 Jul 2010 02:34:42 +0000 Subject: [PATCH] Improve NULL-checking for CFRetain/CFRelease. We now remember that the argument was non-NULL, and we report where the null assumption came from (like AttrNonNullChecker already did). llvm-svn: 107633 --- .../lib/Checker/BasicObjCFoundationChecks.cpp | 93 +++++++++++-------- clang/lib/Checker/BasicObjCFoundationChecks.h | 3 - clang/test/Analysis/retain-release.m | 14 +++ 3 files changed, 66 insertions(+), 44 deletions(-) diff --git a/clang/lib/Checker/BasicObjCFoundationChecks.cpp b/clang/lib/Checker/BasicObjCFoundationChecks.cpp index b852e2ad9a48..ecb2d1c4e34b 100644 --- a/clang/lib/Checker/BasicObjCFoundationChecks.cpp +++ b/clang/lib/Checker/BasicObjCFoundationChecks.cpp @@ -415,59 +415,72 @@ clang::CreateAuditCFNumberCreate(ASTContext& Ctx, BugReporter& BR) { } //===----------------------------------------------------------------------===// -// CFRetain/CFRelease auditing for null arguments. +// CFRetain/CFRelease checking for null arguments. //===----------------------------------------------------------------------===// namespace { -class AuditCFRetainRelease : public GRSimpleAPICheck { +class CFRetainReleaseChecker : public CheckerVisitor { 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){} + CFRetainReleaseChecker(ASTContext& Ctx): BT(NULL), + Retain(&Ctx.Idents.get("CFRetain")), Release(&Ctx.Idents.get("CFRelease")) + {} - ~AuditCFRetainRelease() {} + static void *getTag() { static int x = 0; return &x; } - bool Audit(ExplodedNode* N, GRStateManager&); + void PreVisitCallExpr(CheckerContext& C, const CallExpr* CE); }; } // end anonymous namespace -bool AuditCFRetainRelease::Audit(ExplodedNode* N, GRStateManager&) { - const CallExpr* CE = cast(cast(N->getLocation()).getStmt()); - +void CFRetainReleaseChecker::PreVisitCallExpr(CheckerContext& C, + const CallExpr* CE) { // If the CallExpr doesn't have exactly 1 argument just give up checking. if (CE->getNumArgs() != 1) - return false; + return; - // Check if we called CFRetain/CFRelease. - const GRState* state = N->getState(); + // Get the function declaration of the callee. + const GRState* state = C.getState(); SVal X = state->getSVal(CE->getCallee()); const FunctionDecl* FD = X.getAsFunctionDecl(); if (!FD) - return false; + return; + // Check if we called CFRetain/CFRelease. const IdentifierInfo *FuncII = FD->getIdentifier(); if (!(FuncII == Retain || FuncII == Release)) - return false; + return; + + // FIXME: The rest of this just checks that the argument is non-null. + // It should probably be refactored and combined with AttrNonNullChecker. + + // Get the argument's value. + const Expr *Arg = CE->getArg(0); + SVal ArgVal = state->getSVal(Arg); + DefinedSVal *DefArgVal = dyn_cast(&ArgVal); + if (!DefArgVal) + return; + + // Get a NULL value. + ValueManager &ValMgr = C.getValueManager(); + DefinedSVal Zero = cast(ValMgr.makeZeroVal(Arg->getType())); + + // Make an expression asserting that they're equal. + SValuator &SVator = ValMgr.getSValuator(); + DefinedOrUnknownSVal ArgIsNull = SVator.EvalEQ(state, Zero, *DefArgVal); + + // Are they equal? + const GRState *stateTrue, *stateFalse; + llvm::tie(stateTrue, stateFalse) = state->Assume(ArgIsNull); + + if (stateTrue && !stateFalse) { + ExplodedNode *N = C.GenerateSink(stateTrue); + if (!N) + return; - // 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"); @@ -475,19 +488,16 @@ bool AuditCFRetainRelease::Audit(ExplodedNode* N, GRStateManager&) { ? "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; + EnhancedBugReport *report = new EnhancedBugReport(*BT, description, N); + report->addRange(Arg->getSourceRange()); + report->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, Arg); + + C.EmitReport(report); + return; } - return false; -} - - -GRSimpleAPICheck* -clang::CreateAuditCFRetainRelease(ASTContext& Ctx, BugReporter& BR) { - return new AuditCFRetainRelease(Ctx, BR); + // From here on, we know the argument is non-null. + C.addTransition(stateFalse); } //===----------------------------------------------------------------------===// @@ -569,9 +579,10 @@ void clang::RegisterAppleChecks(GRExprEngine& Eng, const Decl &D) { Eng.AddCheck(CreateBasicObjCFoundationChecks(Ctx, BR), Stmt::ObjCMessageExprClass); Eng.AddCheck(CreateAuditCFNumberCreate(Ctx, BR), Stmt::CallExprClass); - Eng.AddCheck(CreateAuditCFRetainRelease(Ctx, BR), Stmt::CallExprClass); RegisterNSErrorChecks(BR, Eng, D); RegisterNSAutoreleasePoolChecks(Eng); + + Eng.registerCheck(new CFRetainReleaseChecker(Ctx)); Eng.registerCheck(new ClassReleaseChecker(Ctx)); } diff --git a/clang/lib/Checker/BasicObjCFoundationChecks.h b/clang/lib/Checker/BasicObjCFoundationChecks.h index 679c6dc1df2d..8fb057087086 100644 --- a/clang/lib/Checker/BasicObjCFoundationChecks.h +++ b/clang/lib/Checker/BasicObjCFoundationChecks.h @@ -30,9 +30,6 @@ GRSimpleAPICheck *CreateBasicObjCFoundationChecks(ASTContext& Ctx, GRSimpleAPICheck *CreateAuditCFNumberCreate(ASTContext& Ctx, BugReporter& BR); -GRSimpleAPICheck *CreateAuditCFRetainRelease(ASTContext& Ctx, - BugReporter& BR); - void RegisterNSErrorChecks(BugReporter& BR, GRExprEngine &Eng, const Decl &D); void RegisterNSAutoreleasePoolChecks(GRExprEngine &Eng); diff --git a/clang/test/Analysis/retain-release.m b/clang/test/Analysis/retain-release.m index 9e5151d16704..c1d6b06f59c7 100644 --- a/clang/test/Analysis/retain-release.m +++ b/clang/test/Analysis/retain-release.m @@ -468,6 +468,20 @@ void f16(int x, CFTypeRef p) { } } +// Test that an object is non-null after being CFRetained/CFReleased. +void f17(int x, CFTypeRef p) { + if (x) { + CFRelease(p); + if (!p) + CFRelease(0); // no-warning + } + else { + CFRetain(p); + if (!p) + CFRetain(0); // no-warning + } +} + // Test basic tracking of ivars associated with 'self'. For the retain/release // checker we currently do not want to flag leaks associated with stores // of tracked objects to ivars.