Skip to content

Commit d6c4e1a

Browse files
rniwarniwa
committed
[alpha.webkit.UnretainedLocalVarsChecker] Add a checker for local variables to NS and CF types.
This PR adds alpha.webkit.UnretainedLocalVarsChecker by generalizing RawPtrRefLocalVarsChecker. It checks local variables to NS or CF types are guarded with a RetainPtr or not. The new checker is effective for NS and CF types in Objective-C++ code without ARC, and it's effective for CF types in code with ARC.
1 parent b5b8a59 commit d6c4e1a

File tree

12 files changed

+742
-21
lines changed

12 files changed

+742
-21
lines changed

‎clang/docs/analyzer/checkers.rst‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3668,6 +3668,12 @@ Here are some examples of situations that we warn about as they *might* be poten
36683668
RefCountable* uncounted = counted.get(); // warn
36693669
}
36703670
3671+
alpha.webkit.UnretainedLocalVarsChecker
3672+
""""""""""""""""""""""""""""""""""""""
3673+
The goal of this rule is to make sure that any NS or CF local variable is backed by a RetainPtr with lifetime that is strictly larger than the scope of the unretained local variable. To be on the safe side we require the scope of an unretained variable to be embedded in the scope of Retainptr object that backs it.
3674+
3675+
The rules of when to use and not to use RetainPtr are same as alpha.webkit.UncountedCallArgsChecker for ref-counted objects.
3676+
36713677
Debug Checkers
36723678
---------------
36733679

‎clang/include/clang/StaticAnalyzer/Checkers/Checkers.td‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1782,4 +1782,8 @@ def UncheckedLocalVarsChecker : Checker<"UncheckedLocalVarsChecker">,
17821782
HelpText<"Check unchecked local variables.">,
17831783
Documentation<HasDocumentation>;
17841784

