Skip to content

Commit 82e314e

Browse files
nectonecto
authored
[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
1 parent 07514fa commit 82e314e

File tree

6 files changed

+237
-14
lines changed

6 files changed

+237
-14
lines changed

‎clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
//
77
//===----------------------------------------------------------------------===//
88
//
9-
// This file defines CheckerVisitor.
9+
// This file defines various utilities used by checkers.
1010
//
1111
//===----------------------------------------------------------------------===//
1212

@@ -114,6 +114,10 @@ OperatorKind operationKindFromOverloadedOperator(OverloadedOperatorKind OOK,
114114

115115
std::optional<SVal> getPointeeVal(SVal PtrSVal, ProgramStateRef State);
116116

117+
/// Returns true if declaration \p D is in std namespace or any nested namespace
118+
/// or class scope.
119+
bool isWithinStdNamespace(const Decl *D);
120+
117121
} // namespace ento
118122

119123
} // namespace clang

‎clang/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
2121
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
2222
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
23+
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
2324
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
2425
#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
2526
#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
@@ -241,12 +242,22 @@ BlockInCriticalSectionChecker::checkDescriptorMatch(const CallEvent &Call,
241242
return std::nullopt;
242243
}
243244

245+
static const MemRegion *skipStdBaseClassRegion(const MemRegion *Reg) {
246+
while (Reg) {
247+
const auto *BaseClassRegion = dyn_cast<CXXBaseObjectRegion>(Reg);
248+
if (!BaseClassRegion || !isWithinStdNamespace(BaseClassRegion->getDecl()))
249+
break;
250+
Reg = BaseClassRegion->getSuperRegion();
251+
}
252+
return Reg;
253+
}
254+
244255
static const MemRegion *getRegion(const CallEvent &Call,
245256
const MutexDescriptor &Descriptor,
246257
bool IsLock) {
247258
return std::visit(
248-
[&Call, IsLock](auto &&Descriptor) {
249-
return Descriptor.getRegion(Call, IsLock);
259+
[&Call, IsLock](auto &Descr) -> const MemRegion * {
260+
return skipStdBaseClassRegion(Descr.getRegion(Call, IsLock));
250261
},
251262
Descriptor);
252263
}

‎clang/lib/StaticAnalyzer/Core/CallEvent.cpp

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
#include "clang/CrossTU/CrossTranslationUnit.h"
3939
#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
4040
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
41+
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
4142
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicType.h"
4243
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeInfo.h"
4344
#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
@@ -923,17 +924,6 @@ SVal AnyCXXConstructorCall::getCXXThisVal() const {
923924
return UnknownVal();
924925
}
925926

926-
static bool isWithinStdNamespace(const Decl *D) {
927-
const DeclContext *DC = D->getDeclContext();
928-
while (DC) {
929-
if (const auto *NS = dyn_cast<NamespaceDecl>(DC);
930-
NS && NS->isStdNamespace())
931-
return true;
932-
DC = DC->getParent();
933-
}
934-
return false;
935-
}
936-
937927
void AnyCXXConstructorCall::getExtraInvalidatedValues(ValueList &Values,
938928
RegionAndSymbolInvalidationTraits *ETraits) const {
939929
SVal V = getCXXThisVal();

‎clang/lib/StaticAnalyzer/Core/CheckerHelpers.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,5 +190,16 @@ std::optional<SVal> getPointeeVal(SVal PtrSVal, ProgramStateRef State) {
190190
return std::nullopt;
191191
}
192192

193+
bool isWithinStdNamespace(const Decl *D) {
194+
const DeclContext *DC = D->getDeclContext();
195+
while (DC) {
196+
if (const auto *NS = dyn_cast<NamespaceDecl>(DC);
197+
NS && NS->isStdNamespace())
198+
return true;
199+
DC = DC->getParent();
200+
}
201+
return false;
202+
}
203+
193204
} // namespace ento
194205
} // namespace clang

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

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,168 @@ void gh_99628() {
2929
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
3030
m.unlock();
3131
}
32+
33+
void no_false_positive_gh_104241() {
34+
std::mutex m;
35+
m.lock();
36+
// If inheritance not handled properly, this unlock might not match the lock
37+
// above because technically they act on different memory regions:
38+
// __mutex_base and mutex.
39+
m.unlock();
40+
sleep(10); // no-warning
41+
}
42+
43+
struct TwoMutexes {
44+
std::mutex m1;
45+
std::mutex m2;
46+
};
47+
48+
void two_mutexes_no_false_negative(TwoMutexes &tm) {
49+
tm.m1.lock();
50+
// expected-note@-1 {{Entering critical section here}}
51+
tm.m2.unlock();
52+
sleep(10);
53+
// expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
54+
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
55+
tm.m1.unlock();
56+
}
57+
58+
struct MyMutexBase1 : std::mutex {
59+
void lock1() { lock(); }
60+
// expected-note@-1 {{Entering critical section here}}
61+
void unlock1() { unlock(); }
62+
};
63+
struct MyMutexBase2 : std::mutex {
64+
void lock2() { lock(); }
65+
void unlock2() { unlock(); }
66+
};
67+
struct MyMutex : MyMutexBase1, MyMutexBase2 {};
68+
// MyMutex has two distinct std::mutex as base classes
69+
70+
void custom_mutex_tp(MyMutexBase1 &mb) {
71+
mb.lock();
72+
// expected-note@-1 {{Entering critical section here}}
73+
sleep(10);
74+
// expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
75+
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
76+
mb.unlock();
77+
}
78+
79+
void custom_mutex_tn(MyMutexBase1 &mb) {
80+
mb.lock();
81+
mb.unlock();
82+
sleep(10);
83+
}
84+
85+
void custom_mutex_cast_tp(MyMutexBase1 &mb) {
86+
static_cast<std::mutex&>(mb).lock();
87+
// expected-note@-1 {{Entering critical section here}}
88+
sleep(10);
89+
// expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
90+
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
91+
static_cast<std::mutex&>(mb).unlock();
92+
}
93+
94+
void custom_mutex_cast_tn(MyMutexBase1 &mb) {
95+
static_cast<std::mutex&>(mb).lock();
96+
static_cast<std::mutex&>(mb).unlock();
97+
sleep(10);
98+
}
99+
100+
void two_custom_mutex_bases_tp(MyMutex &m) {
101+
m.lock1();
102+
// expected-note@-1 {{Calling 'MyMutexBase1::lock1'}}
103+
// expected-note@-2 {{Returning from 'MyMutexBase1::lock1'}}
104+
m.unlock2();
105+
sleep(10);
106+
// expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
107+
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
108+
m.unlock1();
109+
}
110+
111+
void two_custom_mutex_bases_tn(MyMutex &m) {
112+
m.lock1();
113+
m.unlock1();
114+
sleep(10);
115+
}
116+
117+
void two_custom_mutex_bases_casts_tp(MyMutex &m) {
118+
static_cast<MyMutexBase1&>(m).lock();
119+
// expected-note@-1 {{Entering critical section here}}
120+
static_cast<MyMutexBase2&>(m).unlock();
121+
sleep(10);
122+
// expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
123+
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
124+
static_cast<MyMutexBase1&>(m).unlock();
125+
}
126+
127+
void two_custom_mutex_bases_casts_tn(MyMutex &m) {
128+
static_cast<MyMutexBase1&>(m).lock();
129+
static_cast<MyMutexBase1&>(m).unlock();
130+
sleep(10);
131+
}
132+
133+
struct MutexVirtBase1 : virtual std::mutex {
134+
void lock1() { lock(); }
135+
// expected-note@-1 {{Entering critical section here}}
136+
void unlock1() { unlock(); }
137+
};
138+
139+
struct MutexVirtBase2 : virtual std::mutex {
140+
void lock2() { lock(); }
141+
void unlock2() { unlock(); }
142+
};
143+
144+
struct CombinedVirtMutex : MutexVirtBase1, MutexVirtBase2 {};
145+
146+
void virt_inherited_mutexes_same_base_tn1(CombinedVirtMutex &cvt) {
147+
cvt.lock1();
148+
cvt.unlock1();
149+
sleep(10);
150+
}
151+
152+
void virt_inherited_mutexes_different_bases_tn(CombinedVirtMutex &cvt) {
153+
cvt.lock1();
154+
cvt.unlock2(); // Despite a different name, unlock2 acts on the same mutex as lock1
155+
sleep(10);
156+
}
157+
158+
void virt_inherited_mutexes_different_bases_tp(CombinedVirtMutex &cvt) {
159+
cvt.lock1();
160+
// expected-note@-1 {{Calling 'MutexVirtBase1::lock1'}}
161+
// expected-note@-2 {{Returning from 'MutexVirtBase1::lock1'}}
162+
sleep(10);
163+
// expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
164+
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
165+
cvt.unlock1();
166+
}
167+
168+
namespace std {
169+
template <class... MutexTypes> struct scoped_lock {
170+
explicit scoped_lock(MutexTypes&... m);
171+
~scoped_lock();
172+
};
173+
template <class MutexType> class scoped_lock<MutexType> {
174+
public:
175+
explicit scoped_lock(MutexType& m) : m(m) { m.lock(); }
176+
~scoped_lock() { m.unlock(); }
177+
private:
178+
MutexType& m;
179+
};
180+
} // namespace std
181+
182+
namespace gh_104241 {
183+
int magic_number;
184+
std::mutex m;
185+
186+
void fixed() {
187+
int current;
188+
for (int items_processed = 0; items_processed < 100; ++items_processed) {
189+
{
190+
std::scoped_lock<std::mutex> guard(m);
191+
current = magic_number;
192+
}
193+
sleep(current); // expected no warning
194+
}
195+
}
196+
} // namespace gh_104241

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
2+
// RUN: %clang_analyze_cc1 \
3+
// RUN: -analyzer-checker=unix.BlockInCriticalSection \
4+
// RUN: -std=c++11 \
5+
// RUN: -analyzer-output text \
6+
// RUN: -verify %s
7+
8+
unsigned int sleep(unsigned int seconds) {return 0;}
9+
namespace std {
10+
namespace __detail {
11+
class __mutex_base {
12+
public:
13+
void lock();
14+
};
15+
} // namespace __detail
16+
17+
class mutex : public __detail::__mutex_base{
18+
public:
19+
void unlock();
20+
bool try_lock();
21+
};
22+
} // namespace std
23+
24+
void gh_99628() {
25+
std::mutex m;
26+
m.lock();
27+
// expected-note@-1 {{Entering critical section here}}
28+
sleep(10);
29+
// expected-warning@-1 {{Call to blocking function 'sleep' inside of critical section}}
30+
// expected-note@-2 {{Call to blocking function 'sleep' inside of critical section}}
31+
m.unlock();
32+
}
33+
34+
void no_false_positive_gh_104241() {
35+
std::mutex m;
36+
m.lock();
37+
// If inheritance not handled properly, this unlock might not match the lock
38+
// above because technically they act on different memory regions:
39+
// __mutex_base and mutex.
40+
m.unlock();
41+
sleep(10); // no-warning
42+
}

0 commit comments

Comments
 (0)