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 b852e2a..ecb2d1c 100644
--- a/lib/Checker/BasicObjCFoundationChecks.cpp
+++ b/lib/Checker/BasicObjCFoundationChecks.cpp
@@ -415,59 +415,72 @@
}
//===----------------------------------------------------------------------===//
-// CFRetain/CFRelease auditing for null arguments.
+// CFRetain/CFRelease checking for null arguments.
//===----------------------------------------------------------------------===//
namespace {
-class AuditCFRetainRelease : public GRSimpleAPICheck {
+class CFRetainReleaseChecker : public CheckerVisitor<CFRetainReleaseChecker> {
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<CallExpr>(cast<PostStmt>(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;
- // 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)) {
+ // 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<DefinedSVal>(&ArgVal);
+ if (!DefArgVal)
+ return;
+
+ // Get a NULL value.
+ ValueManager &ValMgr = C.getValueManager();
+ DefinedSVal Zero = cast<DefinedSVal>(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;
+
if (!BT)
BT = new APIMisuse("null passed to CFRetain/CFRelease");
@@ -475,19 +488,16 @@
? "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 @@
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 679c6dc..8fb0570 100644
--- a/lib/Checker/BasicObjCFoundationChecks.h
+++ b/lib/Checker/BasicObjCFoundationChecks.h
@@ -30,9 +30,6 @@
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 9e5151d..c1d6b06 100644
--- a/test/Analysis/retain-release.m
+++ b/test/Analysis/retain-release.m
@@ -468,6 +468,20 @@
}
}
+// 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.