[analyzer] Fix false positive for mutexes inheriting mutex_base

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

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"
@@ -241,12 +242,22 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const CallEvent &Call,
   return std::nullopt;
 }
 
+static const MemRegion *skipStdBaseClassRegion(const MemRegion *Reg) {
+  while (Reg) {
+    const auto *BaseClassRegion = dyn_cast<CXXBaseObjectRegion>(Reg);
+    if (!BaseClassRegion || !isWithinStdNamespace(BaseClassRegion->getDecl()))
+      break;
+    Reg = BaseClassRegion->getSuperRegion();
+  }
+  return Reg;
+}
+
 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 * {
+        return skipStdBaseClassRegion(Descr.getRegion(Call, IsLock));
       },
       Descriptor);
 }

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();

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

clang/test/Analysis/block-in-critical-section-inheritance.cpp

@@ -29,3 +29,168 @@ 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
+}
+
+struct TwoMutexes {
+  std::mutex m1;
+  std::mutex m2;
+};
+
+void two_mutexes_no_false_negative(TwoMutexes &tm) {
+  tm.m1.lock();
+  // expected-note@-1 {{Entering critical section here}}
+  tm.m2.unlock();
+  sleep(10);
+  // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
+  // expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
+  tm.m1.unlock();
+}
+
+struct MyMutexBase1 : std::mutex {
+  void lock1() { lock(); }
+  // expected-note@-1 {{Entering critical section here}}
+  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@-1 {{Entering critical section here}}
+  sleep(10);
+  // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
+  // expected-note@-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@-1 {{Entering critical section here}}
+  sleep(10);
+  // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
+  // expected-note@-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_tp(MyMutex &m) {
+  m.lock1();
+  // expected-note@-1 {{Calling 'MyMutexBase1::lock1'}}
+  // expected-note@-2 {{Returning from 'MyMutexBase1::lock1'}}
+  m.unlock2();
+  sleep(10);
+  // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
+  // expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
+  m.unlock1();
+}
+
+void two_custom_mutex_bases_tn(MyMutex &m) {
+  m.lock1();
+  m.unlock1();
+  sleep(10);
+}
+
+void two_custom_mutex_bases_casts_tp(MyMutex &m) {
+  static_cast<MyMutexBase1&>(m).lock();
+  // expected-note@-1 {{Entering critical section here}}
+  static_cast<MyMutexBase2&>(m).unlock();
+  sleep(10);
+  // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
+  // expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
+  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);
+}
+
+struct MutexVirtBase1 : virtual std::mutex {
+  void lock1() { lock(); }
+  // expected-note@-1 {{Entering critical section here}}
+  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);
+}
+
+void virt_inherited_mutexes_different_bases_tp(CombinedVirtMutex &cvt) {
+  cvt.lock1();
+  // expected-note@-1 {{Calling 'MutexVirtBase1::lock1'}}
+  // expected-note@-2 {{Returning from 'MutexVirtBase1::lock1'}}
+  sleep(10);
+  // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
+  // expected-note@-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);
+  ~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

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@-1 {{Entering critical section here}}
+  sleep(10);
+  // expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
+  // 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
+}