Skip to content

Commit 0696bb4

Browse files
jrose-applejrose-apple
committedApr 1, 2014
[analyzer] Add double-unlock detection to PthreadLockChecker.
We've decided to punt on supporting recursive locks for now; the common case is non-recursive. Patch by Daniel Fahlgren! llvm-svn: 205274
·
llvmorg-21-initllvmorg-3.5.0-rc1
1 parent 5557f6d commit 0696bb4

File tree

2 files changed

+132
-17
lines changed

2 files changed

+132
-17
lines changed
 

‎clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp

+36-17
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ using namespace ento;
2626
namespace {
2727
class PthreadLockChecker : public Checker< check::PostStmt<CallExpr> > {
2828
mutable std::unique_ptr<BugType> BT_doublelock;
29+
mutable std::unique_ptr<BugType> BT_doubleunlock;
2930
mutable std::unique_ptr<BugType> BT_lor;
3031
enum LockingSemantics {
3132
NotApplicable = 0,
@@ -45,6 +46,7 @@ class PthreadLockChecker : public Checker< check::PostStmt<CallExpr> > {
4546
// GDM Entry for tracking lock state.
4647
REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
4748

49+
REGISTER_SET_WITH_PROGRAMSTATE(UnlockSet, const MemRegion *)
4850

4951
void PthreadLockChecker::checkPostStmt(const CallExpr *CE,
5052
CheckerContext &C) const {
@@ -144,6 +146,7 @@ void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE,
144146

145147
// Record that the lock was acquired.
146148
lockSucc = lockSucc->add<LockSet>(lockR);
149+
lockSucc = lockSucc->remove<UnlockSet>(lockR);
147150
C.addTransition(lockSucc);
148151
}
149152

@@ -155,32 +158,48 @@ void PthreadLockChecker::ReleaseLock(CheckerContext &C, const CallExpr *CE,
155158
return;
156159

157160
ProgramStateRef state = C.getState();
158-
LockSetTy LS = state->get<LockSet>();
159161

160-
// FIXME: Better analysis requires IPA for wrappers.
161-
// FIXME: check for double unlocks
162-
if (LS.isEmpty())
163-
return;
164-
165-
const MemRegion *firstLockR = LS.getHead();
166-
if (firstLockR != lockR) {
167-
if (!BT_lor)
168-
BT_lor.reset(new BugType(this, "Lock order reversal", "Lock checker"));
162+
if (state->contains<UnlockSet>(lockR)) {
163+
if (!BT_doubleunlock)
164+
BT_doubleunlock.reset(new BugType(this, "Double unlocking",
165+
"Lock checker"));
169166
ExplodedNode *N = C.generateSink();
170167
if (!N)
171168
return;
172-
BugReport *report = new BugReport(*BT_lor,
173-
"This was not the most "
174-
"recently acquired lock. "
175-
"Possible lock order "
176-
"reversal", N);
177-
report->addRange(CE->getArg(0)->getSourceRange());
178-
C.emitReport(report);
169+
BugReport *Report = new BugReport(*BT_doubleunlock,
170+
"This lock has already been unlocked",
171+
N);
172+
Report->addRange(CE->getArg(0)->getSourceRange());
173+
C.emitReport(Report);
179174
return;
180175
}
181176

177+
LockSetTy LS = state->get<LockSet>();
178+
179+
// FIXME: Better analysis requires IPA for wrappers.
180+
181+
if (!LS.isEmpty()) {
182+
const MemRegion *firstLockR = LS.getHead();
183+
if (firstLockR != lockR) {
184+
if (!BT_lor)
185+
BT_lor.reset(new BugType(this, "Lock order reversal", "Lock checker"));
186+
ExplodedNode *N = C.generateSink();
187+
if (!N)
188+
return;
189+
BugReport *report = new BugReport(*BT_lor,
190+
"This was not the most recently "
191+
"acquired lock. Possible lock order "
192+
"reversal",
193+
N);
194+
report->addRange(CE->getArg(0)->getSourceRange());
195+
C.emitReport(report);
196+
return;
197+
}
198+
}
199+
182200
// Record that the lock was released.
183201
state = state->set<LockSet>(LS.getTail());
202+
state = state->add<UnlockSet>(lockR);
184203
C.addTransition(state);
185204
}
186205

‎clang/test/Analysis/pthreadlock.c

+96
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,31 @@ ok7(void)
6868
lck_mtx_unlock(&lck1); // no-warning
6969
}
7070

71+
void
72+
ok8(void)
73+
{
74+
pthread_mutex_lock(&mtx1); // no-warning
75+
pthread_mutex_lock(&mtx2); // no-warning
76+
pthread_mutex_unlock(&mtx2); // no-warning
77+
pthread_mutex_unlock(&mtx1); // no-warning
78+
}
79+
80+
void
81+
ok9(void)
82+
{
83+
pthread_mutex_unlock(&mtx1); // no-warning
84+
if (pthread_mutex_trylock(&mtx1) == 0) // no-warning
85+
pthread_mutex_unlock(&mtx1); // no-warning
86+
}
87+
88+
void
89+
ok10(void)
90+
{
91+
if (pthread_mutex_trylock(&mtx1) != 0) // no-warning
92+
pthread_mutex_lock(&mtx1); // no-warning
93+
pthread_mutex_unlock(&mtx1); // no-warning
94+
}
95+
7196
void
7297
bad1(void)
7398
{
@@ -135,3 +160,74 @@ bad8(void)
135160
lck_mtx_lock(&lck2); // no-warning
136161
lck_mtx_unlock(&lck1); // expected-warning{{This was not the most recently acquired lock}}
137162
}
163+
164+
void
165+
bad9(void)
166+
{
167+
lck_mtx_unlock(&lck1); // no-warning
168+
lck_mtx_unlock(&lck1); // expected-warning{{This lock has already been unlocked}}
169+
}
170+
171+
void
172+
bad10(void)
173+
{
174+
lck_mtx_lock(&lck1); // no-warning
175+
lck_mtx_unlock(&lck1); // no-warning
176+
lck_mtx_unlock(&lck1); // expected-warning{{This lock has already been unlocked}}
177+
}
178+
179+
static void
180+
bad11_sub(pthread_mutex_t *lock)
181+
{
182+
lck_mtx_unlock(lock); // expected-warning{{This lock has already been unlocked}}
183+
}
184+
185+
void
186+
bad11(int i)
187+
{
188+
lck_mtx_lock(&lck1); // no-warning
189+
lck_mtx_unlock(&lck1); // no-warning
190+
if (i < 5)
191+
bad11_sub(&lck1);
192+
}
193+
194+
void
195+
bad12(void)
196+
{
197+
pthread_mutex_lock(&mtx1); // no-warning
198+
pthread_mutex_unlock(&mtx1); // no-warning
199+
pthread_mutex_lock(&mtx1); // no-warning
200+
pthread_mutex_unlock(&mtx1); // no-warning
201+
pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been unlocked}}
202+
}
203+
204+
void
205+
bad13(void)
206+
{
207+
pthread_mutex_lock(&mtx1); // no-warning
208+
pthread_mutex_unlock(&mtx1); // no-warning
209+
pthread_mutex_lock(&mtx2); // no-warning
210+
pthread_mutex_unlock(&mtx2); // no-warning
211+
pthread_mutex_unlock(&mtx1); // expected-warning{{This lock has already been unlocked}}
212+
}
213+
214+
void
215+
bad14(void)
216+
{
217+
pthread_mutex_lock(&mtx1); // no-warning
218+
pthread_mutex_lock(&mtx2); // no-warning
219+
pthread_mutex_unlock(&mtx2); // no-warning
220+
pthread_mutex_unlock(&mtx1); // no-warning
221+
pthread_mutex_unlock(&mtx2); // expected-warning{{This lock has already been unlocked}}
222+
}
223+
224+
void
225+
bad15(void)
226+
{
227+
pthread_mutex_lock(&mtx1); // no-warning
228+
pthread_mutex_lock(&mtx2); // no-warning
229+
pthread_mutex_unlock(&mtx2); // no-warning
230+
pthread_mutex_unlock(&mtx1); // no-warning
231+
pthread_mutex_lock(&mtx1); // no-warning
232+
pthread_mutex_unlock(&mtx2); // expected-warning{{This lock has already been unlocked}}
233+
}

0 commit comments

Comments
 (0)
Please sign in to comment.