From: Jordan Rose Date: Tue, 1 Apr 2014 03:40:47 +0000 (+0000) Subject: [analyzer] Lock checker: make sure locks aren't used after being destroyed. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=5ba640a6b2ce11bf1b301315bd49bc0f5f121d9a;p=clang [analyzer] Lock checker: make sure locks aren't used after being destroyed. Patch by Daniel Fahlgren! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@205275 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index 921184e757..76ae02731a 100644 --- a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -24,9 +24,35 @@ using namespace clang; using namespace ento; namespace { + +struct LockState { + enum Kind { Destroyed, Locked, Unlocked } K; + +private: + LockState(Kind K) : K(K) {} + +public: + static LockState getLocked(void) { return LockState(Locked); } + static LockState getUnlocked(void) { return LockState(Unlocked); } + static LockState getDestroyed(void) { return LockState(Destroyed); } + + bool operator==(const LockState &X) const { + return K == X.K; + } + + bool isLocked() const { return K == Locked; } + bool isUnlocked() const { return K == Unlocked; } + bool isDestroyed() const { return K == Destroyed; } + + void Profile(llvm::FoldingSetNodeID &ID) const { + ID.AddInteger(K); + } +}; + class PthreadLockChecker : public Checker< check::PostStmt > { mutable std::unique_ptr BT_doublelock; mutable std::unique_ptr BT_doubleunlock; + mutable std::unique_ptr BT_destroylock; mutable std::unique_ptr BT_lor; enum LockingSemantics { NotApplicable = 0, @@ -40,13 +66,15 @@ public: bool isTryLock, enum LockingSemantics semantics) const; void ReleaseLock(CheckerContext &C, const CallExpr *CE, SVal lock) const; + void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const; + void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE) const; }; } // end anonymous namespace // GDM Entry for tracking lock state. REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *) -REGISTER_SET_WITH_PROGRAMSTATE(UnlockSet, const MemRegion *) +REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState) void PthreadLockChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { @@ -56,7 +84,7 @@ void PthreadLockChecker::checkPostStmt(const CallExpr *CE, if (FName.empty()) return; - if (CE->getNumArgs() != 1) + if (CE->getNumArgs() != 1 && CE->getNumArgs() != 2) return; if (FName == "pthread_mutex_lock" || @@ -84,6 +112,9 @@ void PthreadLockChecker::checkPostStmt(const CallExpr *CE, FName == "lck_mtx_unlock" || FName == "lck_rw_done") ReleaseLock(C, CE, state->getSVal(CE->getArg(0), LCtx)); + else if (FName == "pthread_mutex_destroy" || + FName == "lck_mtx_destroy") + DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx)); } void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE, @@ -102,18 +133,24 @@ void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE, DefinedSVal retVal = X.castAs(); - if (state->contains(lockR)) { - if (!BT_doublelock) - BT_doublelock.reset(new BugType(this, "Double locking", "Lock checker")); - ExplodedNode *N = C.generateSink(); - if (!N) + if (const LockState *LState = state->get(lockR)) { + if (LState->isLocked()) { + if (!BT_doublelock) + BT_doublelock.reset(new BugType(this, "Double locking", + "Lock checker")); + ExplodedNode *N = C.generateSink(); + if (!N) + return; + BugReport *report = new BugReport(*BT_doublelock, + "This lock has already been acquired", + N); + report->addRange(CE->getArg(0)->getSourceRange()); + C.emitReport(report); return; - BugReport *report = new BugReport(*BT_doublelock, - "This lock has already " - "been acquired", N); - report->addRange(CE->getArg(0)->getSourceRange()); - C.emitReport(report); - return; + } else if (LState->isDestroyed()) { + reportUseDestroyedBug(C, CE); + return; + } } ProgramStateRef lockSucc = state; @@ -146,7 +183,7 @@ void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE, // Record that the lock was acquired. lockSucc = lockSucc->add(lockR); - lockSucc = lockSucc->remove(lockR); + lockSucc = lockSucc->set(lockR, LockState::getLocked()); C.addTransition(lockSucc); } @@ -159,19 +196,24 @@ void PthreadLockChecker::ReleaseLock(CheckerContext &C, const CallExpr *CE, ProgramStateRef state = C.getState(); - if (state->contains(lockR)) { - if (!BT_doubleunlock) - BT_doubleunlock.reset(new BugType(this, "Double unlocking", - "Lock checker")); - ExplodedNode *N = C.generateSink(); - if (!N) + if (const LockState *LState = state->get(lockR)) { + if (LState->isUnlocked()) { + if (!BT_doubleunlock) + BT_doubleunlock.reset(new BugType(this, "Double unlocking", + "Lock checker")); + ExplodedNode *N = C.generateSink(); + if (!N) + return; + BugReport *Report = new BugReport(*BT_doubleunlock, + "This lock has already been unlocked", + N); + Report->addRange(CE->getArg(0)->getSourceRange()); + C.emitReport(Report); return; - BugReport *Report = new BugReport(*BT_doubleunlock, - "This lock has already been unlocked", - N); - Report->addRange(CE->getArg(0)->getSourceRange()); - C.emitReport(Report); - return; + } else if (LState->isDestroyed()) { + reportUseDestroyedBug(C, CE); + return; + } } LockSetTy LS = state->get(); @@ -195,14 +237,64 @@ void PthreadLockChecker::ReleaseLock(CheckerContext &C, const CallExpr *CE, C.emitReport(report); return; } + // Record that the lock was released. + state = state->set(LS.getTail()); } - // Record that the lock was released. - state = state->set(LS.getTail()); - state = state->add(lockR); + state = state->set(lockR, LockState::getUnlocked()); C.addTransition(state); } +void PthreadLockChecker::DestroyLock(CheckerContext &C, const CallExpr *CE, + SVal Lock) const { + + const MemRegion *LockR = Lock.getAsRegion(); + if (!LockR) + return; + + ProgramStateRef State = C.getState(); + + const LockState *LState = State->get(LockR); + if (!LState || LState->isUnlocked()) { + State = State->set(LockR, LockState::getDestroyed()); + C.addTransition(State); + return; + } + + StringRef Message; + + if (LState->isLocked()) { + Message = "This lock is still locked"; + } else { + Message = "This lock has already been destroyed"; + } + + if (!BT_destroylock) + BT_destroylock.reset(new BugType(this, "Destroy invalid lock", + "Lock checker")); + ExplodedNode *N = C.generateSink(); + if (!N) + return; + BugReport *Report = new BugReport(*BT_destroylock, Message, N); + Report->addRange(CE->getArg(0)->getSourceRange()); + C.emitReport(Report); +} + +void PthreadLockChecker::reportUseDestroyedBug(CheckerContext &C, + const CallExpr *CE) const { + if (!BT_destroylock) + BT_destroylock.reset(new BugType(this, "Use destroyed lock", + "Lock checker")); + ExplodedNode *N = C.generateSink(); + if (!N) + return; + BugReport *Report = new BugReport(*BT_destroylock, + "This lock has already been destroyed", + N); + Report->addRange(CE->getArg(0)->getSourceRange()); + C.emitReport(Report); +} + void ento::registerPthreadLockChecker(CheckerManager &mgr) { mgr.registerChecker(); } diff --git a/test/Analysis/pthreadlock.c b/test/Analysis/pthreadlock.c index bc7ef66353..6a75a6e480 100644 --- a/test/Analysis/pthreadlock.c +++ b/test/Analysis/pthreadlock.c @@ -6,17 +6,24 @@ typedef struct { void *foo; } pthread_mutex_t; +typedef struct { + void *foo; +} lck_grp_t; + typedef pthread_mutex_t lck_mtx_t; 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 lck_mtx_lock(lck_mtx_t *); extern int lck_mtx_unlock(lck_mtx_t *); extern int lck_mtx_try_lock(lck_mtx_t *); +extern void lck_mtx_destroy(lck_mtx_t *lck, lck_grp_t *grp); pthread_mutex_t mtx1, mtx2; lck_mtx_t lck1, lck2; +lck_grp_t grp1; void ok1(void) @@ -93,6 +100,43 @@ ok10(void) pthread_mutex_unlock(&mtx1); // no-warning } +void +ok11(void) +{ + pthread_mutex_destroy(&mtx1); // no-warning +} + +void +ok12(void) +{ + pthread_mutex_destroy(&mtx1); // no-warning + pthread_mutex_destroy(&mtx2); // no-warning +} + +void +ok13(void) +{ + pthread_mutex_unlock(&mtx1); // no-warning + pthread_mutex_destroy(&mtx1); // no-warning +} + +void +ok14(void) +{ + pthread_mutex_unlock(&mtx1); // no-warning + pthread_mutex_destroy(&mtx1); // no-warning + pthread_mutex_unlock(&mtx2); // no-warning + pthread_mutex_destroy(&mtx2); // no-warning +} + +void +ok15(void) +{ + pthread_mutex_lock(&mtx1); // no-warning + pthread_mutex_unlock(&mtx1); // no-warning + pthread_mutex_destroy(&mtx1); // no-warning +} + void bad1(void) { @@ -231,3 +275,59 @@ bad15(void) pthread_mutex_lock(&mtx1); // no-warning pthread_mutex_unlock(&mtx2); // expected-warning{{This lock has already been unlocked}} } + +void +bad16(void) +{ + pthread_mutex_destroy(&mtx1); // no-warning + pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}} +} + +void +bad17(void) +{ + pthread_mutex_destroy(&mtx1); // no-warning + pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been destroyed}} +} + +void +bad18(void) +{ + pthread_mutex_destroy(&mtx1); // no-warning + pthread_mutex_destroy(&mtx1); // expected-warning{{This lock has already been destroyed}} +} + +void +bad19(void) +{ + pthread_mutex_lock(&mtx1); // no-warning + pthread_mutex_destroy(&mtx1); // expected-warning{{This lock is still locked}} +} + +void +bad20(void) +{ + lck_mtx_destroy(&mtx1, &grp1); // no-warning + lck_mtx_lock(&mtx1); // expected-warning{{This lock has already been destroyed}} +} + +void +bad21(void) +{ + lck_mtx_destroy(&mtx1, &grp1); // no-warning + lck_mtx_unlock(&mtx1); // expected-warning{{This lock has already been destroyed}} +} + +void +bad22(void) +{ + lck_mtx_destroy(&mtx1, &grp1); // no-warning + lck_mtx_destroy(&mtx1, &grp1); // expected-warning{{This lock has already been destroyed}} +} + +void +bad23(void) +{ + lck_mtx_lock(&mtx1); // no-warning + lck_mtx_destroy(&mtx1, &grp1); // expected-warning{{This lock is still locked}} +}