[clang] 82e314e - [analyzer] Fix false positive for mutexes inheriting mutex_base (#106240)

via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 28 02:30:22 PDT 2024


Author: Arseniy Zaostrovnykh
Date: 2024-08-28T11:30:18+02:00
New Revision: 82e314e3664d2c8768212e74f751187d51950b87

URL: https://github.com/llvm/llvm-project/commit/82e314e3664d2c8768212e74f751187d51950b87
DIFF: https://github.com/llvm/llvm-project/commit/82e314e3664d2c8768212e74f751187d51950b87.diff

LOG: [analyzer] Fix false positive for mutexes inheriting mutex_base (#106240)

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 likely fixes #104241

CPP-5541

Added: 
    clang/test/Analysis/block-in-critical-section-nested-namespace.cpp

Modified: 
    clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
    clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
    clang/lib/StaticAnalyzer/Core/CallEvent.cpp
    clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp
    clang/test/Analysis/block-in-critical-section-inheritance.cpp

Removed: 
    


################################################################################
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 4cd2f2802f30cd..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"
@@ -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);
 }

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-inheritance.cpp b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
index db20df8c60a5c9..4b674e07436fc5 100644
--- a/clang/test/Analysis/block-in-critical-section-inheritance.cpp
+++ b/clang/test/Analysis/block-in-critical-section-inheritance.cpp
@@ -29,3 +29,168 @@ 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 
diff erent 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 at -1 {{Entering critical section here}}
+  tm.m2.unlock();
+  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();
+}
+
+struct MyMutexBase1 : std::mutex {
+  void lock1() { lock(); }
+  // expected-note at -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 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_tp(MyMutex &m) {
+  m.lock1();
+  // expected-note at -1 {{Calling 'MyMutexBase1::lock1'}}
+  // expected-note at -2 {{Returning from 'MyMutexBase1::lock1'}}
+  m.unlock2();
+  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();
+}
+
+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 at -1 {{Entering critical section here}}
+  static_cast<MyMutexBase2&>(m).unlock();
+  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();
+}
+
+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 at -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_
diff erent_bases_tn(CombinedVirtMutex &cvt) {
+  cvt.lock1();
+  cvt.unlock2(); // Despite a 
diff erent name, unlock2 acts on the same mutex as lock1
+  sleep(10);
+}
+
+void virt_inherited_mutexes_
diff erent_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);
+  ~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

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 
diff erent memory regions:
+  // __mutex_base and mutex.
+  m.unlock();
+  sleep(10); // no-warning
+}


        


More information about the cfe-commits mailing list