[analyzer] CFRetainReleaseChecker: Avoid checking C++ methods with the same name.

[NoQ]

Summary

CFRetainReleaseChecker is a tiny checker that verifies that arguments of CoreFoundation retain/release functions are non-null. The checker accidentally checks all functions with the respective name, not just the actual CFRetain etc, which has caused a crash.

Fix it and modernize the checker to use CallEvent/CallDescription APIs to avoid further mistakes.

rdar://problem/42433152

[george.karpenkov]

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

537

I personally would prefer being less fancy, and avoiding the comma operator, but I suppose it's a matter of style.

567

There's also DefArgVal

[NoQ]

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

537

This isn't comma operator, just initializer list.

Alternatives are:

• CallDescription CFRetain = {"CFRetain", 1} (longer but looks the same)

• CallDescription CFRetain = CallDescription("CFRetain", 1), ... (longer and duplicates type)

• CallDescription CFRetain; CFRetainReleaseChecker(): CFRetain("CFRetain", 1) (longer and duplicates variable name)

567

?

lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace ento;
+using namespace llvm;
 
 namespace {
 class APIMisuse : public BugType {
 public:
   APIMisuse(const CheckerBase *checker, const char *name)
       : BugType(checker, name, "API Misuse (Apple)") {}
 };
 } // end anonymous namespace
void CFNumberChecker::checkPreStmt(const CallExpr *CE,
   }
 }
 
 //===----------------------------------------------------------------------===//
 // CFRetain/CFRelease/CFMakeCollectable/CFAutorelease checking for null arguments.
 //===----------------------------------------------------------------------===//
 
 namespace {
-class CFRetainReleaseChecker : public Checker< check::PreStmt<CallExpr> > {
-  mutable std::unique_ptr<APIMisuse> BT;
-  mutable IdentifierInfo *Retain, *Release, *MakeCollectable, *Autorelease;
+class CFRetainReleaseChecker : public Checker<check::PreCall> {
+  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)
-    return;
-
-  ProgramStateRef state = C.getState();
-  const FunctionDecl *FD = C.getCalleeDecl(CE);
-  if (!FD)
+  // TODO: Make this check part of CallDescription.
+  if (!Call.isGlobalCFunction())
     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<DefinedSVal> DefArgVal = ArgVal.getAs<DefinedSVal>();
   if (!DefArgVal)
     return;
 
-  // Get a NULL value.
-  SValBuilder &svalBuilder = C.getSValBuilder();
-  DefinedSVal zero =
-      svalBuilder.makeZeroVal(Arg->getType()).castAs<DefinedSVal>();
-
-  // 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");
-
-    auto report = llvm::make_unique<BugReport>(*BT, description, N);
-    report->addRange(Arg->getSourceRange());
-    bugreporter::trackNullOrUndefValue(N, Arg, *report);
+    SmallString<64> Str;
+    raw_svector_ostream OS(Str);
+    OS << "Null pointer argument in call to "
+       << cast<FunctionDecl>(Call.getDecl())->getName();
+
+    auto report = llvm::make_unique<BugReport>(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);
 }
 
 //===----------------------------------------------------------------------===//
 // Check for sending 'retain', 'release', or 'autorelease' directly to a Class.
 //===----------------------------------------------------------------------===//
 
 namespace {
 class ClassReleaseChecker : public Checker<check::PreObjCMessage> {

test/Analysis/retain-release.mm

 void _() { _(); }; // no-warning
 
 // Do not assume that IOBSDNameMatching increments a reference counter,
 // unless return type is CFMutableDictionaryRef.
 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
+}
+}