Skip to content

Commit 1ce5827

Browse files
rniwarniwa
committed
[alpha.webkit.RetainPtrCtorAdoptChecker] Support adopt(cast(copy(~)) (llvm#132316)
This PR adds the support for recognizing calling adoptCF/adoptNS on the result of a cast operation on the return value of a function which creates NS or CF types. It also fixes a bug that we weren't reporting memory leaks when CF types are created without ever calling RetainPtr's constructor, adoptCF, or adoptNS. To do this, this PR adds a new mechanism to report a memory leak whenever create or copy CF functions are invoked unless this CallExpr has already been visited while validating a call to adoptCF. Also added an early exit when isOwned returns IsOwnedResult::Skip due to an unresolved template argument.
1 parent 91d64ea commit 1ce5827

File tree

8 files changed

+132
-38
lines changed

8 files changed

+132
-38
lines changed

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

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,16 @@ bool isCheckedPtr(const std::string &Name) {
125125
return Name == "CheckedPtr" || Name == "CheckedRef";
126126
}
127127

128+
bool isSmartPtrClass(const std::string &Name) {
129+
return isRefType(Name) || isCheckedPtr(Name) || isRetainPtr(Name) ||
130+
Name == "WeakPtr" || Name == "WeakPtr" || Name == "WeakPtrFactory" ||
131+
Name == "WeakPtrFactoryWithBitField" || Name == "WeakPtrImplBase" ||
132+
Name == "WeakPtrImplBaseSingleThread" || Name == "ThreadSafeWeakPtr" ||
133+
Name == "ThreadSafeWeakOrStrongPtr" ||
134+
Name == "ThreadSafeWeakPtrControlBlock" ||
135+
Name == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr";
136+
}
137+
128138
bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
129139
assert(F);
130140
const std::string &FunctionName = safeGetName(F);
@@ -222,8 +232,13 @@ void RetainTypeChecker::visitTypedef(const TypedefDecl *TD) {
222232

223233
auto PointeeQT = QT->getPointeeType();
224234
const RecordType *RT = PointeeQT->getAs<RecordType>();
225-
if (!RT)
235+
if (!RT) {
236+
if (TD->hasAttr<ObjCBridgeAttr>() || TD->hasAttr<ObjCBridgeMutableAttr>()) {
237+
if (auto *Type = TD->getTypeForDecl())
238+
RecordlessTypes.insert(Type);
239+
}
226240
return;
241+
}
227242

228243
for (auto *Redecl : RT->getDecl()->getMostRecentDecl()->redecls()) {
229244
if (Redecl->getAttr<ObjCBridgeAttr>() ||
@@ -240,6 +255,17 @@ bool RetainTypeChecker::isUnretained(const QualType QT, bool ignoreARC) {
240255
auto CanonicalType = QT.getCanonicalType();
241256
auto PointeeType = CanonicalType->getPointeeType();
242257
auto *RT = dyn_cast_or_null<RecordType>(PointeeType.getTypePtrOrNull());
258+
if (!RT) {
259+
auto *Type = QT.getTypePtrOrNull();
260+
while (Type) {
261+
if (RecordlessTypes.contains(Type))
262+
return true;
263+
auto *ET = dyn_cast_or_null<ElaboratedType>(Type);
264+
if (!ET)
265+
break;
266+
Type = ET->desugar().getTypePtrOrNull();
267+
}
268+
}
243269
return RT && CFPointees.contains(RT);
244270
}
245271

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ std::optional<bool> isUnchecked(const clang::QualType T);
7070
/// underlying pointer type.
7171
class RetainTypeChecker {
7272
llvm::DenseSet<const RecordType *> CFPointees;
73+
llvm::DenseSet<const Type *> RecordlessTypes;
7374
bool IsARCEnabled{false};
7475

7576
public:
@@ -135,6 +136,9 @@ bool isCheckedPtr(const std::string &Name);
135136
/// \returns true if \p Name is RetainPtr or its variant, false if not.
136137
bool isRetainPtr(const std::string &Name);
137138

139+
/// \returns true if \p Name is a smart pointer type name, false if not.
140+
bool isSmartPtrClass(const std::string &Name);
141+
138142
/// \returns true if \p M is getter of a ref-counted class, false if not.
139143
std::optional<bool> isGetterOfSafePtr(const clang::CXXMethodDecl *Method);
140144

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class RawPtrRefCallArgsChecker
7272
bool shouldVisitImplicitCode() const { return false; }
7373

7474
bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) {
75-
if (isRefType(safeGetName(Decl)))
75+
if (isSmartPtrClass(safeGetName(Decl)))
7676
return true;
7777
return Base::TraverseClassTemplateDecl(Decl);
7878
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,12 @@ class RawPtrRefLocalVarsChecker
267267
return Base::TraverseCompoundStmt(CS);
268268
return true;
269269
}
270+
271+
bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) {
272+
if (isSmartPtrClass(safeGetName(Decl)))
273+
return true;
274+
return Base::TraverseClassTemplateDecl(Decl);
275+
}
270276
};
271277

272278
LocalVisitor visitor(this);

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

Lines changed: 57 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ class RetainPtrCtorAdoptChecker
3434
mutable BugReporter *BR;
3535
mutable std::unique_ptr<RetainSummaryManager> Summaries;
3636
mutable llvm::DenseSet<const ValueDecl *> CreateOrCopyOutArguments;
37+
mutable llvm::DenseSet<const Expr *> CreateOrCopyFnCall;
3738
mutable RetainTypeChecker RTC;
3839

3940
public:
@@ -119,7 +120,7 @@ class RetainPtrCtorAdoptChecker
119120
return;
120121

121122
if (!isAdoptFn(F) || !CE->getNumArgs()) {
122-
rememberOutArguments(CE, F);
123+
checkCreateOrCopyFunction(CE, F, DeclWithIssue);
123124
return;
124125
}
125126

@@ -128,24 +129,29 @@ class RetainPtrCtorAdoptChecker
128129
auto Name = safeGetName(F);
129130
if (Result == IsOwnedResult::Unknown)
130131
Result = IsOwnedResult::NotOwned;
131-
if (Result == IsOwnedResult::NotOwned && !isAllocInit(Arg) &&
132-
!isCreateOrCopy(Arg)) {
133-
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
134-
if (CreateOrCopyOutArguments.contains(DRE->getDecl()))
135-
return;
136-
}
137-
if (RTC.isARCEnabled() && isAdoptNS(F))
138-
reportUseAfterFree(Name, CE, DeclWithIssue, "when ARC is disabled");
139-
else
140-
reportUseAfterFree(Name, CE, DeclWithIssue);
132+
if (isAllocInit(Arg) || isCreateOrCopy(Arg)) {
133+
CreateOrCopyFnCall.insert(Arg); // Avoid double reporting.
134+
return;
141135
}
136+
if (Result == IsOwnedResult::Owned || Result == IsOwnedResult::Skip)
137+
return;
138+
139+
if (auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
140+
if (CreateOrCopyOutArguments.contains(DRE->getDecl()))
141+
return;
142+
}
143+
if (RTC.isARCEnabled() && isAdoptNS(F))
144+
reportUseAfterFree(Name, CE, DeclWithIssue, "when ARC is disabled");
145+
else
146+
reportUseAfterFree(Name, CE, DeclWithIssue);
142147
}
143148

144-
void rememberOutArguments(const CallExpr *CE,
145-
const FunctionDecl *Callee) const {
149+
void checkCreateOrCopyFunction(const CallExpr *CE, const FunctionDecl *Callee,
150+
const Decl *DeclWithIssue) const {
146151
if (!isCreateOrCopyFunction(Callee))
147152
return;
148153

154+
bool hasOutArgument = false;
149155
unsigned ArgCount = CE->getNumArgs();
150156
for (unsigned ArgIndex = 0; ArgIndex < ArgCount; ++ArgIndex) {
151157
auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
@@ -164,7 +170,12 @@ class RetainPtrCtorAdoptChecker
164170
if (!Decl)
165171
continue;
166172
CreateOrCopyOutArguments.insert(Decl);
173+
hasOutArgument = true;
167174
}
175+
if (!RTC.isUnretained(Callee->getReturnType()))
176+
return;
177+
if (!hasOutArgument && !CreateOrCopyFnCall.contains(CE))
178+
reportLeak(CE, DeclWithIssue);
168179
}
169180

170181
void visitConstructExpr(const CXXConstructExpr *CE,
@@ -190,6 +201,13 @@ class RetainPtrCtorAdoptChecker
190201
std::string Name = "RetainPtr constructor";
191202
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
192203
auto Result = isOwned(Arg);
204+
205+
if (isCreateOrCopy(Arg))
206+
CreateOrCopyFnCall.insert(Arg); // Avoid double reporting.
207+
208+
if (Result == IsOwnedResult::Skip)
209+
return;
210+
193211
if (Result == IsOwnedResult::Unknown)
194212
Result = IsOwnedResult::NotOwned;
195213
if (Result == IsOwnedResult::Owned)
@@ -315,11 +333,22 @@ class RetainPtrCtorAdoptChecker
315333
if (auto *Callee = CE->getDirectCallee()) {
316334
if (isAdoptFn(Callee))
317335
return IsOwnedResult::NotOwned;
318-
if (safeGetName(Callee) == "__builtin___CFStringMakeConstantString")
336+
auto Name = safeGetName(Callee);
337+
if (Name == "__builtin___CFStringMakeConstantString")
319338
return IsOwnedResult::NotOwned;
339+
if ((Name == "checked_cf_cast" || Name == "dynamic_cf_cast" ||
340+
Name == "checked_objc_cast" || Name == "dynamic_objc_cast") &&
341+
CE->getNumArgs() == 1) {
342+
E = CE->getArg(0)->IgnoreParenCasts();
343+
continue;
344+
}
320345
auto RetType = Callee->getReturnType();
321346
if (isRetainPtrType(RetType))
322347
return IsOwnedResult::NotOwned;
348+
if (isCreateOrCopyFunction(Callee)) {
349+
CreateOrCopyFnCall.insert(CE);
350+
return IsOwnedResult::Owned;
351+
}
323352
} else if (auto *CalleeExpr = CE->getCallee()) {
324353
if (isa<CXXDependentScopeMemberExpr>(CalleeExpr))
325354
return IsOwnedResult::Skip; // Wait for instantiation.
@@ -383,6 +412,20 @@ class RetainPtrCtorAdoptChecker
383412
Report->setDeclWithIssue(DeclWithIssue);
384413
BR->emitReport(std::move(Report));
385414
}
415+
416+
void reportLeak(const CallExpr *CE, const Decl *DeclWithIssue) const {
417+
SmallString<100> Buf;
418+
llvm::raw_svector_ostream Os(Buf);
419+
420+
Os << "The return value is +1 and results in a memory leak.";
421+
422+
PathDiagnosticLocation BSLoc(CE->getSourceRange().getBegin(),
423+
BR->getSourceManager());
424+
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
425+
Report->addRange(CE->getSourceRange());
426+
Report->setDeclWithIssue(DeclWithIssue);
427+
BR->emitReport(std::move(Report));
428+
}
386429
};
387430
} // namespace
388431

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

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ typedef struct CF_BRIDGED_MUTABLE_TYPE(CFRunLoopRef) __CFRunLoop * CFRunLoopRef;
2424

2525
extern const CFAllocatorRef kCFAllocatorDefault;
2626
typedef struct _NSZone NSZone;
27+
28+
CFTypeID CFGetTypeID(CFTypeRef cf);
29+
2730
CFTypeID CFGetTypeID(CFTypeRef cf);
2831
CFTypeID CFArrayGetTypeID();
2932
CFMutableArrayRef CFArrayCreateMutable(CFAllocatorRef allocator, CFIndex capacity);
@@ -281,12 +284,12 @@ template<typename T> inline RetainPtr<T> retainPtr(T* ptr)
281284

282285
inline NSObject *bridge_cast(CFTypeRef object)
283286
{
284-
return (__bridge NSObject *)object;
287+
return (__bridge NSObject *)object;
285288
}
286289

287290
inline CFTypeRef bridge_cast(NSObject *object)
288291
{
289-
return (__bridge CFTypeRef)object;
292+
return (__bridge CFTypeRef)object;
290293
}
291294

292295
inline id bridge_id_cast(CFTypeRef object)
@@ -386,35 +389,35 @@ template <typename> struct CFTypeTrait;
386389

387390
template<typename T> T dynamic_cf_cast(CFTypeRef object)
388391
{
389-
if (!object)
390-
return nullptr;
392+
if (!object)
393+
return nullptr;
391394

392-
if (CFGetTypeID(object) != CFTypeTrait<T>::typeID())
393-
return nullptr;
395+
if (CFGetTypeID(object) != CFTypeTrait<T>::typeID())
396+
return nullptr;
394397

395-
return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object));
398+
return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object));
396399
}
397400