1785+
def UnretainedLocalVarsChecker : Checker<"UnretainedLocalVarsChecker">,
1786+
HelpText<"Check unretained local variables.">,
1787+
Documentation<HasDocumentation>;
1788+
17851789
} // end alpha.webkit

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ bool isSafePtr(clang::CXXRecordDecl *Decl) {
2424

2525
bool tryToFindPtrOrigin(
2626
const Expr *E, bool StopAtFirstRefCountedObj,
27+
std::function<bool(const clang::CXXRecordDecl *)> isSafePtr,
28+
std::function<bool(const clang::QualType)> isSafePtrType,
2729
std::function<bool(const clang::Expr *, bool)> callback) {
2830
while (E) {
2931
if (auto *tempExpr = dyn_cast<MaterializeTemporaryExpr>(E)) {
@@ -53,9 +55,9 @@ bool tryToFindPtrOrigin(
5355
}
5456
if (auto *Expr = dyn_cast<ConditionalOperator>(E)) {
5557
return tryToFindPtrOrigin(Expr->getTrueExpr(), StopAtFirstRefCountedObj,
56-
callback) &&
58+
isSafePtr, isSafePtrType, callback) &&
5759
tryToFindPtrOrigin(Expr->getFalseExpr(), StopAtFirstRefCountedObj,
58-
callback);
60+
isSafePtr, isSafePtrType, callback);
5961
}
6062
if (auto *cast = dyn_cast<CastExpr>(E)) {
6163
if (StopAtFirstRefCountedObj) {
@@ -92,7 +94,7 @@ bool tryToFindPtrOrigin(
9294
}
9395

9496
if (auto *callee = call->getDirectCallee()) {
95-
if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) {
97+
if (isCtorOfSafePtr(callee)) {
9698
if (StopAtFirstRefCountedObj)
9799
return callback(E, true);
98100

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ class Expr;
5454
/// Returns false if any of calls to callbacks returned false. Otherwise true.
5555
bool tryToFindPtrOrigin(
5656
const clang::Expr *E, bool StopAtFirstRefCountedObj,
57+
std::function<bool(const clang::CXXRecordDecl *)> isSafePtr,
58+
std::function<bool(const clang::QualType)> isSafePtrType,
5759
std::function<bool(const clang::Expr *, bool)> callback);
5860

5961
/// For \p E referring to a ref-countable/-counted pointer/reference we return

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

Lines changed: 76 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "clang/AST/DeclCXX.h"
1414
#include "clang/AST/ExprCXX.h"
1515
#include "clang/AST/StmtVisitor.h"
16+
#include "clang/Analysis/DomainSpecific/CocoaConventions.h"
1617
#include <optional>
1718

1819
using namespace clang;
@@ -117,6 +118,10 @@ bool isRefType(const std::string &Name) {
117118
Name == "RefPtr" || Name == "RefPtrAllowingPartiallyDestroyed";
118119
}
119120

121+
bool isRetainPtr(const std::string &Name) {
122+
return Name == "RetainPtr";
123+
}
124+
120125
bool isCheckedPtr(const std::string &Name) {
121126
return Name == "CheckedPtr" || Name == "CheckedRef";
122127
}
@@ -140,8 +145,15 @@ bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
140145
return isCheckedPtr(safeGetName(F));
141146
}
142147

148+
bool isCtorOfRetainPtr(const clang::FunctionDecl *F) {
149+
const std::string &FunctionName = safeGetName(F);
150+
return FunctionName == "RetainPtr" || FunctionName == "adoptNS" ||
151+
FunctionName == "adoptCF";
152+
}
153+
143154
bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
144-
return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F);
155+
return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F) ||
156+
isCtorOfRetainPtr(F);
145157
}
146158

147159
template <typename Predicate>
@@ -163,11 +175,16 @@ static bool isPtrOfType(const clang::QualType T, Predicate Pred) {
163175
return false;
164176
}
165177

166-
bool isSafePtrType(const clang::QualType T) {
178+
bool isRefOrCheckedPtrType(const clang::QualType T) {
167179
return isPtrOfType(
168180
T, [](auto Name) { return isRefType(Name) || isCheckedPtr(Name); });
169181
}
170182

183+
bool isRetainPtrType(const clang::QualType T) {
184+
return isPtrOfType(
185+
T, [](auto Name) { return Name == "RetainPtr"; });
186+
}
187+
171188
bool isOwnerPtrType(const clang::QualType T) {
172189
return isPtrOfType(T, [](auto Name) {
173190
return isRefType(Name) || isCheckedPtr(Name) || Name == "unique_ptr" ||
@@ -195,6 +212,37 @@ std::optional<bool> isUnchecked(const QualType T) {
195212
return isUnchecked(T->getAsCXXRecordDecl());
196213
}
197214

215+
std::optional<bool> isUnretained(const QualType T, bool IsARCEnabled) {
216+
if (auto *Subst = dyn_cast<SubstTemplateTypeParmType>(T)) {
217+
if (auto *Decl = Subst->getAssociatedDecl()) {
218+
if (isRetainPtr(safeGetName(Decl)))
219+
return false;
220+
}
221+
}
222+
if ((ento::cocoa::isCocoaObjectRef(T) && !IsARCEnabled) ||
223+
ento::coreFoundation::isCFObjectRef(T))
224+
return true;
225+
226+
// RetainPtr strips typedef for CF*Ref. Manually check for struct __CF* types.
227+
auto CanonicalType = T.getCanonicalType();
228+
auto *Type = CanonicalType.getTypePtrOrNull();
229+
if (!Type)
230+
return false;
231+
auto Pointee = Type->getPointeeType();
232+
auto *PointeeType = Pointee.getTypePtrOrNull();
233+
if (!PointeeType)
234+
return false;
235+
auto *Record = PointeeType->getAsStructureType();
236+
if (!Record)
237+
return false;
238+
auto *Decl = Record->getDecl();
239+
if (!Decl)
240+
return false;
241+
auto TypeName = Decl->getName();
242+
return TypeName.starts_with("__CF") || TypeName.starts_with("__CG") ||
243+
TypeName.starts_with("__CM");
244+
}
245+
198246
std::optional<bool> isUncounted(const CXXRecordDecl* Class)
199247
{
200248
// Keep isRefCounted first as it's cheaper.
@@ -230,16 +278,20 @@ std::optional<bool> isUncheckedPtr(const QualType T) {
230278
return false;
231279
}
232280

233-
std::optional<bool> isUnsafePtr(const QualType T) {
281+
std::optional<bool> isUnsafePtr(const QualType T, bool IsArcEnabled) {
234282
if (T->isPointerType() || T->isReferenceType()) {
235283
if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
236284
auto isUncountedPtr = isUncounted(CXXRD);
237285
auto isUncheckedPtr = isUnchecked(CXXRD);
238-
if (isUncountedPtr && isUncheckedPtr)
239-
return *isUncountedPtr || *isUncheckedPtr;
286+
auto isUnretainedPtr = isUnretained(T, IsArcEnabled);
287+
std::optional<bool> result;
240288
if (isUncountedPtr)
241-
return *isUncountedPtr;
242-
return isUncheckedPtr;
289+
result = *isUncountedPtr;
290+
if (isUncheckedPtr)
291+
result = result ? *result || *isUncheckedPtr : *isUncheckedPtr;
292+
if (isUnretainedPtr)
293+
result = result ? *result || *isUnretainedPtr : *isUnretainedPtr;
294+
return result;
243295
}
244296
}
245297
return false;
@@ -263,16 +315,24 @@ std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) {
263315
method == "impl"))
264316
return true;
265317

318+
if (className == "RetainPtr" && method == "get")
319+
return true;
320+
266321
// Ref<T> -> T conversion
267322
// FIXME: Currently allowing any Ref<T> -> whatever cast.
268323
if (isRefType(className)) {
269324
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
270-
return isUnsafePtr(maybeRefToRawOperator->getConversionType());
325+
return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
271326
}
272327

273328
if (isCheckedPtr(className)) {
274329
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
275-
return isUnsafePtr(maybeRefToRawOperator->getConversionType());
330+
return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
331+
}
332+
333+
if (className == "RetainPtr") {
334+
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
335+
return isUnsafePtr(maybeRefToRawOperator->getConversionType(), false);
276336
}
277337
}
278338
return false;
@@ -297,6 +357,13 @@ bool isCheckedPtr(const CXXRecordDecl *R) {
297357
return false;
298358
}
299359

360+
bool isRetainPtr(const CXXRecordDecl *R) {
361+
assert(R);
362+
if (auto *TmplR = R->getTemplateInstantiationPattern())
363+
return safeGetName(TmplR) == "RetainPtr";
364+
return false;
365+
}
366+
300367
bool isPtrConversion(const FunctionDecl *F) {
301368
assert(F);
302369
if (isCtorOfRefCounted(F))

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,9 @@ bool isRefCounted(const clang::CXXRecordDecl *Class);
5151
/// \returns true if \p Class is a CheckedPtr / CheckedRef, false if not.
5252
bool isCheckedPtr(const clang::CXXRecordDecl *Class);
5353

54+
/// \returns true if \p Class is a RetainPtr, false if not.
55+
bool isRetainPtr(const clang::CXXRecordDecl *Class);
56+
5457
/// \returns true if \p Class is ref-countable AND not ref-counted, false if
5558
/// not, std::nullopt if inconclusive.
5659
std::optional<bool> isUncounted(const clang::QualType T);
@@ -59,6 +62,10 @@ std::optional<bool> isUncounted(const clang::QualType T);
5962
/// not, std::nullopt if inconclusive.
6063
std::optional<bool> isUnchecked(const clang::QualType T);
6164

65+
/// \returns true if \p Class is NS or CF objects AND not retained, false if
66+
/// not, std::nullopt if inconclusive.
67+
std::optional<bool> isUnretained(const clang::QualType T, bool IsARCEnabled);
68+
6269
/// \returns true if \p Class is ref-countable AND not ref-counted, false if
6370
/// not, std::nullopt if inconclusive.
6471
std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
@@ -77,11 +84,14 @@ std::optional<bool> isUncheckedPtr(const clang::QualType T);
7784

7885
/// \returns true if \p T is either a raw pointer or reference to an uncounted
7986
/// or unchecked class, false if not, std::nullopt if inconclusive.
80-
std::optional<bool> isUnsafePtr(const QualType T);
87+
std::optional<bool> isUnsafePtr(const QualType T, bool IsArcEnabled);
8188

8289
/// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
8390
/// variant, false if not.
84-
bool isSafePtrType(const clang::QualType T);
91+
bool isRefOrCheckedPtrType(const clang::QualType T);
92+
93+
/// \returns true if \p T is a RetainPtr, false if not.
94+
bool isRetainPtrType(const clang::QualType T);
8595

8696
/// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or
8797
/// unique_ptr, false if not.

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ class RawPtrRefCallArgsChecker
4141

4242
virtual std::optional<bool> isUnsafeType(QualType) const = 0;
4343
virtual std::optional<bool> isUnsafePtr(QualType) const = 0;
44+
virtual bool isSafePtr(const CXXRecordDecl *Record) const = 0;
45+
virtual bool isSafePtrType(const QualType type) const = 0;
4446
virtual const char *ptrKind() const = 0;
4547

4648
void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
@@ -141,6 +143,12 @@ class RawPtrRefCallArgsChecker
141143

142144
bool isPtrOriginSafe(const Expr *Arg) const {
143145
return tryToFindPtrOrigin(Arg, /*StopAtFirstRefCountedObj=*/true,
146+
[&](const clang::CXXRecordDecl *Record) {
147+
return isSafePtr(Record);
148+
},
149+
[&](const clang::QualType T) {
150+
return isSafePtrType(T);
151+
},
144152
[&](const clang::Expr *ArgOrigin, bool IsSafe) {
145153
if (IsSafe)
146154
return true;
@@ -315,6 +323,14 @@ class UncountedCallArgsChecker final : public RawPtrRefCallArgsChecker {
315323
return isUncountedPtr(QT);
316324
}
317325

326+
bool isSafePtr(const CXXRecordDecl *Record) const final {
327+
return isRefCounted(Record) || isCheckedPtr(Record);
328+
}
329+
330+
bool isSafePtrType(const QualType type) const final {
331+
return isRefOrCheckedPtrType(type);
332+
}
333+
318334
const char *ptrKind() const final { return "uncounted"; }
319335
};
320336

@@ -332,6 +348,14 @@ class UncheckedCallArgsChecker final : public RawPtrRefCallArgsChecker {
332348
return isUncheckedPtr(QT);
333349
}
334350

351+
bool isSafePtr(const CXXRecordDecl *Record) const final {
352+
return isRefCounted(Record) || isCheckedPtr(Record);
353+
}
354+
355+
bool isSafePtrType(const QualType type) const final {
356+
return isRefOrCheckedPtrType(type);
357+
}
358+
335359
const char *ptrKind() const final { return "unchecked"; }
336360
};
337361

0 commit comments

Comments
 (0)