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@-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@-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_ 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@-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 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@-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 diff erent memory regions: + // __mutex_base and mutex. + m.unlock(); + sleep(10); // no-warning +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
