[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