Skip to content

Commit c26d097

Browse files
rniwarniwa
authored
[alpha.webkit.RetainPtrCtorAdoptChecker] Support adopt(cast(copy(~)) (#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 80267f8 commit c26d097

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
@@ -69,7 +69,7 @@ class RawPtrRefCallArgsChecker
6969
}
7070

7171
bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) override {
72-
if (isRefType(safeGetName(Decl)))
72+
if (isSmartPtrClass(safeGetName(Decl)))
7373
return true;
7474
return DynamicRecursiveASTVisitor::TraverseClassTemplateDecl(Decl);
7575
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,12 @@ class RawPtrRefLocalVarsChecker
261261
return DynamicRecursiveASTVisitor::TraverseCompoundStmt(CS);
262262
return true;
263263
}
264+
265+
bool TraverseClassTemplateDecl(ClassTemplateDecl *Decl) override {
266+
if (isSmartPtrClass(safeGetName(Decl)))
267+
return true;
268+
return DynamicRecursiveASTVisitor::TraverseClassTemplateDecl(Decl);
269+
}
264270
};
265271

266272
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 = nullptr;
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:
@@ -120,7 +121,7 @@ class RetainPtrCtorAdoptChecker
120121
return;
121122

122123
if (!isAdoptFn(F) || !CE->getNumArgs()) {
123-
rememberOutArguments(CE, F);
124+
checkCreateOrCopyFunction(CE, F, DeclWithIssue);
124125
return;
125126
}
126127

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

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

155+
bool hasOutArgument = false;
150156
unsigned ArgCount = CE->getNumArgs();
151157
for (unsigned ArgIndex = 0; ArgIndex < ArgCount; ++ArgIndex) {
152158
auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
@@ -165,7 +171,12 @@ class RetainPtrCtorAdoptChecker
165171
if (!Decl)
166172
continue;
167173
CreateOrCopyOutArguments.insert(Decl);
174+
hasOutArgument = true;
168175
}
176+
if (!RTC.isUnretained(Callee->getReturnType()))
177+
return;
178+
if (!hasOutArgument && !CreateOrCopyFnCall.contains(CE))
179+
reportLeak(CE, DeclWithIssue);
169180
}
170181

171182
void visitConstructExpr(const CXXConstructExpr *CE,
@@ -192,6 +203,13 @@ class RetainPtrCtorAdoptChecker
192203
std::string Name = "RetainPtr constructor";
193204
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
194205
auto Result = isOwned(Arg);
206+
207+
if (isCreateOrCopy(Arg))
208+
CreateOrCopyFnCall.insert(Arg); // Avoid double reporting.
209+
210+
if (Result == IsOwnedResult::Skip)
211+
return;
212+
195213
if (Result == IsOwnedResult::Unknown)
196214
Result = IsOwnedResult::NotOwned;
197215
if (Result == IsOwnedResult::Owned)
@@ -317,11 +335,22 @@ class RetainPtrCtorAdoptChecker
317335
if (auto *Callee = CE->getDirectCallee()) {
318336
if (isAdoptFn(Callee))
319337
return IsOwnedResult::NotOwned;
320-
if (safeGetName(Callee) == "__builtin___CFStringMakeConstantString")
338+
auto Name = safeGetName(Callee);
339+
if (Name == "__builtin___CFStringMakeConstantString")
321340
return IsOwnedResult::NotOwned;
341+
if ((Name == "checked_cf_cast" || Name == "dynamic_cf_cast" ||
342+
Name == "checked_objc_cast" || Name == "dynamic_objc_cast") &&
343+
CE->getNumArgs() == 1) {
344+
E = CE->getArg(0)->IgnoreParenCasts();
345+
continue;
346+
}
322347
auto RetType = Callee->getReturnType();
323348
if (isRetainPtrType(RetType))
324349
return IsOwnedResult::NotOwned;
350+
if (isCreateOrCopyFunction(Callee)) {
351+
CreateOrCopyFnCall.insert(CE);
352+
return IsOwnedResult::Owned;
353+
}
325354
} else if (auto *CalleeExpr = CE->getCallee()) {
326355
if (isa<CXXDependentScopeMemberExpr>(CalleeExpr))
327356
return IsOwnedResult::Skip; // Wait for instantiation.
@@ -387,6 +416,20 @@ class RetainPtrCtorAdoptChecker
387416
Report->setDeclWithIssue(DeclWithIssue);
388417
BR->emitReport(std::move(Report));
389418
}
419+
420+
void reportLeak(const CallExpr *CE, const Decl *DeclWithIssue) const {
421+
SmallString<100> Buf;
422+
llvm::raw_svector_ostream Os(Buf);
423+
424+
Os << "The return value is +1 and results in a memory leak.";
425+
426+
PathDiagnosticLocation BSLoc(CE->getSourceRange().getBegin(),
427+
BR->getSourceManager());
428+
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
429+
Report->addRange(CE->getSourceRange());
430+
Report->setDeclWithIssue(DeclWithIssue);
431+
BR->emitReport(std::move(Report));
432+
}
390433
};
391434
} // namespace
392435