398401
template<typename T> T checked_cf_cast(CFTypeRef object)
399402
{
400-
if (!object)
401-
return nullptr;
403+
if (!object)
404+
return nullptr;
402405

403-
if (CFGetTypeID(object) != CFTypeTrait<T>::typeID())
404-
WTFCrash();
406+
if (CFGetTypeID(object) != CFTypeTrait<T>::typeID())
407+
WTFCrash();
405408

406-
return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object));
409+
return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object));
407410
}
408411

409412
template<typename T, typename U> RetainPtr<T> dynamic_cf_cast(RetainPtr<U>&& object)
410413
{
411-
if (!object)
412-
return nullptr;
414+
if (!object)
415+
return nullptr;
413416

414-
if (CFGetTypeID(object.get()) != CFTypeTrait<T>::typeID())
415-
return nullptr;
417+
if (CFGetTypeID(object.get()) != CFTypeTrait<T>::typeID())
418+
return nullptr;
416419

417-
return adoptCF(static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object.leakRef())));
420+
return adoptCF(static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object.leakRef())));
418421
}
419422

420423
} // namespace WTF
@@ -448,4 +451,4 @@ using WTF::is_objc;
448451
using WTF::checked_objc_cast;
449452
using WTF::dynamic_objc_cast;
450453
using WTF::checked_cf_cast;
451-
using WTF::dynamic_cf_cast;
454+
using WTF::dynamic_cf_cast;

