[analyzer] PthreadLock: Escape the pointers.

[NoQ]

Summary

As usual, we need to invalidate mutex states when something may touch them. Implement this boilerplate for the thread lock checker.

The previous refactoring is handy for listing functions on which we don't need to invalidate mutex states because we model them instead.

Additionally, i don't invalidate mutex states when any system header function is called, unless the mutex is passed directly into it. The TODO here would be to model *all* system functions that may change mutex states, and then disable invalidation for the rest of them even if they take a mutex; that's what other checkers do.

[xazax.hun]

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

594 ↗

(On Diff #115053)

Wouldn't it be better to branch on IsLibraryFunction and in one branch iterate on Regions and in the other, iterate on ExplicitRegions?
That would avoid the possible quadratic explosion when lots of explicit regions are invalidated.

[a.sidorin]

Hi Artem. The patch looks mostly good, but I have an inline question.

lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

588 ↗

(On Diff #115053)

Do we think that only system headers contain library functions? Shouldn't we use CheckerContext::isCLibraryFunction() instead?

clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

public:
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddInteger(K);
   }
 };
 
 class PthreadLockChecker
-    : public Checker<check::PostCall, check::DeadSymbols> {
+    : public Checker<check::PostCall, check::DeadSymbols,
+                     check::RegionChanges> {
   BugType BT_doublelock{this, "Double locking", "Lock checker"},
           BT_doubleunlock{this, "Double unlocking", "Lock checker"},
           BT_destroylock{this, "Use destroyed lock", "Lock checker"},
           BT_initlock{this, "Init invalid lock", "Lock checker"},
           BT_lor{this, "Lock order reversal", "Lock checker"};
 
   enum LockingSemantics {
     NotApplicable = 0,
class PthreadLockChecker
   void DestroyPthreadLock(const CallEvent &Call, CheckerContext &C) const;
   void DestroyXNULock(const CallEvent &Call, CheckerContext &C) const;
   void DestroyLockAux(const CallEvent &Call, CheckerContext &C, unsigned ArgNo,
                       SVal Lock, enum LockingSemantics semantics) const;
 
 public:
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+  ProgramStateRef
+  checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *Symbols,
+                     ArrayRef<const MemRegion *> ExplicitRegions,
+                     ArrayRef<const MemRegion *> Regions,
+                     const LocationContext *LCtx, const CallEvent *Call) const;
   void printState(raw_ostream &Out, ProgramStateRef State,
                   const char *NL, const char *Sep) const override;
 };
 } // end anonymous namespace
 
 // A stack of locks for tracking lock-unlock order.
 REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
 
for (DestroyRetValTy::iterator I = TrackedSymbols.begin(),
     bool IsSymDead = SymReaper.isDead(Sym);
     // Remove the dead symbol from the return value symbols map.
     if (IsSymDead)
       State = resolvePossiblyDestroyedMutex(State, lockR, &Sym);
   }
   C.addTransition(State);
 }
 
+ProgramStateRef PthreadLockChecker::checkRegionChanges(
+    ProgramStateRef State, const InvalidatedSymbols *Symbols,
+    ArrayRef<const MemRegion *> ExplicitRegions,
+    ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx,
+    const CallEvent *Call) const {
+
+  bool IsLibraryFunction = false;
+  if (Call && Call->isGlobalCFunction()) {
+    // Avoid invalidating mutex state when a known supported function is called.
+    if (Callbacks.lookup(*Call))
+        return State;
+
+    if (Call->isInSystemHeader())
+      IsLibraryFunction = true;
+  }
+
+  for (auto R : Regions) {
+    // We assume that system library function wouldn't touch the mutex unless
+    // it takes the mutex explicitly as an argument.
+    // FIXME: This is a bit quadratic.
+    if (IsLibraryFunction &&
+        std::find(ExplicitRegions.begin(), ExplicitRegions.end(), R) ==
+            ExplicitRegions.end())
+      continue;
+
+    State = State->remove<LockMap>(R);
+    State = State->remove<DestroyRetVal>(R);
+
+    // TODO: We need to invalidate the lock stack as well. This is tricky
+    // to implement correctly and efficiently though, because the effects
+    // of mutex escapes on lock order may be fairly varied.
+  }
+
+  return State;
+}
+
 void ento::registerPthreadLockChecker(CheckerManager &mgr) {
   mgr.registerChecker<PthreadLockChecker>();
 }
 
 bool ento::shouldRegisterPthreadLockChecker(const LangOptions &LO) {
   return true;
 }

clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h

 } lck_grp_t;
 
 typedef struct {
   void *foo;
 } lck_rw_t;
 
 typedef pthread_mutex_t lck_mtx_t;
 
+extern void fake_system_function();
+extern void fake_system_function_that_takes_a_mutex(pthread_mutex_t *mtx);
+
 extern int pthread_mutex_lock(pthread_mutex_t *);
 extern int pthread_mutex_unlock(pthread_mutex_t *);
 extern int pthread_mutex_trylock(pthread_mutex_t *);
 extern int pthread_mutex_destroy(pthread_mutex_t *);
 extern int pthread_mutex_init(pthread_mutex_t  *mutex, const pthread_mutexattr_t *mutexattr);
 
 typedef int boolean_t;
 extern void lck_mtx_lock(lck_mtx_t *);
 extern void lck_mtx_unlock(lck_mtx_t *);
 extern boolean_t lck_mtx_try_lock(lck_mtx_t *);
 extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp);
 
 extern void lck_rw_lock_exclusive(lck_rw_t *lck);
 extern void lck_rw_unlock_exclusive(lck_rw_t *lck);
 extern void lck_rw_lock_shared(lck_rw_t *lck);
 extern void lck_rw_unlock_shared(lck_rw_t *lck);

clang/test/Analysis/pthreadlock.c

 
 void ok29(void) {
   lck_rw_lock_shared(&rw);
   lck_rw_unlock_shared(&rw);
   lck_rw_lock_exclusive(&rw); // no-warning
   lck_rw_unlock_exclusive(&rw); // no-warning
 }
 
+void escape_mutex(pthread_mutex_t *m);
+void ok30(void) {
+  pthread_mutex_t local_mtx;
+  pthread_mutex_init(&local_mtx, NULL);
+  pthread_mutex_lock(&local_mtx);
+  escape_mutex(&local_mtx);
+  pthread_mutex_lock(&local_mtx); // no-warning
+  pthread_mutex_unlock(&local_mtx);
+  pthread_mutex_destroy(&local_mtx);
+}
+
+void ok31(void) {
+  pthread_mutex_t local_mtx;
+  pthread_mutex_init(&local_mtx, NULL);
+  pthread_mutex_lock(&local_mtx);
+  fake_system_function_that_takes_a_mutex(&local_mtx);
+  pthread_mutex_lock(&local_mtx); // no-warning
+  pthread_mutex_unlock(&local_mtx);
+  pthread_mutex_destroy(&local_mtx);
+}
+
 void
 bad1(void)
 {
   pthread_mutex_lock(&mtx1);  // no-warning
   pthread_mutex_lock(&mtx1);  // expected-warning{{This lock has already been acquired}}
 }
 
 void

 }
 
 void bad32(void) {
   lck_rw_lock_shared(&rw);
   lck_rw_unlock_exclusive(&rw); // FIXME: warn - should be shared?
   lck_rw_lock_exclusive(&rw);
   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}}
+}