[analyzer] Fix false positives in Keychain API checker

[zaks.anna]

Summary

The checker has several false positives that this patch addresses:

1. 1. Do not check if the return status has been compared to error (or no error) at the time when leaks are reported since the status symbol might no longer be alive. Instead, pattern match on the assume and stop tracking allocated symbols on error paths.
   2. The checker used to report error when an unknown symbol was freed. This could lead to false positives, let's not repot those. This leads to loss of coverage in double frees.
   3. Do not enforce that we should only call free if we are sure that error was not returned and the pointer is not null. That warning is too noisy and we received several false positive reports about it. (I removed: "Only call free if a valid (non-NULL) buffer was returned")
2. Use !isDead instead of isLive in leak reporting. Otherwise, we report leaks for objects we loose track of. This change triggered change #1.

This also adds checker specific dump to the state.

[alexander-shaposhnikov]

lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

527 ↗

(On Diff #83146)

nit: auto I = ...

[alexander-shaposhnikov]

lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

527 ↗

(On Diff #83146)

and the same below (although not particularly important)

[dcoughlin]

Looks good to me, aside from minor quibbles about capitalization and variable naming.

lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

502 ↗

(On Diff #83146)

It seems a bit weird to call this a 'Set' -- it is a map, right?

514 ↗

(On Diff #83146)

Typo: 'SymintExpr' --> 'SymIntExpr'

518 ↗

(On Diff #83146)

'errorIsReturned' should start with a capital letter.

[zaks.anna]

Addressed all comments

[NoQ]

Do not check if the return status has been compared to error (or no error) at the time when leaks are reported since the status symbol might no longer be alive. Instead, pattern match on the assume and stop tracking allocated symbols on error paths.

Aha, i see! So we have pairs (A, B) of symbols (symbol A - the data that needs to be freed, and symbol B - the corresponding return value that needs to be checked for error). And liveness of A during free() doesn't imply liveness of B during free().

There are multiple options:

1. In checkDeadSymbols, detect that B is dying, extract the necessary assume() results, and update the allocation state similarly to how you did in evalAssume, but only upon death of B.
2. In checkLiveSymbols, mark B as live for as long as A is alive.

I'm in favor of option 2 ideologically (if we ever automate GDM symbol-to-symbol maps to avoid manual cleanup, they'd naturally work that way out of the box and will be easy to understand) and of option 1 performance-wise (we'd maintain less live symbols, our most frequently-accessed maps will become smaller).

In any case, we shouldn't keep dead symbols in checker state maps, because the kind of error you spotted may show up pretty often.

[zaks.anna]

I did not think of solution #1! It's definitely better than the pattern matching I've added here. However, this checker fires so infrequently, that I do not think it's worth investing more time into perfecting it.

I suspect the solution #2 is what this checker was trying to use to begin with. It marks the return symbol as dependent on the allocated symbol by calling:

C.getSymbolManager().addSymbolDependency(V, RetStatusSymbol);

However, addSymbolDependency only worked for isLive() and not for !isDead(). Would be good to investigate this further as other checkers such as malloc also use addSymbolDependency.

[NoQ]

In D28330#637075, @zaks.anna wrote:

I did not think of solution #1! It's definitely better than the pattern matching I've added here. However, this checker fires so infrequently, that I do not think it's worth investing more time into perfecting it.

Well, the thing i like about your solution is that it resists arbitrary state splits. Eg., if the reason for the state split is some body farm function or a checker state split, which doesn't constitute a programmer's intent to check for error, then there's no reason to clean state. However, we won't be able to find this out by looking at range constraints retrospectively - we'd only know it during evalAssume/checkBranchCondition. However, currently we're doing a great job in other places around the analyzer to avoid unnecessary state splits, so range constraints are pretty reliable.

In fact, a check in an inlined function should also not be considered a valid reason for state cleanup. Or probably even for a state split. But that's another story. Anyway, i approve your approach here :)

cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp

 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class MacOSKeychainAPIChecker : public Checker<check::PreStmt<CallExpr>,
                                                check::PostStmt<CallExpr>,
-                                               check::DeadSymbols> {
+                                               check::DeadSymbols,
+                                               eval::Assume> {
   mutable std::unique_ptr<BugType> BT;
 
 public:
   /// AllocationState is a part of the checker specific state together with the
   /// MemRegion corresponding to the allocated data.
   struct AllocationState {
     /// The index of the allocator function.
     unsigned int AllocatorIdx;
void Profile(llvm::FoldingSetNodeID &ID) const {
       ID.AddInteger(AllocatorIdx);
       ID.AddPointer(Region);
     }
   };
 
   void checkPreStmt(const CallExpr *S, CheckerContext &C) const;
   void checkPostStmt(const CallExpr *S, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
+  ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond,
+                             bool Assumption) const;
+  void printState(raw_ostream &Out, ProgramStateRef State,
+                  const char *NL, const char *Sep) const;
 
 private:
   typedef std::pair<SymbolRef, const AllocationState*> AllocationPair;
   typedef SmallVector<AllocationPair, 2> AllocationPairVec;
 
   enum APIKind {
     /// Denotes functions tracked by this checker.
     ValidAPI = 0,
private:
 
   /// Find the allocation site for Sym on the path leading to the node N.
   const ExplodedNode *getAllocationNode(const ExplodedNode *N, SymbolRef Sym,
                                         CheckerContext &C) const;
 
   std::unique_ptr<BugReport> generateAllocatedDataNotReleasedReport(
       const AllocationPair &AP, ExplodedNode *N, CheckerContext &C) const;
 
-  /// Check if RetSym evaluates to an error value in the current state.
-  bool definitelyReturnedError(SymbolRef RetSym,
-                               ProgramStateRef State,
-                               SValBuilder &Builder,
-                               bool noError = false) const;
-
-  /// Check if RetSym evaluates to a NoErr value in the current state.
-  bool definitelyDidnotReturnError(SymbolRef RetSym,
-                                   ProgramStateRef State,
-                                   SValBuilder &Builder) const {
-    return definitelyReturnedError(RetSym, State, Builder, true);
-  }
-
   /// Mark an AllocationPair interesting for diagnostic reporting.
   void markInteresting(BugReport *R, const AllocationPair &AP) const {
     R->markInteresting(AP.first);
     R->markInteresting(AP.second->Region);
   }
 
   /// The bug visitor which allows us to print extra diagnostics along the
   /// BugReport path. For example, showing the allocation site of the leaked
if (Optional<loc::MemRegionVal> X = ArgV.getAs<loc::MemRegionVal>()) {
     StoreManager& SM = C.getStoreManager();
     SymbolRef sym = SM.getBinding(State->getStore(), *X).getAsLocSymbol();
     if (sym)
       return sym;
   }
   return nullptr;
 }
 
-// When checking for error code, we need to consider the following cases:
-// 1) noErr / [0]
-// 2) someErr / [1, inf]
-// 3) unknown
-// If noError, returns true iff (1).
-// If !noError, returns true iff (2).
-bool MacOSKeychainAPIChecker::definitelyReturnedError(SymbolRef RetSym,
-                                                      ProgramStateRef State,
-                                                      SValBuilder &Builder,
-                                                      bool noError) const {
-  DefinedOrUnknownSVal NoErrVal = Builder.makeIntVal(NoErr,
-    Builder.getSymbolManager().getType(RetSym));
-  DefinedOrUnknownSVal NoErr = Builder.evalEQ(State, NoErrVal,
-                                                     nonloc::SymbolVal(RetSym));
-  ProgramStateRef ErrState = State->assume(NoErr, noError);
-  return ErrState == State;
-}
-
 // Report deallocator mismatch. Remove the region from tracking - reporting a
 // missing free error after this one is redundant.
 void MacOSKeychainAPIChecker::
   generateDeallocatorMismatchReport(const AllocationPair &AP,
                                     const Expr *ArgExpr,
                                     CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   State = State->remove<AllocatedData>(AP.first);
void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE,
   if (idx != InvalidIdx) {
     unsigned paramIdx = FunctionsToTrack[idx].Param;
     if (CE->getNumArgs() <= paramIdx)
       return;
 
     const Expr *ArgExpr = CE->getArg(paramIdx);
     if (SymbolRef V = getAsPointeeSymbol(ArgExpr, C))
       if (const AllocationState *AS = State->get<AllocatedData>(V)) {
-        if (!definitelyReturnedError(AS->Region, State, C.getSValBuilder())) {
-          // Remove the value from the state. The new symbol will be added for
-          // tracking when the second allocator is processed in checkPostStmt().
-          State = State->remove<AllocatedData>(V);
-          ExplodedNode *N = C.generateNonFatalErrorNode(State);
-          if (!N)
-            return;
-          initBugType();
-          SmallString<128> sbuf;
-          llvm::raw_svector_ostream os(sbuf);
-          unsigned int DIdx = FunctionsToTrack[AS->AllocatorIdx].DeallocatorIdx;
-          os << "Allocated data should be released before another call to "
-              << "the allocator: missing a call to '"
-              << FunctionsToTrack[DIdx].Name
-              << "'.";
-          auto Report = llvm::make_unique<BugReport>(*BT, os.str(), N);
-          Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(V));
-          Report->addRange(ArgExpr->getSourceRange());
-          Report->markInteresting(AS->Region);
-          C.emitReport(std::move(Report));
-        }
-      }
     return;
   }
 
   // Is it a call to one of deallocator functions?
   idx = getTrackedFunctionIndex(funName, false);
   if (idx == InvalidIdx)
     return;
 
void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE,
   if (!ArgSM) {
     if (!isBadDeallocationArgument(ArgSVal.getAsRegion()))
       return;
     RegionArgIsBad = true;
   }
 
   // Is the argument to the call being tracked?
   const AllocationState *AS = State->get<AllocatedData>(ArgSM);
-  if (!AS && FunctionsToTrack[idx].Kind != ValidAPI) {
+  if (!AS)
     return;
-  }
-  // If trying to free data which has not been allocated yet, report as a bug.
-  // TODO: We might want a more precise diagnostic for double free
+
+  // TODO: We might want to report double free here.
   // (that would involve tracking all the freed symbols in the checker state).
-  if (!AS || RegionArgIsBad) {
+  if (RegionArgIsBad) {
     // It is possible that this is a false positive - the argument might
     // have entered as an enclosing function parameter.
     if (isEnclosingFunctionParam(ArgExpr))
       return;
 
     ExplodedNode *N = C.generateNonFatalErrorNode(State);
     if (!N)
       return;
void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE,
   // Check if the proper deallocator is used.
   unsigned int PDeallocIdx = FunctionsToTrack[AS->AllocatorIdx].DeallocatorIdx;
   if (PDeallocIdx != idx || (FunctionsToTrack[idx].Kind == ErrorAPI)) {
     const AllocationPair AP = std::make_pair(ArgSM, AS);
     generateDeallocatorMismatchReport(AP, ArgExpr, C);
     return;
   }
 
-  // If the buffer can be null and the return status can be an error,
-  // report a bad call to free.
-  if (State->assume(ArgSVal.castAs<DefinedSVal>(), false) &&
-      !definitelyDidnotReturnError(AS->Region, State, C.getSValBuilder())) {
-    ExplodedNode *N = C.generateNonFatalErrorNode(State);
-    if (!N)
-      return;
-    initBugType();
-    auto Report = llvm::make_unique<BugReport>(
-        *BT, "Only call free if a valid (non-NULL) buffer was returned.", N);
-    Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(ArgSM));
-    Report->addRange(ArgExpr->getSourceRange());
-    Report->markInteresting(AS->Region);
-    C.emitReport(std::move(Report));
-    return;
-  }
-
   C.addTransition(State);
 }
 
 void MacOSKeychainAPIChecker::checkPostStmt(const CallExpr *CE,
                                             CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   const FunctionDecl *FD = C.getCalleeDecl(CE);
   if (!FD || FD->getKind() != Decl::Function)
auto Report =
       llvm::make_unique<BugReport>(*BT, os.str(), N, LocUsedForUniqueing,
                                   AllocNode->getLocationContext()->getDecl());
 
   Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(AP.first));
   markInteresting(Report.get(), AP);
   return Report;
 }
 
+/// If the return symbol is assumed to be error, remove the allocated info
+/// from consideration.
+ProgramStateRef MacOSKeychainAPIChecker::evalAssume(ProgramStateRef State,
+                                                    SVal Cond,
+                                                    bool Assumption) const {
+  AllocatedDataTy AMap = State->get<AllocatedData>();
+  if (AMap.isEmpty())
+    return State;
+
+  auto *CondBSE = dyn_cast_or_null<BinarySymExpr>(Cond.getAsSymExpr());
+  if (!CondBSE)
+    return State;
+  BinaryOperator::Opcode OpCode = CondBSE->getOpcode();
+  if (OpCode != BO_EQ && OpCode != BO_NE)
+    return State;
+
+  // Match for a restricted set of patterns for cmparison of error codes.
+  // Note, the comparisons of type '0 == st' are transformed into SymIntExpr.
+  SymbolRef ReturnSymbol = nullptr;
+  if (auto *SIE = dyn_cast<SymIntExpr>(CondBSE)) {
+    const llvm::APInt &RHS = SIE->getRHS();
+    bool ErrorIsReturned = (OpCode == BO_EQ && RHS != NoErr) ||
+                           (OpCode == BO_NE && RHS == NoErr);
+    if (!Assumption)
+      ErrorIsReturned = !ErrorIsReturned;
+    if (ErrorIsReturned)
+      ReturnSymbol = SIE->getLHS();
+  }
+
+  if (ReturnSymbol)
+    for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) {
+      if (ReturnSymbol == I->second.Region)
+        State = State->remove<AllocatedData>(I->first);
+    }
+
+  return State;
+}
+
 void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR,
                                                CheckerContext &C) const {
   ProgramStateRef State = C.getState();
-  AllocatedDataTy ASet = State->get<AllocatedData>();
-  if (ASet.isEmpty())
+  AllocatedDataTy AMap = State->get<AllocatedData>();
+  if (AMap.isEmpty())
     return;
 
   bool Changed = false;
   AllocationPairVec Errors;
-  for (AllocatedDataTy::iterator I = ASet.begin(), E = ASet.end(); I != E; ++I) {
-    if (SR.isLive(I->first))
+  for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) {
+    if (!SR.isDead(I->first))
       continue;
 
     Changed = true;
     State = State->remove<AllocatedData>(I->first);
-    // If the allocated symbol is null or if the allocation call might have
-    // returned an error, do not report.
+    // If the allocated symbol is null do not report.
     ConstraintManager &CMgr = State->getConstraintManager();
     ConditionTruthVal AllocFailed = CMgr.isNull(State, I.getKey());
-    if (AllocFailed.isConstrainedTrue() ||
-        definitelyReturnedError(I->second.Region, State, C.getSValBuilder()))
+    if (AllocFailed.isConstrainedTrue())
       continue;
     Errors.push_back(std::make_pair(I->first, &I->second));
   }
   if (!Changed) {
     // Generate the new, cleaned up state.
     C.addTransition(State);
     return;
   }
MacOSKeychainAPIChecker::SecKeychainBugVisitor::VisitNode(
   assert(Idx != InvalidIdx && "This should be a call to an allocator.");
   const Expr *ArgExpr = CE->getArg(FunctionsToTrack[Idx].Param);
   PathDiagnosticLocation Pos(ArgExpr, BRC.getSourceManager(),
                              N->getLocationContext());
   return std::make_shared<PathDiagnosticEventPiece>(Pos,
                                                     "Data is allocated here.");
 }
 
+void MacOSKeychainAPIChecker::printState(raw_ostream &Out,
+                                         ProgramStateRef State,
+                                         const char *NL,
+                                         const char *Sep) const {
+
+  AllocatedDataTy AMap = State->get<AllocatedData>();
+
+  if (!AMap.isEmpty()) {
+    Out << Sep << "KeychainAPIChecker :" << NL;
+    for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) {
+      I.getKey()->dumpToStream(Out);
+    }
+  }
+}
+
+
 void ento::registerMacOSKeychainAPIChecker(CheckerManager &mgr) {
   mgr.registerChecker<MacOSKeychainAPIChecker>();
 }

cfe/trunk/test/Analysis/keychainAPI.m

-// RUN: %clang_cc1 -analyze -analyzer-checker=osx.SecKeychainAPI %s -verify
+// RUN: %clang_cc1 -analyze -analyzer-checker=osx.SecKeychainAPI -fblocks %s -verify
+
+#include "Inputs/system-header-simulator-objc.h"
 
 // Fake typedefs.
 typedef unsigned int OSStatus;
 typedef unsigned int SecKeychainAttributeList;
 typedef unsigned int SecKeychainItemRef;
 typedef unsigned int SecItemClass;
 typedef unsigned int UInt32;
-typedef unsigned int CFTypeRef;
-typedef unsigned int UInt16;
 typedef unsigned int SecProtocolType;
 typedef unsigned int SecAuthenticationType;
 typedef unsigned int SecKeychainAttributeInfo;
 enum {
   noErr                      = 0,
   GenericError               = 1
 };
 

 
 void errRetVal() {
   unsigned int *ptr = 0;
   OSStatus st = 0;
   UInt32 length;
   void *outData;
   st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData);
   if (st == GenericError)
-    SecKeychainItemFreeContent(ptr, outData); // expected-warning{{Only call free if a valid (non-NULL) buffer was returned}}
+    SecKeychainItemFreeContent(ptr, outData);
 } // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}}
 
 // If null is passed in, the data is not allocated, so no need for the matching free.
 void fooDoNotReportNull() {
     unsigned int *ptr = 0;
     OSStatus st = 0;
     UInt32 *length = 0;
     void **outData = 0;
     SecKeychainItemCopyContent(2, ptr, ptr, 0, 0);
     SecKeychainItemCopyContent(2, ptr, ptr, length, outData);
 }// no-warning
 
 void doubleAlloc() {
     unsigned int *ptr = 0;
     OSStatus st = 0;
     UInt32 length;
     void *outData;
     st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData);
     st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData); // expected-warning {{Allocated data should be released before another call to the allocator:}}
     if (st == noErr)
       SecKeychainItemFreeContent(ptr, outData);
 }
 
-void fooOnlyFree() {
-  unsigned int *ptr = 0;
-  OSStatus st = 0;
-  UInt32 length;
-  void *outData = &length;
-  SecKeychainItemFreeContent(ptr, outData);// expected-warning{{Trying to free data which has not been allocated}}
-}
-
 // Do not warn if undefined value is passed to a function.
 void fooOnlyFreeUndef() {
   unsigned int *ptr = 0;
   OSStatus st = 0;
   UInt32 length;
   void *outData;
   SecKeychainItemFreeContent(ptr, outData);
 }// no-warning
int foo(CFTypeRef keychainOrArray, SecProtocolType protocol,
   st = SecKeychainFindInternetPassword(keychainOrArray, 
                                        16, "server", 16, "domain", 16, "account",
                                        16, "path", 222, protocol, authenticationType,
                                        &length, &(outData[3]), itemRef);
   if (length == 5) {
     if (st == noErr)
       SecKeychainItemFreeContent(ptr, outData[3]);
   }
-  if (length) { // expected-warning{{Allocated data is not released: missing a call to 'SecKeychainItemFreeContent'}}
+  if (length) { // TODO: We do not report a warning here since the symbol is no longer live, but it's not marked as dead.
     length++;
   }
   return 0;
-}// no-warning
+}
+
+int testErrorCodeAsLHS(CFTypeRef keychainOrArray, SecProtocolType protocol,
+        SecAuthenticationType authenticationType, SecKeychainItemRef *itemRef) {
+  unsigned int *ptr = 0;
+  OSStatus st = 0;
+  UInt32 length;
+  void *outData;
+  st = SecKeychainFindInternetPassword(keychainOrArray,
+                                       16, "server", 16, "domain", 16, "account",
+                                       16, "path", 222, protocol, authenticationType,
+                                       &length, &outData, itemRef);
+  if (noErr == st)
+    SecKeychainItemFreeContent(ptr, outData);
+
+  return 0;
+}
 
 void free(void *ptr);
 void deallocateWithFree() {
     unsigned int *ptr = 0;
     OSStatus st = 0;
     UInt32 length;
     void *outData;
     st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &outData);

 typedef const struct __CFAllocator * CFAllocatorRef;
 extern const CFAllocatorRef kCFAllocatorDefault;
 extern const CFAllocatorRef kCFAllocatorSystemDefault;
 extern const CFAllocatorRef kCFAllocatorMalloc;
 extern const CFAllocatorRef kCFAllocatorMallocZone;
 extern const CFAllocatorRef kCFAllocatorNull;
 extern const CFAllocatorRef kCFAllocatorUseContext;
 CFStringRef CFStringCreateWithBytesNoCopy(CFAllocatorRef alloc, const uint8_t *bytes, CFIndex numBytes, CFStringEncoding encoding, Boolean externalFormat, CFAllocatorRef contentsDeallocator);
-extern void CFRelease(CFStringRef cf);
 
 void DellocWithCFStringCreate1(CFAllocatorRef alloc) {
   unsigned int *ptr = 0;
   OSStatus st = 0;
   UInt32 length;
   void *bytes;
   char * x;
   st = SecKeychainItemCopyContent(2, ptr, ptr, &length, &bytes);
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() {
+void radar10508828_20092614() {
   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}}
+  SecKeychainItemFreeContent(0, pwdBytes);
 }
 
 //Example from bug 10797.
 __inline__ static
 const char *__WBASLLevelString(int level) {
   return "foo";
 }
 

 void allocAndFree3(void *attrList) {
     UInt32 length = 32;
     void *outData;
     OSStatus st = my_Allocate_Param(&outData, &length); 
     if (st == noErr)
       SecKeychainItemFreeContent(attrList, outData);
 }
 
+typedef struct AuthorizationValue {
+    int length;
+    void *data;
+} AuthorizationValue;
+typedef struct AuthorizationCallback {
+    OSStatus (*SetContextVal)(AuthorizationValue *inValue);
+} AuthorizationCallback;
+static AuthorizationCallback cb;
+int radar_19196494() {
+  @autoreleasepool {
+    AuthorizationValue login_password = {};
+    UInt32 passwordLength;
+    void *passwordData = 0;
+    OSStatus err = SecKeychainFindGenericPassword(0, 0, "", 0, "", (UInt32 *)&login_password.length, (void**)&login_password.data, 0);
+    cb.SetContextVal(&login_password);
+    if (err == noErr) {
+      SecKeychainItemFreeContent(0, login_password.data);
+    }
+  }
+  return 0;
+}