‎clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use-arc.mm

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
#include "objc-mock-types.h"
44

5+
CFTypeRef CFCopyArray(CFArrayRef);
6+
57
void basic_correct() {
68
auto ns1 = adoptNS([SomeObj alloc]);
79
auto ns2 = adoptNS([[SomeObj alloc] init]);
@@ -11,6 +13,7 @@ void basic_correct() {
1113
auto ns6 = retainPtr([ns3 next]);
1214
CFMutableArrayRef cf1 = adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10));
1315
auto cf2 = adoptCF(SecTaskCreateFromSelf(kCFAllocatorDefault));
16+
auto cf3 = adoptCF(checked_cf_cast<CFArrayRef>(CFCopyArray(cf1)));
1417
}
1518

1619
CFMutableArrayRef provide_cf();
@@ -26,6 +29,8 @@ void basic_wrong() {
2629
// expected-warning@-1{{Incorrect use of adoptCF. The argument is +0 and results in an use-after-free [alpha.webkit.RetainPtrCtorAdoptChecker]}}
2730
RetainPtr<CFTypeRef> cf3 = SecTaskCreateFromSelf(kCFAllocatorDefault);
2831
// expected-warning@-1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}}
32+
CFCopyArray(cf1);
33+
// expected-warning@-1{{The return value is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}}
2934
}
3035

