[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)
Arseniy Zaostrovnykh via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 28 00:24:50 PDT 2024
https://github.com/necto updated https://github.com/llvm/llvm-project/pull/106240
>From 0c86e46516466f9513652a04ba87aa2a018ff6b8 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Tue, 27 Aug 2024 17:52:25 +0200
Subject: [PATCH 1/8] [analyzer] Fix false positive for mutexes inheriting
mutex_base
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 #104241
CPP-5541
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 6 ++++--
.../Analysis/block-in-critical-section-inheritance.cpp | 10 ++++++++++
2 files changed, 14 insertions(+), 2 deletions(-)
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 at -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
+}
>From 013a6d138b38c51c64860a99b95591491f86c223 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 08:27:50 +0200
Subject: [PATCH 2/8] Record a false negative associated with the new mutex
canonicalization
---
.../block-in-critical-section-inheritance.cpp | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index c60ba2632cee25..5f6fa565f42fb9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -39,3 +39,17 @@ void no_false_positive_gh_104241() {
m.unlock();
sleep(10); // no-warning
}
+
+struct TwoMutexes {
+ std::mutex m1;
+ std::mutex m2;
+};
+
+void two_mutexes_false_negative(TwoMutexes &tm) {
+ tm.m1.lock();
+ tm.m2.unlock();
+ // Critical section is associated with tm now so tm.m1 and tm.m2 are
+ // undistinguishiable
+ sleep(10); // False-negative
+ tm.m1.unlock();
+}
>From 744272e0f1ccd5217c0d456a7c499a9bcae84679 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 08:33:02 +0200
Subject: [PATCH 3/8] Fix false negative for field regions
---
.../Checkers/BlockInCriticalSectionChecker.cpp | 9 ++++++++-
.../Analysis/block-in-critical-section-inheritance.cpp | 9 +++++----
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 52ff639c6b8022..fd8700902c1835 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -241,13 +241,20 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const CallEvent &Call,
return std::nullopt;
}
+static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
+ while (const auto *BaseClassRegion = dyn_cast<CXXBaseObjectRegion>(Reg)) {
+ Reg = BaseClassRegion->getSuperRegion();
+ }
+ return Reg;
+}
+
static const MemRegion *getRegion(const CallEvent &Call,
const MutexDescriptor &Descriptor,
bool IsLock) {
return std::visit(
[&Call, IsLock](auto &Descr) -> const MemRegion * {
if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
- return Reg->getBaseRegion();
+ return skipBaseClassRegion(Reg);
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 5f6fa565f42fb9..9db627b75a77f5 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -45,11 +45,12 @@ struct TwoMutexes {
std::mutex m2;
};
-void two_mutexes_false_negative(TwoMutexes &tm) {
+void two_mutexes_no_false_negative(TwoMutexes &tm) {
tm.m1.lock();
+ // expected-note at -1 {{Entering critical section here}}
tm.m2.unlock();
- // Critical section is associated with tm now so tm.m1 and tm.m2 are
- // undistinguishiable
- sleep(10); // False-negative
+ sleep(10);
+ // expected-warning at -1 {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note at -2 {{Call to blocking function 'sleep' inside of critical section}}
tm.m1.unlock();
}
>From 6dc5002e2a0e3552aeadf3489c035304f4864b36 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 08:56:59 +0200
Subject: [PATCH 4/8] Document false negatives on multiple inheritance
---
.../block-in-critical-section-inheritance.cpp | 74 +++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index 9db627b75a77f5..753a19dcc72e60 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -54,3 +54,77 @@ void two_mutexes_no_false_negative(TwoMutexes &tm) {
// expected-note at -2 {{Call to blocking function 'sleep' inside of critical section}}
tm.m1.unlock();
}
+
+struct MyMutexBase1 : std::mutex {
+ void lock1() { lock(); }
+ void unlock1() { unlock(); }
+};
+struct MyMutexBase2 : std::mutex {
+ void lock2() { lock(); }
+ void unlock2() { unlock(); }
+};
+struct MyMutex : MyMutexBase1, MyMutexBase2 {};
+// MyMutex has two distinct std::mutex as base classes
+
+void custom_mutex_tp(MyMutexBase1 &mb) {
+ mb.lock();
+ // expected-note at -1 {{Entering critical section here}}
+ sleep(10);
+ // expected-warning at -1 {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note at -2 {{Call to blocking function 'sleep' inside of critical section}}
+ mb.unlock();
+}
+
+void custom_mutex_tn(MyMutexBase1 &mb) {
+ mb.lock();
+ mb.unlock();
+ sleep(10);
+}
+
+void custom_mutex_cast_tp(MyMutexBase1 &mb) {
+ static_cast<std::mutex&>(mb).lock();
+ // expected-note at -1 {{Entering critical section here}}
+ sleep(10);
+ // expected-warning at -1 {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note at -2 {{Call to blocking function 'sleep' inside of critical section}}
+ static_cast<std::mutex&>(mb).unlock();
+}
+
+void custom_mutex_cast_tn(MyMutexBase1 &mb) {
+ static_cast<std::mutex&>(mb).lock();
+ static_cast<std::mutex&>(mb).unlock();
+ sleep(10);
+}
+
+void two_custom_mutex_bases_fn(MyMutex &m) {
+ m.lock1();
+ m.unlock2();
+ // The critical section is associated with `m` ignoring the fact that m holds
+ // two mutexes. hense unlock2 is considered to unlock the same mutex as lock1
+ // locks
+ sleep(10); // False negative
+ m.unlock1();
+}
+
+void two_custom_mutex_bases_tn(MyMutex &m) {
+ m.lock1();
+ m.unlock1();
+ sleep(10);
+}
+
+void two_custom_mutex_bases_casts_fn(MyMutex &m) {
+ static_cast<MyMutexBase1&>(m).lock();
+ static_cast<MyMutexBase2&>(m).unlock();
+ // The critical section is associated with `m` ignoring the fact that m holds
+ // two mutexes. hense MyMutexBase2::unlock is considered to unlock the same
+ // mutex as MyMutexBase1::lock locks
+ sleep(10); // False negative
+ static_cast<MyMutexBase1&>(m).unlock();
+}
+
+void two_custom_mutex_bases_casts_tn(MyMutex &m) {
+ static_cast<MyMutexBase1&>(m).lock();
+ static_cast<MyMutexBase1&>(m).unlock();
+ sleep(10);
+}
+
>From 97c685c0ff270f1fcfc6514f2efbbc4e621165fd Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 09:06:34 +0200
Subject: [PATCH 5/8] Test cases for virtual inheritance
---
.../block-in-critical-section-inheritance.cpp | 23 +++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index 753a19dcc72e60..65d4426d3c4031 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -128,3 +128,26 @@ void two_custom_mutex_bases_casts_tn(MyMutex &m) {
sleep(10);
}
+struct MutexVirtBase1 : virtual std::mutex {
+ void lock1() { lock(); }
+ void unlock1() { unlock(); }
+};
+
+struct MutexVirtBase2 : virtual std::mutex {
+ void lock2() { lock(); }
+ void unlock2() { unlock(); }
+};
+
+struct CombinedVirtMutex : MutexVirtBase1, MutexVirtBase2 {};
+
+void virt_inherited_mutexes_same_base_tn1(CombinedVirtMutex &cvt) {
+ cvt.lock1();
+ cvt.unlock1();
+ sleep(10);
+}
+
+void virt_inherited_mutexes_different_bases_tn(CombinedVirtMutex &cvt) {
+ cvt.lock1();
+ cvt.unlock2(); // Despite a different name, unlock2 acts on the same mutex as lock1
+ sleep(10);
+}
>From 0d9558316c1d4faba22dd5ae273d1deac02dc4f3 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 09:12:18 +0200
Subject: [PATCH 6/8] Add test case from issue #104241
---
.../block-in-critical-section-inheritance.cpp | 29 +++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index 65d4426d3c4031..c5323d965b2458 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -151,3 +151,32 @@ void virt_inherited_mutexes_different_bases_tn(CombinedVirtMutex &cvt) {
cvt.unlock2(); // Despite a different name, unlock2 acts on the same mutex as lock1
sleep(10);
}
+namespace std {
+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
+
+namespace gh_104241 {
+int magic_number;
+std::mutex m;
+
+void fixed() {
+ int current;
+ for (int items_processed = 0; items_processed < 100; ++items_processed) {
+ {
+ std::scoped_lock<std::mutex> guard(m);
+ current = magic_number;
+ }
+ sleep(current); // expected no warning
+ }
+}
+} // namespace gh_104241
>From 23e11853e7446e998e073a03cc7af0f8b1b26095 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 09:22:29 +0200
Subject: [PATCH 7/8] Fix multiple-inheritance false negative
---
.../BlockInCriticalSectionChecker.cpp | 12 ++++++----
.../block-in-critical-section-inheritance.cpp | 22 ++++++++++---------
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index fd8700902c1835..07f860c95f8777 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -241,10 +241,14 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const CallEvent &Call,
return std::nullopt;
}
-static const MemRegion *skipBaseClassRegion(const MemRegion *Reg) {
- while (const auto *BaseClassRegion = dyn_cast<CXXBaseObjectRegion>(Reg)) {
+static const MemRegion *skipStdBaseClassRegion(const MemRegion *Reg) {
+ do {
+ assert(Reg);
+ const auto *BaseClassRegion = dyn_cast<CXXBaseObjectRegion>(Reg);
+ if (!BaseClassRegion || !BaseClassRegion->getDecl()->isInStdNamespace())
+ break;
Reg = BaseClassRegion->getSuperRegion();
- }
+ } while (true);
return Reg;
}
@@ -254,7 +258,7 @@ static const MemRegion *getRegion(const CallEvent &Call,
return std::visit(
[&Call, IsLock](auto &Descr) -> const MemRegion * {
if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
- return skipBaseClassRegion(Reg);
+ return skipStdBaseClassRegion(Reg);
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 c5323d965b2458..31565c6ef52bc9 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -57,6 +57,7 @@ void two_mutexes_no_false_negative(TwoMutexes &tm) {
struct MyMutexBase1 : std::mutex {
void lock1() { lock(); }
+ // expected-note at -1 {{Entering critical section here}}
void unlock1() { unlock(); }
};
struct MyMutexBase2 : std::mutex {
@@ -96,13 +97,14 @@ void custom_mutex_cast_tn(MyMutexBase1 &mb) {
sleep(10);
}
-void two_custom_mutex_bases_fn(MyMutex &m) {
+void two_custom_mutex_bases_tp(MyMutex &m) {
m.lock1();
+ // expected-note at -1 {{Calling 'MyMutexBase1::lock1'}}
+ // expected-note at -2 {{Returning from 'MyMutexBase1::lock1'}}
m.unlock2();
- // The critical section is associated with `m` ignoring the fact that m holds
- // two mutexes. hense unlock2 is considered to unlock the same mutex as lock1
- // locks
- sleep(10); // False negative
+ sleep(10);
+ // expected-warning at -1 {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note at -2 {{Call to blocking function 'sleep' inside of critical section}}
m.unlock1();
}
@@ -112,13 +114,13 @@ void two_custom_mutex_bases_tn(MyMutex &m) {
sleep(10);
}
-void two_custom_mutex_bases_casts_fn(MyMutex &m) {
+void two_custom_mutex_bases_casts_tp(MyMutex &m) {
static_cast<MyMutexBase1&>(m).lock();
+ // expected-note at -1 {{Entering critical section here}}
static_cast<MyMutexBase2&>(m).unlock();
- // The critical section is associated with `m` ignoring the fact that m holds
- // two mutexes. hense MyMutexBase2::unlock is considered to unlock the same
- // mutex as MyMutexBase1::lock locks
- sleep(10); // False negative
+ sleep(10);
+ // expected-warning at -1 {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note at -2 {{Call to blocking function 'sleep' inside of critical section}}
static_cast<MyMutexBase1&>(m).unlock();
}
>From 629889b2d7c8bb313f8d9ecdb86e91e9cf95f1ba Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 09:24:25 +0200
Subject: [PATCH 8/8] Add tp case for virtual inheritance
---
.../block-in-critical-section-inheritance.cpp | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/clang/test/Analysis/block-in-critical-section-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index 31565c6ef52bc9..4b674e07436fc5 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -132,6 +132,7 @@ void two_custom_mutex_bases_casts_tn(MyMutex &m) {
struct MutexVirtBase1 : virtual std::mutex {
void lock1() { lock(); }
+ // expected-note at -1 {{Entering critical section here}}
void unlock1() { unlock(); }
};
@@ -153,6 +154,17 @@ void virt_inherited_mutexes_different_bases_tn(CombinedVirtMutex &cvt) {
cvt.unlock2(); // Despite a different name, unlock2 acts on the same mutex as lock1
sleep(10);
}
+
+void virt_inherited_mutexes_different_bases_tp(CombinedVirtMutex &cvt) {
+ cvt.lock1();
+ // expected-note at -1 {{Calling 'MutexVirtBase1::lock1'}}
+ // expected-note at -2 {{Returning from 'MutexVirtBase1::lock1'}}
+ sleep(10);
+ // expected-warning at -1 {{Call to blocking function 'sleep' inside of critical section}}
+ // expected-note at -2 {{Call to blocking function 'sleep' inside of critical section}}
+ cvt.unlock1();
+}
+
namespace std {
template <class... MutexTypes> struct scoped_lock {
explicit scoped_lock(MutexTypes&... m);
More information about the cfe-commits
mailing list