Skip to content

Commit 7791593

Browse files
haoNoQhaoNoQ
committedMay 29, 2017
[analyzer] PthreadLockChecker: model failed pthread_mutex_destroy() calls.
pthread_mutex_destroy() may fail, returning a non-zero error number, and keeping the mutex untouched. The mutex can be used on the execution branch that follows such failure, so the analyzer shouldn't warn on using a mutex that was previously destroyed, when in fact the destroy call has failed. Patch by Malhar Thakkar! Differential revision: https://reviews.llvm.org/D32449 llvm-svn: 304159
·
llvmorg-21-initllvmorg-5.0.0-rc1
1 parent f3b84c4 commit 7791593

File tree

2 files changed

+212
-13
lines changed

2 files changed

+212
-13
lines changed
 

Diff for: ‎clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

+133-13
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,13 @@ using namespace ento;
2525
namespace {
2626

2727
struct LockState {
28-
enum Kind { Destroyed, Locked, Unlocked } K;
28+
enum Kind {
29+
Destroyed,
30+
Locked,
31+
Unlocked,
32+
UntouchedAndPossiblyDestroyed,
33+
UnlockedAndPossiblyDestroyed
34+
} K;
2935

3036
private:
3137
LockState(Kind K) : K(K) {}
@@ -34,6 +40,12 @@ struct LockState {
3440
static LockState getLocked() { return LockState(Locked); }
3541
static LockState getUnlocked() { return LockState(Unlocked); }
3642
static LockState getDestroyed() { return LockState(Destroyed); }
43+
static LockState getUntouchedAndPossiblyDestroyed() {
44+
return LockState(UntouchedAndPossiblyDestroyed);
45+
}
46+
static LockState getUnlockedAndPossiblyDestroyed() {
47+
return LockState(UnlockedAndPossiblyDestroyed);
48+
}
3749

3850
bool operator==(const LockState &X) const {
3951
return K == X.K;
@@ -42,13 +54,20 @@ struct LockState {
4254
bool isLocked() const { return K == Locked; }
4355
bool isUnlocked() const { return K == Unlocked; }
4456
bool isDestroyed() const { return K == Destroyed; }
57+
bool isUntouchedAndPossiblyDestroyed() const {
58+
return K == UntouchedAndPossiblyDestroyed;
59+
}
60+
bool isUnlockedAndPossiblyDestroyed() const {
61+
return K == UnlockedAndPossiblyDestroyed;
62+
}
4563

4664
void Profile(llvm::FoldingSetNodeID &ID) const {
4765
ID.AddInteger(K);
4866
}
4967
};
5068

51-
class PthreadLockChecker : public Checker< check::PostStmt<CallExpr> > {
69+
class PthreadLockChecker
70+
: public Checker<check::PostStmt<CallExpr>, check::DeadSymbols> {
5271
mutable std::unique_ptr<BugType> BT_doublelock;
5372
mutable std::unique_ptr<BugType> BT_doubleunlock;
5473
mutable std::unique_ptr<BugType> BT_destroylock;
@@ -61,22 +80,31 @@ class PthreadLockChecker : public Checker< check::PostStmt<CallExpr> > {
6180
};
6281
public:
6382
void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
83+
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
6484

6585
void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock,
6686
bool isTryLock, enum LockingSemantics semantics) const;
6787

6888
void ReleaseLock(CheckerContext &C, const CallExpr *CE, SVal lock) const;
69-
void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
89+
void DestroyLock(CheckerContext &C, const CallExpr *CE, SVal Lock,
90+
enum LockingSemantics semantics) const;
7091
void InitLock(CheckerContext &C, const CallExpr *CE, SVal Lock) const;
7192
void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE) const;
93+
ProgramStateRef resolvePossiblyDestroyedMutex(ProgramStateRef state,
94+
const MemRegion *lockR,
95+
const SymbolRef *sym) const;
7296
};
7397
} // end anonymous namespace
7498

75-
// GDM Entry for tracking lock state.
99+
// A stack of locks for tracking lock-unlock order.
76100
REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
77101

102+
// An entry for tracking lock states.
78103
REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
79104

105+
// Return values for unresolved calls to pthread_mutex_destroy().
106+
REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef)
107+
80108
void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
81109
CheckerContext &C) const {
82110
ProgramStateRef state = C.getState();
@@ -113,13 +141,49 @@ void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
113141
FName == "lck_mtx_unlock" ||
114142
FName == "lck_rw_done")
115143
ReleaseLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
116-
else if (FName == "pthread_mutex_destroy" ||
117-
FName == "lck_mtx_destroy")
118-
DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
144+
else if (FName == "pthread_mutex_destroy")
145+
DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), PthreadSemantics);
146+
else if (FName == "lck_mtx_destroy")
147+
DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), XNUSemantics);
119148
else if (FName == "pthread_mutex_init")
120149
InitLock(C, CE, state->getSVal(CE->getArg(0), LCtx));
121150
}
122151