3136
RetainPtr<CVPixelBufferRef> cf_out_argument() {
@@ -67,7 +72,7 @@ void adopt_retainptr() {
6772

6873
class MemberInit {
6974
public:
70-
MemberInit(CFMutableArrayRef array, NSString *str, CFRunLoopRef runLoop)
75+
MemberInit(RetainPtr<CFMutableArrayRef>&& array, NSString *str, CFRunLoopRef runLoop)
7176
: m_array(array)
7277
, m_str(str)
7378
, m_runLoop(runLoop)
@@ -79,7 +84,7 @@ void adopt_retainptr() {
7984
RetainPtr<CFRunLoopRef> m_runLoop;
8085
};
8186
void create_member_init() {
82-
MemberInit init { CFArrayCreateMutable(kCFAllocatorDefault, 10), @"hello", CFRunLoopGetCurrent() };
87+
MemberInit init { adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)), @"hello", CFRunLoopGetCurrent() };
8388
}
8489

8590
RetainPtr<CFStringRef> cfstr() {

‎clang/test/Analysis/Checkers/WebKit/retain-ptr-ctor-adopt-use.mm

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22

33
#include "objc-mock-types.h"
44

5+
CFTypeRef CFCopyArray(CFArrayRef);
6+
void* CreateCopy();
7+
58
void basic_correct() {
69
auto ns1 = adoptNS([SomeObj alloc]);
710
auto ns2 = adoptNS([[SomeObj alloc] init]);
@@ -11,6 +14,8 @@ void basic_correct() {
1114
auto ns6 = retainPtr([ns3 next]);
1215
CFMutableArrayRef cf1 = adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10));
1316
auto cf2 = adoptCF(SecTaskCreateFromSelf(kCFAllocatorDefault));
17+
auto cf3 = adoptCF(checked_cf_cast<CFArrayRef>(CFCopyArray(cf1)));
18+
CreateCopy();
1419
}
1520

1621
CFMutableArrayRef provide_cf();
@@ -26,6 +31,8 @@ void basic_wrong() {
2631
// expected-warning@-1{{Incorrect use of adoptCF. The argument is +0 and results in an use-after-free [alpha.webkit.RetainPtrCtorAdoptChecker]}}
2732
RetainPtr<CFTypeRef> cf3 = SecTaskCreateFromSelf(kCFAllocatorDefault);
2833
// expected-warning@-1{{Incorrect use of RetainPtr constructor. The argument is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}}
34+
CFCopyArray(cf1);
35+
// expected-warning@-1{{The return value is +1 and results in a memory leak [alpha.webkit.RetainPtrCtorAdoptChecker]}}
2936
}
3037

3138
RetainPtr<CVPixelBufferRef> cf_out_argument() {
@@ -67,7 +74,7 @@ void adopt_retainptr() {
6774

6875
class MemberInit {
6976
public:
70-
MemberInit(CFMutableArrayRef array, NSString *str, CFRunLoopRef runLoop)
77+
MemberInit(RetainPtr<CFMutableArrayRef>&& array, NSString *str, CFRunLoopRef runLoop)
7178
: m_array(array)
7279
, m_str(str)
7380
, m_runLoop(runLoop)
@@ -79,7 +86,7 @@ void adopt_retainptr() {
7986
RetainPtr<CFRunLoopRef> m_runLoop;
8087
};
8188
void create_member_init() {
82-
MemberInit init { CFArrayCreateMutable(kCFAllocatorDefault, 10), @"hello", CFRunLoopGetCurrent() };
89+
MemberInit init { adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)), @"hello", CFRunLoopGetCurrent() };
8390
}
8491

8592
RetainPtr<CFStringRef> cfstr() {

0 commit comments

Comments
 (0)