From: Jordy Rose Date: Tue, 6 Jul 2010 02:34:42 +0000 (+0000) Subject: Improve NULL-checking for CFRetain/CFRelease. We now remember that the argument was... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=61fb55c87909bd749a367f7c12b302e39aa3e392;p=clang 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). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@107633 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Checker/BasicObjCFoundationChecks.cpp b/lib/Checker/BasicObjCFoundationChecks.cpp index b852e2ad9a..ecb2d1c4e3 100644 --- a/lib/Checker/BasicObjCFoundationChecks.cpp +++ b/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; - } - - return false; -} + EnhancedBugReport *report = new EnhancedBugReport(*BT, description, N); + report->addRange(Arg->getSourceRange()); + report->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, Arg); + C.EmitReport(report); + return; + } -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/lib/Checker/BasicObjCFoundationChecks.h b/lib/Checker/BasicObjCFoundationChecks.h index 679c6dc1df..8fb0570870 100644 --- a/lib/Checker/BasicObjCFoundationChecks.h +++ b/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/test/Analysis/retain-release.m b/test/Analysis/retain-release.m index 9e5151d167..c1d6b06f59 100644 --- a/test/Analysis/retain-release.m +++ b/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.