[clang] [analyzer] Fix false positive for mutexes inheriting mutex_base (PR #106240)

[Arseniy Zaostrovnykh via cfe-commits]

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/3] [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/3] 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/3] 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();
 }