[clang][Checkers] Fix PthreadLockChecker state cleanup at dead symbol.

[balazske]

Summary

It is possible that an entry in 'DestroyRetVal' lives longer
than an entry in 'LockMap' if not removed at checkDeadSymbols.
The added test case demonstrates this.

[balazske]

Improved commit message.

[ASDenysPetrov]

Hi, @balazske

clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

290–305

I'm just wondering, did you think about such way of fixing?

[NoQ]

Hmm this probably even deserves a warning? Like, the mutex goes out of scope but we don't know whether we've successfully destroyed it? Even if we perform the check later, we can't do anything about it anymore? (not sure how bad it is in practice)

clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

290–305

This involves keeping dead regions in the program state. I'd be pretty worried about it.

[balazske]

Exactly the case is (I think) that the mutex goes out of scope and we have not checked if it was really destroyed. Still the program can check later if it was destroyed (like the if in the test case). A resource leak may be the problem (if destroy failed) so a warning like "possible resource leak if destroy call fails" can be added. The case is detected in checkDeadSymbols that may make adding a good warning more difficult.

clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

290–305

This was my first idea but did not like it because state is not cleaned up correctly. (And LockMap contains data about unaccessible mutex.)

clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

 // function to see if the return value was checked by now and set the lock state
 // - either to destroyed state or back to its previous state.
 
 // In PthreadSemantics, pthread_mutex_destroy() returns zero if the lock is
 // successfully destroyed and it returns a non-zero value otherwise.
 ProgramStateRef PthreadLockChecker::resolvePossiblyDestroyedMutex(
     ProgramStateRef state, const MemRegion *lockR, const SymbolRef *sym) const {
   const LockState *lstate = state->get<LockMap>(lockR);
   // Existence in DestroyRetVal ensures existence in LockMap.
   // Existence in Destroyed also ensures that the lock state for lockR is either
   // UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
   assert(lstate->isUntouchedAndPossiblyDestroyed() ||
          lstate->isUnlockedAndPossiblyDestroyed());
 
   ConstraintManager &CMgr = state->getConstraintManager();
   ConditionTruthVal retZero = CMgr.isNull(state, *sym);
   if (retZero.isConstrainedFalse()) {
     if (lstate->isUntouchedAndPossiblyDestroyed())
       state = state->remove<LockMap>(lockR);
     else if (lstate->isUnlockedAndPossiblyDestroyed())
       state = state->set<LockMap>(lockR, LockState::getUnlocked());
   } else
     state = state->set<LockMap>(lockR, LockState::getDestroyed());
 
   // Removing the map entry (lockR, sym) from DestroyRetVal as the lock state is
   // now resolved.
   state = state->remove<DestroyRetVal>(lockR);
   return state;
 }
 
 void PthreadLockChecker::printState(raw_ostream &Out, ProgramStateRef State,
                                     const char *NL, const char *Sep) const {
for (auto I : State->get<DestroyRetVal>()) {
     // against it. See if the return value was checked before this point.
     // This would remove the symbol from the map as well.
     if (SymReaper.isDead(I.second))
       State = resolvePossiblyDestroyedMutex(State, I.first, &I.second);
   }
 
   for (auto I : State->get<LockMap>()) {
     // Stop tracking dead mutex regions as well.
-    if (!SymReaper.isLiveRegion(I.first))
+    if (!SymReaper.isLiveRegion(I.first)) {
       State = State->remove<LockMap>(I.first);
+      State = State->remove<DestroyRetVal>(I.first);
+    }
   }
 
   // TODO: We probably need to clean up the lock stack as well.
   // It is tricky though: even if the mutex cannot be unlocked anymore,
   // it can still participate in lock order reversal resolution.
 
   C.addTransition(State);
 }

clang/test/Analysis/pthreadlock.c

void bad32(void) {
   lck_rw_unlock_shared(&rw); // FIXME: warn - should be exclusive?
 }
 
 void bad33(void) {
   pthread_mutex_lock(pmtx);
   fake_system_function();
   pthread_mutex_lock(pmtx); // expected-warning{{This lock has already been acquired}}
 }
+
+void nocrash1(pthread_mutex_t *mutex) {
+  int ret = pthread_mutex_destroy(mutex);
+  if (ret == 0) // no crash
+    ;
+}