[analyzer] Fix false positive for mutexes inheriting mutex_base #106240
Conversation
If a mutex interface is split in inheritance chain, e.g. struct mutex has `unlock` and inherits `lock` from __mutex_base then calls m.lock() and m.unlock() have different "this" targets: m and the __mutex_base of m, which used to confuse the `ActiveCritSections` list. Taking base region canonicalizes the region used to identify a critical section and enables search in ActiveCritSections list regardless of which class the callee is the member of. This possibly fixes llvm#104241 CPP-5541
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Arseniy Zaostrovnykh (necto) ChangesIf a mutex interface is split in inheritance chain, e.g. struct mutex has Taking base region canonicalizes the region used to identify a critical section and enables search in ActiveCritSections list regardless of which class the callee is the member of. This possibly fixes #104241 CPP-5541 Full diff: https://github.com/llvm/llvm-project/pull/106240.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 4cd2f2802f30cd..52ff639c6b8022 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -245,8 +245,10 @@ static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
- [&Call, IsLock](auto &&Descriptor) {
- return Descriptor.getRegion(Call, IsLock);
+ [&Call, IsLock](auto &Descr) -> const MemRegion * {
+ if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
+ return Reg->getBaseRegion();
+ return nullptr;
},
Descriptor);
}
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..c60ba2632cee25 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,13 @@ void gh_99628() {
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
m.unlock();
}
+
+void no_false_positive_gh_104241() {
+ std::mutex m;
+ m.lock();
+ // If inheritance not handled properly, this unlock might not match the lock
+ // above because technically they act on different memory regions:
+ // __mutex_base and mutex.
+ m.unlock();
+ sleep(10); // no-warning
+}
|
relaxed in 744272e |
|
Could you also add this test case? namespace std {
struct __mutex_base {
void lock();
};
struct mutex : __mutex_base {
void unlock();
bool try_lock();
};
template <class... MutexTypes> struct scoped_lock {
explicit scoped_lock(MutexTypes&... m);
~scoped_lock();
};
template <class MutexType> class scoped_lock<MutexType> {
public:
explicit scoped_lock(MutexType& m) : m(m) { m.lock(); }
~scoped_lock() { m.unlock(); }
private:
MutexType& m;
};
} // namespace std
extern "C" unsigned int sleep(unsigned int seconds);
int magic_number;
std::mutex m;
void fixed() {
int current;
for (int items_processed = 0; items_processed < 100; ++items_processed) {
{
std::scoped_lock guard(m);
current = magic_number;
}
sleep(current); // expected no warning
}
}Or is it already implied by other tests? |
My first test void no_false_positive_gh_104241() {
std::mutex m;
m.lock();
// If inheritance not handled properly, this unlock might not match the lock
// above because technically they act on different memory regions:
// __mutex_base and mutex.
m.unlock();
sleep(10); // no-warning
}Was derived from your example, so I guess it is implied, but I added your example as is with 0d95583 to make it extra clear. |
|
@steakhal I added a fix for multiple-inheritance fn, please take another look |
clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
Outdated
Show resolved
Hide resolved
If a mutex interface is split in inheritance chain, e.g. struct mutex has
unlockand inheritslockfrom __mutex_base then calls m.lock() and m.unlock() have different "this" targets: m and the __mutex_base of m, which used to confuse theActiveCritSectionslist.Taking base region canonicalizes the region used to identify a critical section and enables search in ActiveCritSections list regardless of which class the callee is the member of.
This possibly fixes #104241
CPP-5541