[PATCH] D37812: [analyzer] PthreadLock: Escape the pointers.
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdd22be1e3d98: [analyzer] PthreadLock: Implement mutex
escaping. (authored by dergachev.a).
Herald added subscribers: Charusso, dkrupp, donat.nagy, jfb, Szelethus,
mikhail.ramalho, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: clang.
Changed prior to commit:
https://reviews.llvm.org/D37812?vs=115053=240199#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D37812/new/
https://reviews.llvm.org/D37812
Files:
clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
clang/test/Analysis/pthreadlock.c
Index: clang/test/Analysis/pthreadlock.c
===
--- clang/test/Analysis/pthreadlock.c
+++ clang/test/Analysis/pthreadlock.c
@@ -221,6 +221,27 @@
lck_rw_unlock_exclusive(); // no-warning
}
+void escape_mutex(pthread_mutex_t *m);
+void ok30(void) {
+ pthread_mutex_t local_mtx;
+ pthread_mutex_init(_mtx, NULL);
+ pthread_mutex_lock(_mtx);
+ escape_mutex(_mtx);
+ pthread_mutex_lock(_mtx); // no-warning
+ pthread_mutex_unlock(_mtx);
+ pthread_mutex_destroy(_mtx);
+}
+
+void ok31(void) {
+ pthread_mutex_t local_mtx;
+ pthread_mutex_init(_mtx, NULL);
+ pthread_mutex_lock(_mtx);
+ fake_system_function_that_takes_a_mutex(_mtx);
+ pthread_mutex_lock(_mtx); // no-warning
+ pthread_mutex_unlock(_mtx);
+ pthread_mutex_destroy(_mtx);
+}
+
void
bad1(void)
{
@@ -486,3 +507,9 @@
lck_rw_lock_exclusive();
lck_rw_unlock_shared(); // 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}}
+}
Index: clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
===
--- clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
+++ clang/test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
@@ -21,6 +21,9 @@
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 *);
Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -66,7 +66,8 @@
};
class PthreadLockChecker
-: public Checker {
+: public Checker {
BugType BT_doublelock{this, "Double locking", "Lock checker"},
BT_doubleunlock{this, "Double unlocking", "Lock checker"},
BT_destroylock{this, "Use destroyed lock", "Lock checker"},
@@ -155,6 +156,11 @@
public:
void checkPostCall(const CallEvent , CheckerContext ) const;
void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+ ProgramStateRef
+ checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *Symbols,
+ ArrayRef ExplicitRegions,
+ ArrayRef Regions,
+ const LocationContext *LCtx, const CallEvent *Call) const;
void printState(raw_ostream , ProgramStateRef State,
const char *NL, const char *Sep) const override;
};
@@ -546,6 +552,42 @@
C.addTransition(State);
}
+ProgramStateRef PthreadLockChecker::checkRegionChanges(
+ProgramStateRef State, const InvalidatedSymbols *Symbols,
+ArrayRef ExplicitRegions,
+ArrayRef 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(R);
+State = State->remove(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 ) {
[PATCH] D37812: [analyzer] PthreadLock: Escape the pointers.
a.sidorin added a comment. Hi Artem. The patch looks mostly good, but I have an inline question. Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:588 +if (Call->isInSystemHeader()) + IsLibraryFunction = true; + } Do we think that only system headers contain library functions? Shouldn't we use `CheckerContext::isCLibraryFunction()` instead? https://reviews.llvm.org/D37812 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37812: [analyzer] PthreadLock: Escape the pointers.
xazax.hun added inline comments. Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:594 +// it takes the mutex explicitly as an argument. +if (IsLibraryFunction && +std::find(ExplicitRegions.begin(), ExplicitRegions.end(), R) == 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. https://reviews.llvm.org/D37812 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37812: [analyzer] PthreadLock: Escape the pointers.
NoQ created this revision.
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.
https://reviews.llvm.org/D37812
Files:
lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
test/Analysis/pthreadlock.c
Index: test/Analysis/pthreadlock.c
===
--- test/Analysis/pthreadlock.c
+++ test/Analysis/pthreadlock.c
@@ -221,6 +221,27 @@
lck_rw_unlock_exclusive(); // no-warning
}
+void escape_mutex(pthread_mutex_t *m);
+void ok30(void) {
+ pthread_mutex_t local_mtx;
+ pthread_mutex_init(_mtx, NULL);
+ pthread_mutex_lock(_mtx);
+ escape_mutex(_mtx);
+ pthread_mutex_lock(_mtx); // no-warning
+ pthread_mutex_unlock(_mtx);
+ pthread_mutex_destroy(_mtx);
+}
+
+void ok31(void) {
+ pthread_mutex_t local_mtx;
+ pthread_mutex_init(_mtx, NULL);
+ pthread_mutex_lock(_mtx);
+ fake_system_function_that_takes_a_mutex(_mtx);
+ pthread_mutex_lock(_mtx); // no-warning
+ pthread_mutex_unlock(_mtx);
+ pthread_mutex_destroy(_mtx);
+}
+
void
bad1(void)
{
@@ -486,3 +507,9 @@
lck_rw_lock_exclusive();
lck_rw_unlock_shared(); // 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}}
+}
Index: test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
===
--- test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
+++ test/Analysis/Inputs/system-header-simulator-for-pthread-lock.h
@@ -3,6 +3,8 @@
// should not invalidate regions of their arguments.
#pragma clang system_header
+extern void fake_system_function();
+
typedef struct {
void *foo;
} OpaqueThing;
@@ -12,6 +14,8 @@
typedef OpaqueThing pthread_rwlock_t;
typedef OpaqueThing pthread_mutexattr_t;
+extern void fake_system_function_that_takes_a_mutex(pthread_mutex_t *mtx);
+
// XNU.
typedef OpaqueThing lck_mtx_t;
typedef OpaqueThing lck_rw_t;
Index: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -66,8 +66,8 @@
}
};
-class PthreadLockChecker
-: public Checker {
+class PthreadLockChecker : public Checker {
mutable std::unique_ptr BT_doublelock;
mutable std::unique_ptr BT_doubleunlock;
mutable std::unique_ptr BT_destroylock;
@@ -124,6 +124,11 @@
void checkPostCall(const CallEvent , CheckerContext ) const;
void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
+ ProgramStateRef
+ checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *Symbols,
+ ArrayRef ExplicitRegions,
+ ArrayRef Regions,
+ const LocationContext *LCtx, const CallEvent *Call) const;
void printState(raw_ostream , ProgramStateRef State,
const char *NL, const char *Sep) const override;
};
@@ -567,6 +572,38 @@
C.addTransition(State);
}
+ProgramStateRef PthreadLockChecker::checkRegionChanges(
+ProgramStateRef State, const InvalidatedSymbols *Symbols,
+ArrayRef ExplicitRegions,
+ArrayRef 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.
+for (auto I : SupportedAPIs)
+ if (Call->isCalled(I.Name))
+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.
+if (IsLibraryFunction &&
+std::find(ExplicitRegions.begin(), ExplicitRegions.end(), R) ==
+ExplicitRegions.end())
+ continue;
+
+State = State->remove(R);
+State = State->remove(R);
+// TODO: Destroy the lock stack as well.
+ }
+
+ return State;
+}
+
void ento::registerPthreadLockChecker(CheckerManager ) {
mgr.registerChecker();
}
