[analyzer] Make KeychainAPI checker less aggressive. radar://10508828

We trigger an error if free is called after a possibly failed allocation. Do not trigger the error if we know that the buffer is not null.

llvm-svn: 145584

2 files changed, 22 insertions, 4 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
index b49684233a6..78707e7e8ae 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
@@ -414,14 +414,16 @@ void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE,
     return;
   }
 
-  // If the return status is undefined or is error, report a bad call to free.
-  if (!definitelyDidnotReturnError(AS->Region, State, C.getSValBuilder())) {
+  // If the buffer can be null and the return status can be an error,
+  // report a bad call to free.
+  if (State->assume(cast<DefinedSVal>(ArgSVal), false) &&
+      !definitelyDidnotReturnError(AS->Region, State, C.getSValBuilder())) {
     ExplodedNode *N = C.addTransition(State);
     if (!N)
       return;
     initBugType();
     BugReport *Report = new BugReport(*BT,
-        "Call to free data when error was returned during allocation.", N);
+        "Only call free if a valid (non-NULL) buffer was returned.", N);
     Report->addVisitor(new SecKeychainBugVisitor(ArgSM));
     Report->addRange(ArgExpr->getSourceRange());
     C.EmitReport(Report);
diff --git a/clang/test/Analysis/keychainAPI.m b/clang/test/Analysis/keychainAPI.m
index d10600dea59..613d74659fd 100644
--- a/clang/test/Analysis/keychainAPI.m
+++ b/clang/test/Analysis/keychainAPI.m
@@ -77,7 +77,7 @@ void errRetVal() {
   void *outData;
   st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData);
   if (st == GenericError) // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'.}}
-    SecKeychainItemFreeContent(ptr, outData); // expected-warning{{Call to free data when error was returned during allocation.}}
+    SecKeychainItemFreeContent(ptr, outData); // expected-warning{{Only call free if a valid (non-NULL) buffer was returned}}
 }
 
 // If null is passed in, the data is not allocated, so no need for the matching free.
@@ -305,6 +305,22 @@ void DellocWithCFStringCreate4(CFAllocatorRef alloc) {
   }
 }
 
+void radar10508828() {
+  UInt32 pwdLen = 0;
+  void*  pwdBytes = 0;
+  OSStatus rc = SecKeychainFindGenericPassword(0, 3, "foo", 3, "bar", &pwdLen, &pwdBytes, 0);
+#pragma unused(rc)
+  if (pwdBytes)
+    SecKeychainItemFreeContent(0, pwdBytes);
+}
+
+void radar10508828_2() {
+  UInt32 pwdLen = 0;
+  void*  pwdBytes = 0;
+  OSStatus rc = SecKeychainFindGenericPassword(0, 3, "foo", 3, "bar", &pwdLen, &pwdBytes, 0);
+  SecKeychainItemFreeContent(0, pwdBytes); // expected-warning {{Only call free if a valid (non-NULL) buffer was returned.}}
+}
+
 //Example from bug 10797.
 __inline__ static
 const char *__WBASLLevelString(int level) {