Skip to content

Commit 5c20891

Browse files
rniwarniwa
authored
[WebKit Checkers] Allow a guardian CheckedPtr/CheckedRef (llvm#110222)
This PR makes WebKit checkers allow a guardian variable which is CheckedPtr or CheckedRef as in addition to RefPtr or Ref.
1 parent 81e536e commit 5c20891

File tree

8 files changed

+177
-20
lines changed

8 files changed

+177
-20
lines changed

‎clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@
1717

1818
namespace clang {
1919

20+
bool isSafePtr(clang::CXXRecordDecl *Decl) {
21+
return isRefCounted(Decl) || isCheckedPtr(Decl);
22+
}
23+
2024
bool tryToFindPtrOrigin(
2125
const Expr *E, bool StopAtFirstRefCountedObj,
2226
std::function<bool(const clang::Expr *, bool)> callback) {
@@ -31,7 +35,7 @@ bool tryToFindPtrOrigin(
3135
}
3236
if (auto *tempExpr = dyn_cast<CXXTemporaryObjectExpr>(E)) {
3337
if (auto *C = tempExpr->getConstructor()) {
34-
if (auto *Class = C->getParent(); Class && isRefCounted(Class))
38+
if (auto *Class = C->getParent(); Class && isSafePtr(Class))
3539
return callback(E, true);
3640
break;
3741
}
@@ -56,7 +60,7 @@ bool tryToFindPtrOrigin(
5660
if (StopAtFirstRefCountedObj) {
5761
if (auto *ConversionFunc =
5862
dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) {
59-
if (isCtorOfRefCounted(ConversionFunc))
63+
if (isCtorOfSafePtr(ConversionFunc))
6064
return callback(E, true);
6165
}
6266
}
@@ -68,7 +72,7 @@ bool tryToFindPtrOrigin(
6872
if (auto *call = dyn_cast<CallExpr>(E)) {
6973
if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) {
7074
if (auto *decl = memberCall->getMethodDecl()) {
71-
std::optional<bool> IsGetterOfRefCt = isGetterOfRefCounted(decl);
75+
std::optional<bool> IsGetterOfRefCt = isGetterOfSafePtr(decl);
7276
if (IsGetterOfRefCt && *IsGetterOfRefCt) {
7377
E = memberCall->getImplicitObjectArgument();
7478
if (StopAtFirstRefCountedObj) {
@@ -87,15 +91,15 @@ bool tryToFindPtrOrigin(
8791
}
8892

8993
if (auto *callee = call->getDirectCallee()) {
90-
if (isCtorOfRefCounted(callee)) {
94+
if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) {
9195
if (StopAtFirstRefCountedObj)
9296
return callback(E, true);
9397

9498
E = call->getArg(0);
9599
continue;
96100
}
97101

98-
if (isRefType(callee->getReturnType()))
102+
if (isSafePtrType(callee->getReturnType()))
99103
return callback(E, true);
100104

101105
if (isSingleton(callee))
@@ -114,7 +118,7 @@ bool tryToFindPtrOrigin(
114118
}
115119
if (auto *ObjCMsgExpr = dyn_cast<ObjCMessageExpr>(E)) {
116120
if (auto *Method = ObjCMsgExpr->getMethodDecl()) {
117-
if (isRefType(Method->getReturnType()))
121+
if (isSafePtrType(Method->getReturnType()))
118122
return callback(E, true);
119123
}
120124
}

‎clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,16 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
135135
|| FunctionName == "Identifier";
136136
}
137137

138-
bool isRefType(const clang::QualType T) {
138+
bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
139+
assert(F);
140+
return isCheckedPtr(safeGetName(F));
141+
}
142+
143+
bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
144+
return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F);
145+
}
146+
147+
bool isSafePtrType(const clang::QualType T) {
139148
QualType type = T;
140149
while (!type.isNull()) {
141150
if (auto *elaboratedT = type->getAs<ElaboratedType>()) {
@@ -145,7 +154,7 @@ bool isRefType(const clang::QualType T) {
145154
if (auto *specialT = type->getAs<TemplateSpecializationType>()) {
146155
if (auto *decl = specialT->getTemplateName().getAsTemplateDecl()) {
147156
auto name = decl->getNameAsString();
148-
return isRefType(name);
157+
return isRefType(name) || isCheckedPtr(name);
149158
}
150159
return false;
151160
}
@@ -177,6 +186,12 @@ std::optional<bool> isUncounted(const CXXRecordDecl* Class)
177186
return (*IsRefCountable);
178187
}
179188

189+
std::optional<bool> isUnchecked(const CXXRecordDecl *Class) {
190+
if (isCheckedPtr(Class))
191+
return false; // Cheaper than below
192+
return isCheckedPtrCapable(Class);
193+
}
194+
180195
std::optional<bool> isUncountedPtr(const QualType T) {
181196
if (T->isPointerType() || T->isReferenceType()) {
182197
if (auto *CXXRD = T->getPointeeCXXRecordDecl())
@@ -185,15 +200,26 @@ std::optional<bool> isUncountedPtr(const QualType T) {
185200
return false;
186201
}
187202

188-
std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M)
189-
{
203+
std::optional<bool> isUnsafePtr(const QualType T) {
204+
if (T->isPointerType() || T->isReferenceType()) {
205+
if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
206+
return isUncounted(CXXRD) || isUnchecked(CXXRD);
207+
}
208+
}
209+
return false;
210+
}
211+
212+
std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) {
190213
assert(M);
191214

192215
if (isa<CXXMethodDecl>(M)) {
193216
const CXXRecordDecl *calleeMethodsClass = M->getParent();
194217
auto className = safeGetName(calleeMethodsClass);
195218
auto method = safeGetName(M);
196219

220+
if (isCheckedPtr(className) && (method == "get" || method == "ptr"))
221+
return true;
222+
197223
if ((isRefType(className) && (method == "get" || method == "ptr")) ||
198224
((className == "String" || className == "AtomString" ||
199225
className == "AtomStringImpl" || className == "UniqueString" ||
@@ -205,7 +231,12 @@ std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M)
205231
// FIXME: Currently allowing any Ref<T> -> whatever cast.
206232
if (isRefType(className)) {
207233
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
208-
return isUncountedPtr(maybeRefToRawOperator->getConversionType());
234+
return isUnsafePtr(maybeRefToRawOperator->getConversionType());
235+
}
236+
237+
if (isCheckedPtr(className)) {
238+
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
239+
return isUnsafePtr(maybeRefToRawOperator->getConversionType());
209240
}
210241
}
211242
return false;
@@ -448,7 +479,7 @@ class TrivialFunctionAnalysisVisitor
448479
if (!Callee)
449480
return false;
450481

451-
std::optional<bool> IsGetterOfRefCounted = isGetterOfRefCounted(Callee);
482+
std::optional<bool> IsGetterOfRefCounted = isGetterOfSafePtr(Callee);
452483
if (IsGetterOfRefCounted && *IsGetterOfRefCounted)
453484
return true;
454485

‎clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -63,18 +63,30 @@ std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
6363
/// class, false if not, std::nullopt if inconclusive.
6464
std::optional<bool> isUncountedPtr(const clang::QualType T);
6565

66-
/// \returns true if Name is a RefPtr, Ref, or its variant, false if not.
67-
bool isRefType(const std::string &Name);
66+
/// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
67+
/// variant, false if not.
68+
bool isSafePtrType(const clang::QualType T);
6869

6970
/// \returns true if \p F creates ref-countable object from uncounted parameter,
7071
/// false if not.
7172
bool isCtorOfRefCounted(const clang::FunctionDecl *F);
7273

73-
/// \returns true if \p T is RefPtr, Ref, or its variant, false if not.
74-
bool isRefType(const clang::QualType T);
74+
/// \returns true if \p F creates checked ptr object from uncounted parameter,
75+
/// false if not.
76+
bool isCtorOfCheckedPtr(const clang::FunctionDecl *F);
77+
78+
/// \returns true if \p F creates ref-countable or checked ptr object from
79+
/// uncounted parameter, false if not.
80+
bool isCtorOfSafePtr(const clang::FunctionDecl *F);
81+
82+
/// \returns true if \p Name is RefPtr, Ref, or its variant, false if not.
83+
bool isRefType(const std::string &Name);
84+
85+
/// \returns true if \p Name is CheckedRef or CheckedPtr, false if not.
86+
bool isCheckedPtr(const std::string &Name);
7587

7688
/// \returns true if \p M is getter of a ref-counted class, false if not.
77-
std::optional<bool> isGetterOfRefCounted(const clang::CXXMethodDecl* Method);
89+
std::optional<bool> isGetterOfSafePtr(const clang::CXXMethodDecl *Method);
7890

7991
/// \returns true if \p F is a conversion between ref-countable or ref-counted
8092
/// pointer types.

‎clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ class UncountedCallArgsChecker
9696
auto name = safeGetName(MD);
9797
if (name == "ref" || name == "deref")
9898
return;
99+
if (name == "incrementPtrCount" || name == "decrementPtrCount")
100+
return;
99101
}
100102
auto *E = MemberCallExpr->getImplicitObjectArgument();
101103
QualType ArgType = MemberCallExpr->getObjectType().getCanonicalType();

‎clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,7 @@ class UncountedLocalVarsChecker
227227
if (MaybeGuardianArgCXXRecord) {
228228
if (MaybeGuardian->isLocalVarDecl() &&
229229
(isRefCounted(MaybeGuardianArgCXXRecord) ||
230+
isCheckedPtr(MaybeGuardianArgCXXRecord) ||
230231
isRefcountedStringsHack(MaybeGuardian)) &&
231232
isGuardedScopeEmbeddedInGuardianScope(
232233
V, MaybeGuardian))

‎clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
2+
3+
#include "mock-types.h"
4+
5+
RefCountableAndCheckable* makeObj();
6+
CheckedRef<RefCountableAndCheckable> makeObjChecked();
7+
void someFunction(RefCountableAndCheckable*);
8+
9+
namespace call_args_unchecked_uncounted {
10+
11+
static void foo() {
12+
someFunction(makeObj());
13+
// expected-warning@-1{{Call argument is uncounted and unsafe [alpha.webkit.UncountedCallArgsChecker]}}
14+
}
15+
16+
} // namespace call_args_checked
17+
18+
namespace call_args_checked {
19+
20+
static void foo() {
21+
CheckedPtr<RefCountableAndCheckable> ptr = makeObj();
22+
someFunction(ptr.get());
23+
}
24+
25+
static void bar() {
26+
someFunction(CheckedPtr { makeObj() }.get());
27+
}
28+
29+
static void baz() {
30+
someFunction(makeObjChecked().ptr());
31+
}
32+
33+
} // namespace call_args_checked
34+
35+
namespace call_args_default {
36+
37+
void someFunction(RefCountableAndCheckable* = makeObj());
38+
// expected-warning@-1{{Call argument is uncounted and unsafe [alpha.webkit.UncountedCallArgsChecker]}}
39+
void otherFunction(RefCountableAndCheckable* = makeObjChecked().ptr());
40+
41+
void foo() {
42+
someFunction();
43+
otherFunction();
44+
}
45+
46+
}

‎clang/test/Analysis/Checkers/WebKit/mock-types.h

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,8 @@ template <typename T> struct CheckedRef {
114114

115115
public:
116116
CheckedRef() : t{} {};
117-
CheckedRef(T &t) : t(t) { t->incrementPtrCount(); }
118-
CheckedRef(const CheckedRef& o) : t(o.t) { if (t) t->incrementPtrCount(); }
117+
CheckedRef(T &t) : t(&t) { t.incrementPtrCount(); }
118+
CheckedRef(const CheckedRef &o) : t(o.t) { if (t) t->incrementPtrCount(); }
119119
~CheckedRef() { if (t) t->decrementPtrCount(); }
120120
T &get() { return *t; }
121121
T *ptr() { return t; }
@@ -135,7 +135,7 @@ template <typename T> struct CheckedPtr {
135135
if (t)
136136
t->incrementPtrCount();
137137
}
138-
CheckedPtr(Ref<T>&& o)
138+
CheckedPtr(Ref<T> &&o)
139139
: t(o.leakRef())
140140
{ }
141141
~CheckedPtr() {
@@ -156,4 +156,14 @@ class CheckedObj {
156156
void decrementPtrCount();
157157
};
158158

159+
class RefCountableAndCheckable {
160+
public:
161+
void incrementPtrCount() const;
162+
void decrementPtrCount() const;
163+
void ref() const;
164+
void deref() const;
165+
void method();
166+
int trivial() { return 0; }
167+
};
168+
159169
#endif

‎clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,57 @@ void foo() {
290290

291291
} // namespace local_assignment_to_global
292292

293+
namespace local_refcountable_checkable_object {
294+
295+
RefCountableAndCheckable* provide_obj();
296+
297+
void local_raw_ptr() {
298+
RefCountableAndCheckable* a = nullptr;
299+
// expected-warning@-1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
300+
a = provide_obj();
301+
a->method();
302+
}
303+
304+
void local_checked_ptr() {
305+
CheckedPtr<RefCountableAndCheckable> a = nullptr;
306+
a = provide_obj();
307+
a->method();
308+
}
309+
310+
void local_var_with_guardian_checked_ptr() {
311+
CheckedPtr<RefCountableAndCheckable> a = provide_obj();
312+
{
313+
auto* b = a.get();
314+
b->method();
315+
}
316+
}
317+
318+
void local_var_with_guardian_checked_ptr_with_assignment() {
319+
CheckedPtr<RefCountableAndCheckable> a = provide_obj();
320+
{
321+
RefCountableAndCheckable* b = a.get();
322+
// expected-warning@-1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
323+
b = provide_obj();
324+
b->method();
325+
}
326+
}
327+
328+
void local_var_with_guardian_checked_ref() {
329+
CheckedRef<RefCountableAndCheckable> a = *provide_obj();
330+
{
331+
RefCountableAndCheckable& b = a;
332+
b.method();
333+
}
334+
}
335+
336+
void static_var() {
337+
static RefCountableAndCheckable* a = nullptr;
338+
// expected-warning@-1{{Static local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
339+
a = provide_obj();
340+
}
341+
342+
} // namespace local_refcountable_checkable_object
343+
293344
namespace local_var_in_recursive_function {
294345

295346
struct TreeNode {

0 commit comments

Comments
 (0)