[cfe-commits] r138296 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp test/Analysis/keychainAPI.m

[Anna Zaks]

Author: zaks
Date: Mon Aug 22 18:18:12 2011
New Revision: 138296

URL: http://llvm.org/viewvc/llvm-project?rev=138296&view=rev
Log:
[analyzer] MacOSKeychainAPIChecker: Users of KeyChain API often use free() to deallocate the password. Catch this error explicitly and generate the error message at the place where free() is called.

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
    cfe/trunk/test/Analysis/keychainAPI.m

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp?rev=138296&r1=138295&r2=138296&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp Mon Aug 22 18:18:12 2011
@@ -68,9 +68,13 @@
     const char* Name;
     unsigned int Param;
     unsigned int DeallocatorIdx;
+    /// The flag specifies if the call is valid or is a result of a common user
+    /// error (Ex: free instead of SecKeychainItemFreeContent), which we also
+    /// track for better diagnostics.
+    bool isValid;
   };
   static const unsigned InvalidIdx = 100000;
-  static const unsigned FunctionsToTrackSize = 6;
+  static const unsigned FunctionsToTrackSize = 7;
   static const ADFunctionInfo FunctionsToTrack[FunctionsToTrackSize];
   /// The value, which represents no error return value for allocator functions.
   static const unsigned NoErr = 0;
@@ -129,12 +133,13 @@
 
 const MacOSKeychainAPIChecker::ADFunctionInfo
   MacOSKeychainAPIChecker::FunctionsToTrack[FunctionsToTrackSize] = {
-    {"SecKeychainItemCopyContent", 4, 3},                       // 0
-    {"SecKeychainFindGenericPassword", 6, 3},                   // 1
-    {"SecKeychainFindInternetPassword", 13, 3},                 // 2
-    {"SecKeychainItemFreeContent", 1, InvalidIdx},              // 3
-    {"SecKeychainItemCopyAttributesAndData", 5, 5},             // 4
-    {"SecKeychainItemFreeAttributesAndData", 1, InvalidIdx},    // 5
+    {"SecKeychainItemCopyContent", 4, 3, true},                       // 0
+    {"SecKeychainFindGenericPassword", 6, 3, true},                   // 1
+    {"SecKeychainFindInternetPassword", 13, 3, true},                 // 2
+    {"SecKeychainItemFreeContent", 1, InvalidIdx, true},              // 3
+    {"SecKeychainItemCopyAttributesAndData", 5, 5, true},             // 4
+    {"SecKeychainItemFreeAttributesAndData", 1, InvalidIdx, true},    // 5
+    {"free", 0, InvalidIdx, false},                                   // 6
 };
 
 unsigned MacOSKeychainAPIChecker::getTrackedFunctionIndex(StringRef Name,
@@ -257,6 +262,9 @@
   if (idx == InvalidIdx)
     return;
 
+  // We also track invalid deallocators. Ex: free() for enhanced error messages.
+  bool isValidDeallocator = FunctionsToTrack[idx].isValid;
+
   // Check the argument to the deallocator.
   const Expr *ArgExpr = CE->getArg(FunctionsToTrack[idx].Param);
   SVal ArgSVal = State->getSVal(ArgExpr);
@@ -280,7 +288,7 @@
   // TODO: We might want a more precise diagnostic for double free
   // (that would involve tracking all the freed symbols in the checker state).
   const AllocationState *AS = State->get<AllocatedData>(ArgSM);
-  if (!AS || RegionArgIsBad) {
+  if ((!AS || RegionArgIsBad) && isValidDeallocator) {
     // It is possible that this is a false positive - the argument might
     // have entered as an enclosing function parameter.
     if (isEnclosingFunctionParam(ArgExpr))
@@ -305,7 +313,7 @@
   // tracking the allocated symbol to avoid reporting a missing free after the
   // deallocator mismatch error.
   unsigned int PDeallocIdx = FunctionsToTrack[AS->AllocatorIdx].DeallocatorIdx;
-  if (PDeallocIdx != idx) {
+  if (PDeallocIdx != idx || !isValidDeallocator) {
     ExplodedNode *N = C.generateNode(State);
     if (!N)
       return;
@@ -313,7 +321,7 @@
 
     llvm::SmallString<80> sbuf;
     llvm::raw_svector_ostream os(sbuf);
-    os << "Allocator doesn't match the deallocator: '"
+    os << "Deallocator doesn't match the allocator: '"
        << FunctionsToTrack[PDeallocIdx].Name << "' should be used.";
     BugReport *Report = new BugReport(*BT, os.str(), N);
     Report->addRange(ArgExpr->getSourceRange());

Modified: cfe/trunk/test/Analysis/keychainAPI.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/keychainAPI.m?rev=138296&r1=138295&r2=138296&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/keychainAPI.m (original)
+++ cfe/trunk/test/Analysis/keychainAPI.m Mon Aug 22 18:18:12 2011
@@ -176,7 +176,7 @@
   st = SecKeychainItemCopyAttributesAndData(itemRef, info, itemClass, 
                                             &attrList, &length, &outData); 
   if (st == noErr)
-    SecKeychainItemFreeContent(attrList, outData); // expected-warning{{Allocator doesn't match the deallocator}}
+    SecKeychainItemFreeContent(attrList, outData); // expected-warning{{Deallocator doesn't match the allocator}}
   return 0;
 }
 
@@ -225,3 +225,15 @@
   }
   return 0;
 }// no-warning
+
+void free(void *ptr);
+void deallocateWithFree() {
+    unsigned int *ptr = 0;
+    OSStatus st = 0;
+    UInt32 length;
+    void *outData;
+    st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData);
+    if (st == noErr)
+      free(outData); // expected-warning{{Deallocator doesn't match the allocator: 'SecKeychainItemFreeContent' should be used}}
+}
+