‎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
@@ -3,6 +3,8 @@
33

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

6+
CFTypeRef CFCopyArray(CFArrayRef);
7+
68
void basic_correct() {
79
auto ns1 = adoptNS([SomeObj alloc]);
810
auto ns2 = adoptNS([[SomeObj alloc] init]);
@@ -12,6 +14,7 @@ void basic_correct() {
1214
auto ns6 = retainPtr([ns3 next]);
1315
CFMutableArrayRef cf1 = adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10));
1416
auto cf2 = adoptCF(SecTaskCreateFromSelf(kCFAllocatorDefault));
17+
auto cf3 = adoptCF(checked_cf_cast<CFArrayRef>(CFCopyArray(cf1)));
1518
}
1619

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

3237
RetainPtr<CVPixelBufferRef> cf_out_argument() {
@@ -68,7 +73,7 @@ void adopt_retainptr() {
6873

6974
class MemberInit {
7075
public:
71-
MemberInit(CFMutableArrayRef array, NSString *str, CFRunLoopRef runLoop)
76+
MemberInit(RetainPtr<CFMutableArrayRef>&& array, NSString *str, CFRunLoopRef runLoop)
7277
: m_array(array)
7378
, m_str(str)
7479
, m_runLoop(runLoop)
@@ -80,7 +85,7 @@ void adopt_retainptr() {
8085
RetainPtr<CFRunLoopRef> m_runLoop;
8186
};
8287
void create_member_init() {
83-
MemberInit init { CFArrayCreateMutable(kCFAllocatorDefault, 10), @"hello", CFRunLoopGetCurrent() };
88+
MemberInit init { adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)), @"hello", CFRunLoopGetCurrent() };
8489
}
8590

8691
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
@@ -3,6 +3,9 @@
33

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

6+
CFTypeRef CFCopyArray(CFArrayRef);
7+
void* CreateCopy();
8+
69
void basic_correct() {
710
auto ns1 = adoptNS([SomeObj alloc]);
811
auto ns2 = adoptNS([[SomeObj alloc] init]);
@@ -12,6 +15,8 @@ void basic_correct() {
1215
auto ns6 = retainPtr([ns3 next]);
1316
CFMutableArrayRef cf1 = adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10));
1417
auto cf2 = adoptCF(SecTaskCreateFromSelf(kCFAllocatorDefault));
18+
auto cf3 = adoptCF(checked_cf_cast<CFArrayRef>(CFCopyArray(cf1)));
19+
CreateCopy();
1520
}
1621

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

3239
RetainPtr<CVPixelBufferRef> cf_out_argument() {
@@ -68,7 +75,7 @@ void adopt_retainptr() {
6875

6976
class MemberInit {
7077
public:
71-
MemberInit(CFMutableArrayRef array, NSString *str, CFRunLoopRef runLoop)
78+
MemberInit(RetainPtr<CFMutableArrayRef>&& array, NSString *str, CFRunLoopRef runLoop)
7279
: m_array(array)
7380
, m_str(str)
7481
, m_runLoop(runLoop)
@@ -80,7 +87,7 @@ void adopt_retainptr() {
8087
RetainPtr<CFRunLoopRef> m_runLoop;
8188
};
8289
void create_member_init() {
83-
MemberInit init { CFArrayCreateMutable(kCFAllocatorDefault, 10), @"hello", CFRunLoopGetCurrent() };
90+
MemberInit init { adoptCF(CFArrayCreateMutable(kCFAllocatorDefault, 10)), @"hello", CFRunLoopGetCurrent() };
8491
}
8592

8693
RetainPtr<CFStringRef> cfstr() {

0 commit comments

Comments
 (0)