[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 02:00:36 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 01/10] [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 02/10] 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 03/10] 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 04/10] 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 05/10] 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 06/10] 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 07/10] 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 08/10] 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);

>From 244e04076b3ce330c3c1baf0e475d57cd0c879d1 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 09:26:34 +0200
Subject: [PATCH 09/10] [NFC] Replace do {} while with a usual while {} loop

---
 .../Checkers/BlockInCriticalSectionChecker.cpp               | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 07f860c95f8777..5405bdb0706c3e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -242,13 +242,12 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const CallEvent &Call,
 }
 
 static const MemRegion *skipStdBaseClassRegion(const MemRegion *Reg) {
-  do {
-    assert(Reg);
+  while (Reg) {
     const auto *BaseClassRegion = dyn_cast<CXXBaseObjectRegion>(Reg);
     if (!BaseClassRegion || !BaseClassRegion->getDecl()->isInStdNamespace())
       break;
     Reg = BaseClassRegion->getSuperRegion();
-  } while (true);
+  }
   return Reg;
 }
 

>From e94327b9fda5f9d21c1d8a6b8a43ddb103ae4921 Mon Sep 17 00:00:00 2001
From: Arseniy Zaostrovnykh <necto.ne at gmail.com>
Date: Wed, 28 Aug 2024 10:59:52 +0200
Subject: [PATCH 10/10] Fix FN for mutexes with base class declared in a nested
 namespace

---
 .../Core/PathSensitive/CheckerHelpers.h       |  6 ++-
 .../BlockInCriticalSectionChecker.cpp         |  7 ++--
 clang/lib/StaticAnalyzer/Core/CallEvent.cpp   | 12 +-----
 .../StaticAnalyzer/Core/CheckerHelpers.cpp    | 11 +++++
 ...k-in-critical-section-nested-namespace.cpp | 42 +++++++++++++++++++
 5 files changed, 62 insertions(+), 16 deletions(-)
 create mode 100644 clang/test/Analysis/block-in-critical-section-nested-namespace.cpp

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
index d053a97189123a..b4afaaeec9a4bd 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
@@ -6,7 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 //
-//  This file defines CheckerVisitor.
+//  This file defines various utilities used by checkers.
 //
 //===----------------------------------------------------------------------===//
 
@@ -114,6 +114,10 @@ OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
 
 std::optional<SVal> getPointeeVal(SVal PtrSVal, ProgramStateRef State);
 
+/// Returns true if declaration \p D is in std namespace or any nested namespace
+/// or class scope.
+bool isWithinStdNamespace(const Decl *D);
+
 } // namespace ento
 
 } // namespace clang
diff --git a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
index 5405bdb0706c3e..7460781799d08a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
@@ -20,6 +20,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
@@ -244,7 +245,7 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const CallEvent &Call,
 static const MemRegion *skipStdBaseClassRegion(const MemRegion *Reg) {
   while (Reg) {
     const auto *BaseClassRegion = dyn_cast<CXXBaseObjectRegion>(Reg);
-    if (!BaseClassRegion || !BaseClassRegion->getDecl()->isInStdNamespace())
+    if (!BaseClassRegion || !isWithinStdNamespace(BaseClassRegion->getDecl()))
       break;
     Reg = BaseClassRegion->getSuperRegion();
   }
@@ -256,9 +257,7 @@ static const MemRegion *getRegion(const CallEvent &Call,
                                   bool IsLock) {
   return std::visit(
       [&Call, IsLock](auto &Descr) -> const MemRegion * {
-        if (const MemRegion *Reg = Descr.getRegion(Call, IsLock))
-          return skipStdBaseClassRegion(Reg);
-        return nullptr;
+        return skipStdBaseClassRegion(Descr.getRegion(Call, IsLock));
       },
       Descriptor);
 }
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index eba224b8ec01ce..1efac6ac1c57f4 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -38,6 +38,7 @@
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicType.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeInfo.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
@@ -923,17 +924,6 @@ SVal AnyCXXConstructorCall::getCXXThisVal() const {
   return UnknownVal();
 }
 
-static bool isWithinStdNamespace(const Decl *D) {
-  const DeclContext *DC = D->getDeclContext();
-  while (DC) {
-    if (const auto *NS = dyn_cast<NamespaceDecl>(DC);
-        NS && NS->isStdNamespace())
-      return true;
-    DC = DC->getParent();
-  }
-  return false;
-}
-
 void AnyCXXConstructorCall::getExtraInvalidatedValues(ValueList &Values,
                            RegionAndSymbolInvalidationTraits *ETraits) const {
   SVal V = getCXXThisVal();
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp b/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
index d7137a915b3d3d..4ed4113919c1d4 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -190,5 +190,16 @@ std::optional<SVal> getPointeeVal(SVal PtrSVal, ProgramStateRef State) {
   return std::nullopt;
 }
 
+bool isWithinStdNamespace(const Decl *D) {
+  const DeclContext *DC = D->getDeclContext();
+  while (DC) {
+    if (const auto *NS = dyn_cast<NamespaceDecl>(DC);
+        NS && NS->isStdNamespace())
+      return true;
+    DC = DC->getParent();
+  }
+  return false;
+}
+
 } // namespace ento
 } // namespace clang
diff --git a/clang/test/Analysis/block-in-critical-section-nested-namespace.cpp b/clang/test/Analysis/block-in-critical-section-nested-namespace.cpp
new file mode 100644
index 00000000000000..01975bf4a6a041
--- /dev/null
+++ b/clang/test/Analysis/block-in-critical-section-nested-namespace.cpp
@@ -0,0 +1,42 @@
+
+// RUN: %clang_analyze_cc1 \
+// RUN:   -analyzer-checker=unix.BlockInCriticalSection \
+// RUN:   -std=c++11 \
+// RUN:   -analyzer-output text \
+// RUN:   -verify %s
+
+unsigned int sleep(unsigned int seconds) {return 0;}
+namespace std {
+namespace __detail {
+class __mutex_base {
+public:
+  void lock();
+};
+} // namespace __detail
+
+class mutex : public __detail::__mutex_base{
+public:
+  void unlock();
+  bool try_lock();
+};
+} // namespace std
+
+void gh_99628() {
+  std::mutex m;
+  m.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}}
+  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
+}



More information about the cfe-commits mailing list