152+
// When a lock is destroyed, in some semantics(like PthreadSemantics) we are not
153+
// sure if the destroy call has succeeded or failed, and the lock enters one of
154+
// the 'possibly destroyed' state. There is a short time frame for the
155+
// programmer to check the return value to see if the lock was successfully
156+
// destroyed. Before we model the next operation over that lock, we call this
157+
// function to see if the return value was checked by now and set the lock state
158+
// - either to destroyed state or back to its previous state.
159+
160+
// In PthreadSemantics, pthread_mutex_destroy() returns zero if the lock is
161+
// successfully destroyed and it returns a non-zero value otherwise.
162+
ProgramStateRef PthreadLockChecker::resolvePossiblyDestroyedMutex(
163+
ProgramStateRef state, const MemRegion *lockR, const SymbolRef *sym) const {
164+
const LockState *lstate = state->get<LockMap>(lockR);
165+
// Existence in DestroyRetVal ensures existence in LockMap.
166+
// Existence in Destroyed also ensures that the lock state for lockR is either
167+
// UntouchedAndPossiblyDestroyed or UnlockedAndPossiblyDestroyed.
168+
assert(lstate->isUntouchedAndPossiblyDestroyed() ||
169+
lstate->isUnlockedAndPossiblyDestroyed());
170+
171+
ConstraintManager &CMgr = state->getConstraintManager();
172+
ConditionTruthVal retZero = CMgr.isNull(state, *sym);
173+
if (retZero.isConstrainedFalse()) {
174+
if (lstate->isUntouchedAndPossiblyDestroyed())
175+
state = state->remove<LockMap>(lockR);
176+
else if (lstate->isUnlockedAndPossiblyDestroyed())
177+
state = state->set<LockMap>(lockR, LockState::getUnlocked());
178+
} else
179+
state = state->set<LockMap>(lockR, LockState::getDestroyed());
180+
181+
// Removing the map entry (lockR, sym) from DestroyRetVal as the lock state is
182+
// now resolved.
183+
state = state->remove<DestroyRetVal>(lockR);
184+
return state;
185+
}
186+
123187
void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
124188
SVal lock, bool isTryLock,
125189
enum LockingSemantics semantics) const {
@@ -129,6 +193,9 @@ void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
129193
return;
130194

131195
ProgramStateRef state = C.getState();
196+
const SymbolRef *sym = state->get<DestroyRetVal>(lockR);
197+
if (sym)
198+
state = resolvePossiblyDestroyedMutex(state, lockR, sym);
132199

133200
SVal X = state->getSVal(CE, C.getLocationContext());
134201
if (X.isUnknownOrUndef())
@@ -197,6 +264,9 @@ void PthreadLockChecker::ReleaseLock(CheckerContext &C, const CallExpr *CE,
197264
return;
198265

199266
ProgramStateRef state = C.getState();
267+
const SymbolRef *sym = state->get<DestroyRetVal>(lockR);
268+
if (sym)
269+
state = resolvePossiblyDestroyedMutex(state, lockR, sym);
200270

201271
if (const LockState *LState = state->get<LockMap>(lockR)) {
202272
if (LState->isUnlocked()) {
@@ -245,21 +315,47 @@ void PthreadLockChecker::ReleaseLock(CheckerContext &C, const CallExpr *CE,
245315
}
246316

247317
void PthreadLockChecker::DestroyLock(CheckerContext &C, const CallExpr *CE,
248-
SVal Lock) const {
318+
SVal Lock,
319+
enum LockingSemantics semantics) const {
249320

250321
const MemRegion *LockR = Lock.getAsRegion();
251322
if (!LockR)
252323
return;
253324

254325
ProgramStateRef State = C.getState();
255326

327+
const SymbolRef *sym = State->get<DestroyRetVal>(LockR);
328+
if (sym)
329+
State = resolvePossiblyDestroyedMutex(State, LockR, sym);
330+
256331
const LockState *LState = State->get<LockMap>(LockR);
257-
if (!LState || LState->isUnlocked()) {
258-
State = State->set<LockMap>(LockR, LockState::getDestroyed());
259-
C.addTransition(State);
260-
return;
332+
// Checking the return value of the destroy method only in the case of
333+
// PthreadSemantics
334+
if (semantics == PthreadSemantics) {
335+
if (!LState || LState->isUnlocked()) {
336+
SymbolRef sym = C.getSVal(CE).getAsSymbol();
337+
if (!sym) {
338+
State = State->remove<LockMap>(LockR);
339+
C.addTransition(State);
340+
return;
341+
}
342+
State = State->set<DestroyRetVal>(LockR, sym);
343+
if (LState && LState->isUnlocked())
344+
State = State->set<LockMap>(
345+
LockR, LockState::getUnlockedAndPossiblyDestroyed());
346+
else
347+
State = State->set<LockMap>(
348+
LockR, LockState::getUntouchedAndPossiblyDestroyed());
349+
C.addTransition(State);
350+
return;
351+
}
352+
} else {
353+
if (!LState || LState->isUnlocked()) {
354+
State = State->set<LockMap>(LockR, LockState::getDestroyed());
355+
C.addTransition(State);
356+
return;
357+
}
261358
}
262-
263359
StringRef Message;
264360

265361
if (LState->isLocked()) {
@@ -288,6 +384,10 @@ void PthreadLockChecker::InitLock(CheckerContext &C, const CallExpr *CE,
288384

289385
ProgramStateRef State = C.getState();
290386

387+
const SymbolRef *sym = State->get<DestroyRetVal>(LockR);
388+
if (sym)
389+
State = resolvePossiblyDestroyedMutex(State, LockR, sym);
390+
291391
const struct LockState *LState = State->get<LockMap>(LockR);
292392
if (!LState || LState->isDestroyed()) {
293393
State = State->set<LockMap>(LockR, LockState::getUnlocked());
@@ -328,6 +428,26 @@ void PthreadLockChecker::reportUseDestroyedBug(CheckerContext &C,
328428
C.emitReport(std::move(Report));
329429
}
330430

431+
void PthreadLockChecker::checkDeadSymbols(SymbolReaper &SymReaper,
432+
CheckerContext &C) const {
433+
ProgramStateRef State = C.getState();
434+
435+
// TODO: Clean LockMap when a mutex region dies.
436+
437+
DestroyRetValTy TrackedSymbols = State->get<DestroyRetVal>();
438+
for (DestroyRetValTy::iterator I = TrackedSymbols.begin(),
439+
E = TrackedSymbols.end();
440+
I != E; ++I) {
441+
const SymbolRef Sym = I->second;
442+
const MemRegion *lockR = I->first;
443+
bool IsSymDead = SymReaper.isDead(Sym);
444+
// Remove the dead symbol from the return value symbols map.
445+
if (IsSymDead)
446+
State = resolvePossiblyDestroyedMutex(State, lockR, &Sym);
447+
}
448+
C.addTransition(State);
449+
}
450+
331451
void ento::registerPthreadLockChecker(CheckerManager &mgr) {
332452
mgr.registerChecker<PthreadLockChecker>();
333453
}

Diff for: ‎clang/test/Analysis/pthreadlock.c

+79
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,42 @@ ok22(void) {
176176
pthread_mutex_unlock(pmtx); // no-warning
177177
}
178178

179+
void ok23(void) {
180+
if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
181+
pthread_mutex_destroy(&mtx1); // no-warning
182+
}
183+
184+
void ok24(void) {
185+
if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
186+
pthread_mutex_lock(&mtx1); // no-warning
187+
}
188+
189+
void ok25(void) {
190+
if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
191+
pthread_mutex_unlock(&mtx1); // no-warning
192+
}
193+
194+
void ok26(void) {
195+
pthread_mutex_unlock(&mtx1); // no-warning
196+
if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
197+
pthread_mutex_lock(&mtx1); // no-warning
198+
}
199+
200+
void ok27(void) {
201+
pthread_mutex_unlock(&mtx1); // no-warning
202+
if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
203+
pthread_mutex_lock(&mtx1); // no-warning
204+
else
205+
pthread_mutex_init(&mtx1, NULL); // no-warning
206+
}
207+
208+
void ok28(void) {
209+
if (pthread_mutex_destroy(&mtx1) != 0) { // no-warning
210+
pthread_mutex_lock(&mtx1); // no-warning
211+
pthread_mutex_unlock(&mtx1); // no-warning
212+
pthread_mutex_destroy(&mtx1); // no-warning
213+
}
214+
}
179215

180216
void
181217
bad1(void)
@@ -392,3 +428,46 @@ bad26(void)
392428
pthread_mutex_unlock(&mtx1); // no-warning
393429
pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
394430
}
431+
432+
void bad27(void) {
433+
pthread_mutex_unlock(&mtx1); // no-warning
434+
int ret = pthread_mutex_destroy(&mtx1); // no-warning
435+
if (ret != 0) // no-warning
436+
pthread_mutex_lock(&mtx1); // no-warning
437+
else
438+
pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been destroyed}}
439+
}
440+
441+
void bad28(void) {
442+
pthread_mutex_unlock(&mtx1); // no-warning
443+
int ret = pthread_mutex_destroy(&mtx1); // no-warning
444+
if (ret != 0) // no-warning
445+
pthread_mutex_lock(&mtx1); // no-warning
446+
else
447+
pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}}
448+
}
449+
450+
void bad29(void) {
451+
pthread_mutex_lock(&mtx1); // no-warning
452+
pthread_mutex_unlock(&mtx1); // no-warning
453+
if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
454+
pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
455+
else
456+
pthread_mutex_init(&mtx1, NULL); // no-warning
457+
}
458+
459+
void bad30(void) {
460+
pthread_mutex_lock(&mtx1); // no-warning
461+
pthread_mutex_unlock(&mtx1); // no-warning
462+
if (pthread_mutex_destroy(&mtx1) != 0) // no-warning
463+
pthread_mutex_init(&mtx1, NULL); // expected-warning{{This lock has already been initialized}}
464+
else
465+
pthread_mutex_destroy(&mtx1); // expected-warning{{This lock has already been destroyed}}
466+
}
467+
468+
void bad31(void) {
469+
int ret = pthread_mutex_destroy(&mtx1); // no-warning
470+
pthread_mutex_lock(&mtx1); // expected-warning{{This lock has already been destroyed}}
471+
if (ret != 0)
472+
pthread_mutex_lock(&mtx1);
473+
}

0 commit comments

Comments
 (0)
Please sign in to comment.