From 19ada6cb9bf6b625bfd756e126d50e7418ead4dc Mon Sep 17 00:00:00 2001 From: Artem Dergachev Date: Wed, 29 Aug 2018 22:39:20 +0000 Subject: [PATCH] [analyzer] CFRetainReleaseChecker: Don't check C++ methods with the same name. Don't try to understand what's going on when there's a C++ method called eg. CFRetain(). Refactor the checker a bit, to use more modern APIs. Differential Revision: https://reviews.llvm.org/D50866 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@340982 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/BasicObjCFoundationChecks.cpp | 89 ++++++------------- test/Analysis/retain-release.mm | 15 ++++ 2 files changed, 43 insertions(+), 61 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp index 70ce41ac89..a21e10d948 100644 --- a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp +++ b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp @@ -36,6 +36,7 @@ using namespace clang; using namespace ento; +using namespace llvm; namespace { class APIMisuse : public BugType { @@ -531,93 +532,59 @@ void CFNumberChecker::checkPreStmt(const CallExpr *CE, //===----------------------------------------------------------------------===// namespace { -class CFRetainReleaseChecker : public Checker< check::PreStmt > { - mutable std::unique_ptr BT; - mutable IdentifierInfo *Retain, *Release, *MakeCollectable, *Autorelease; +class CFRetainReleaseChecker : public Checker { + mutable APIMisuse BT{this, "null passed to CF memory management function"}; + CallDescription CFRetain{"CFRetain", 1}, + CFRelease{"CFRelease", 1}, + CFMakeCollectable{"CFMakeCollectable", 1}, + CFAutorelease{"CFAutorelease", 1}; public: - CFRetainReleaseChecker() - : Retain(nullptr), Release(nullptr), MakeCollectable(nullptr), - Autorelease(nullptr) {} - void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; }; } // end anonymous namespace -void CFRetainReleaseChecker::checkPreStmt(const CallExpr *CE, +void CFRetainReleaseChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { - // If the CallExpr doesn't have exactly 1 argument just give up checking. - if (CE->getNumArgs() != 1) + // TODO: Make this check part of CallDescription. + if (!Call.isGlobalCFunction()) return; - ProgramStateRef state = C.getState(); - const FunctionDecl *FD = C.getCalleeDecl(CE); - if (!FD) - return; - - if (!BT) { - ASTContext &Ctx = C.getASTContext(); - Retain = &Ctx.Idents.get("CFRetain"); - Release = &Ctx.Idents.get("CFRelease"); - MakeCollectable = &Ctx.Idents.get("CFMakeCollectable"); - Autorelease = &Ctx.Idents.get("CFAutorelease"); - BT.reset(new APIMisuse( - this, "null passed to CF memory management function")); - } - // Check if we called CFRetain/CFRelease/CFMakeCollectable/CFAutorelease. - const IdentifierInfo *FuncII = FD->getIdentifier(); - if (!(FuncII == Retain || FuncII == Release || FuncII == MakeCollectable || - FuncII == Autorelease)) + if (!(Call.isCalled(CFRetain) || Call.isCalled(CFRelease) || + Call.isCalled(CFMakeCollectable) || Call.isCalled(CFAutorelease))) return; - // FIXME: The rest of this just checks that the argument is non-null. - // It should probably be refactored and combined with NonNullParamChecker. - // Get the argument's value. - const Expr *Arg = CE->getArg(0); - SVal ArgVal = C.getSVal(Arg); + SVal ArgVal = Call.getArgSVal(0); Optional DefArgVal = ArgVal.getAs(); if (!DefArgVal) return; - // Get a NULL value. - SValBuilder &svalBuilder = C.getSValBuilder(); - DefinedSVal zero = - svalBuilder.makeZeroVal(Arg->getType()).castAs(); - - // Make an expression asserting that they're equal. - DefinedOrUnknownSVal ArgIsNull = svalBuilder.evalEQ(state, zero, *DefArgVal); - - // Are they equal? - ProgramStateRef stateTrue, stateFalse; - std::tie(stateTrue, stateFalse) = state->assume(ArgIsNull); + // Is it null? + ProgramStateRef state = C.getState(); + ProgramStateRef stateNonNull, stateNull; + std::tie(stateNonNull, stateNull) = state->assume(*DefArgVal); - if (stateTrue && !stateFalse) { - ExplodedNode *N = C.generateErrorNode(stateTrue); + if (!stateNonNull) { + ExplodedNode *N = C.generateErrorNode(stateNull); if (!N) return; - const char *description; - if (FuncII == Retain) - description = "Null pointer argument in call to CFRetain"; - else if (FuncII == Release) - description = "Null pointer argument in call to CFRelease"; - else if (FuncII == MakeCollectable) - description = "Null pointer argument in call to CFMakeCollectable"; - else if (FuncII == Autorelease) - description = "Null pointer argument in call to CFAutorelease"; - else - llvm_unreachable("impossible case"); + SmallString<64> Str; + raw_svector_ostream OS(Str); + OS << "Null pointer argument in call to " + << cast(Call.getDecl())->getName(); - auto report = llvm::make_unique(*BT, description, N); - report->addRange(Arg->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, Arg, *report); + auto report = llvm::make_unique(BT, OS.str(), N); + report->addRange(Call.getArgSourceRange(0)); + bugreporter::trackNullOrUndefValue(N, Call.getArgExpr(0), *report); C.emitReport(std::move(report)); return; } // From here on, we know the argument is non-null. - C.addTransition(stateFalse); + C.addTransition(stateNonNull); } //===----------------------------------------------------------------------===// diff --git a/test/Analysis/retain-release.mm b/test/Analysis/retain-release.mm index ac83c1a48e..5dc8f857d8 100644 --- a/test/Analysis/retain-release.mm +++ b/test/Analysis/retain-release.mm @@ -470,3 +470,18 @@ void* IOBSDNameMatching(); void rdar33832412() { void* x = IOBSDNameMatching(); // no-warning } + + +namespace member_CFRetains { +class Foo { +public: + void CFRetain(const Foo &) {} + void CFRetain(int) {} +}; + +void bar() { + Foo foo; + foo.CFRetain(foo); // no-warning + foo.CFRetain(0); // no-warning +} +} -- 2.40.0