Skip to content

Commit 642057c

Browse files
rniwarniwa
committed
[alpha.webkit.RetainPtrCtorAdoptChecker] Support adopt(cast(copy(~))
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 fe6bced commit 642057c

File tree

4 files changed

+140
-20
lines changed

4 files changed

+140
-20
lines changed

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

Lines changed: 56 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,30 @@ 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;
135+
}
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;
141142
}
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,
150+
const FunctionDecl *Callee,
151+
const Decl *DeclWithIssue) const {
146152
if (!isCreateOrCopyFunction(Callee))
147153
return;
148154

155+
bool hasOutArgument = false;
149156
unsigned ArgCount = CE->getNumArgs();
150157
for (unsigned ArgIndex = 0; ArgIndex < ArgCount; ++ArgIndex) {
151158
auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts();
@@ -164,7 +171,10 @@ class RetainPtrCtorAdoptChecker
164171
if (!Decl)
165172
continue;
166173
CreateOrCopyOutArguments.insert(Decl);
174+
hasOutArgument = true;
167175
}
176+
if (!hasOutArgument && !CreateOrCopyFnCall.contains(CE))
177+
reportLeak(CE, DeclWithIssue);
168178
}
169179

170180
void visitConstructExpr(const CXXConstructExpr *CE,
@@ -190,6 +200,13 @@ class RetainPtrCtorAdoptChecker
190200
std::string Name = "RetainPtr constructor";
191201
auto *Arg = CE->getArg(0)->IgnoreParenCasts();
192202
auto Result = isOwned(Arg);
203+
204+
if (isCreateOrCopy(Arg))
205+
CreateOrCopyFnCall.insert(Arg); // Avoid double reporting.
206+
207+
if (Result == IsOwnedResult::Skip)
208+
return;
209+
193210
if (Result == IsOwnedResult::Unknown)
194211
Result = IsOwnedResult::NotOwned;
195212
if (Result == IsOwnedResult::Owned)
@@ -303,11 +320,22 @@ class RetainPtrCtorAdoptChecker
303320
if (auto *Callee = CE->getDirectCallee()) {
304321
if (isAdoptFn(Callee))
305322
return IsOwnedResult::NotOwned;
306-
if (safeGetName(Callee) == "__builtin___CFStringMakeConstantString")
323+
auto Name = safeGetName(Callee);
324+
if (Name == "__builtin___CFStringMakeConstantString")
307325
return IsOwnedResult::NotOwned;
326+
if ((Name == "checked_cf_cast" || Name == "dynamic_cf_cast" ||
327+
Name == "checked_objc_cast" || Name == "dynamic_objc_cast") &&
328+
CE->getNumArgs() == 1) {
329+
E = CE->getArg(0)->IgnoreParenCasts();
330+
continue;
331+
}
308332
auto RetType = Callee->getReturnType();
309333
if (isRetainPtrType(RetType))
310334
return IsOwnedResult::NotOwned;
335+
if (isCreateOrCopyFunction(Callee)) {
336+
CreateOrCopyFnCall.insert(CE);
337+
return IsOwnedResult::Owned;
338+
}
311339
} else if (auto *CalleeExpr = CE->getCallee()) {
312340
if (isa<CXXDependentScopeMemberExpr>(CalleeExpr))
313341
return IsOwnedResult::Skip; // Wait for instantiation.
@@ -371,6 +399,20 @@ class RetainPtrCtorAdoptChecker
371399
Report->setDeclWithIssue(DeclWithIssue);
372400
BR->emitReport(std::move(Report));
373401
}
402+
403+
void reportLeak(const CallExpr *CE, const Decl *DeclWithIssue) const {
404+
SmallString<100> Buf;
405+
llvm::raw_svector_ostream Os(Buf);
406+
407+
Os << "The return value is +1 and results in a memory leak.";
408+
409+
PathDiagnosticLocation BSLoc(CE->getSourceRange().getBegin(),
410+
BR->getSourceManager());
411+
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
412+
Report->addRange(CE->getSourceRange());
413+
Report->setDeclWithIssue(DeclWithIssue);
414+
BR->emitReport(std::move(Report));
415+
}
374416
};
375417
} // namespace
376418

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

Lines changed: 70 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ typedef struct CF_BRIDGED_MUTABLE_TYPE(CFRunLoopRef) __CFRunLoop * CFRunLoopRef;
2121

2222
extern const CFAllocatorRef kCFAllocatorDefault;
2323
typedef struct _NSZone NSZone;
24+
typedef unsigned long CFTypeID;
25+
26+
CFTypeID CFGetTypeID(CFTypeRef cf);
27+
28+
CFTypeID CFArrayGetTypeID(void);
2429
CFMutableArrayRef CFArrayCreateMutable(CFAllocatorRef allocator, CFIndex capacity);
2530
extern void CFArrayAppendValue(CFMutableArrayRef theArray, const void *value);
2631
CFArrayRef CFArrayCreate(CFAllocatorRef allocator, const void **values, CFIndex numValues);
@@ -29,6 +34,7 @@ CFIndex CFArrayGetCount(CFArrayRef theArray);
2934
typedef const struct CF_BRIDGED_TYPE(NSDictionary) __CFDictionary * CFDictionaryRef;
3035
typedef struct CF_BRIDGED_MUTABLE_TYPE(NSMutableDictionary) __CFDictionary * CFMutableDictionaryRef;
3136

37+
CFTypeID CFDictionaryGetTypeID(void);
3238
CFDictionaryRef CFDictionaryCreate(CFAllocatorRef allocator, const void **keys, const void **values, CFIndex numValues);
3339
CFDictionaryRef CFDictionaryCreateCopy(CFAllocatorRef allocator, CFDictionaryRef theDict);
3440
CFDictionaryRef CFDictionaryCreateMutableCopy(CFAllocatorRef allocator, CFIndex capacity, CFDictionaryRef theDict);
@@ -135,6 +141,8 @@ __attribute__((objc_root_class))
135141

136142
namespace WTF {
137143

144+
void WTFCrash(void);
145+
138146
template<typename T> class RetainPtr;
139147
template<typename T> RetainPtr<T> adoptNS(T*);
140148
template<typename T> RetainPtr<T> adoptCF(T);
@@ -265,19 +273,79 @@ template<typename T> inline RetainPtr<T> retainPtr(T* ptr)
265273

266274
inline NSObject *bridge_cast(CFTypeRef object)
267275
{
268-
return (__bridge NSObject *)object;
276+
return (__bridge NSObject *)object;
269277
}
270278

271279
inline CFTypeRef bridge_cast(NSObject *object)
272280
{
273-
return (__bridge CFTypeRef)object;
281+
return (__bridge CFTypeRef)object;
282+
}
283+
284+
template <typename> struct CFTypeTrait;
285+
286+
// Use dynamic_cf_cast<> instead of checked_cf_cast<> when actively checking CF types,
287+
// similar to dynamic_cast<> in C++. Be sure to include a nullptr check.
288+
289+
template<typename T> T dynamic_cf_cast(CFTypeRef object)
290+
{
291+
if (!object)
292+
return nullptr;
293+
294+
if (CFGetTypeID(object) != CFTypeTrait<T>::typeID())
295+
return nullptr;
296+
297+
return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object));
298+
}
299+
300+
template<typename T, typename U> RetainPtr<T> dynamic_cf_cast(RetainPtr<U>&& object)
301+
{
302+
if (!object)
303+
return nullptr;
304+
305+
if (CFGetTypeID(object.get()) != CFTypeTrait<T>::typeID())
306+
return nullptr;
307+
308+
return adoptCF(static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object.leakRef())));
274309
}
275310

311+
// Use checked_cf_cast<> instead of dynamic_cf_cast<> when a specific CF type is required.
312+
313+
template<typename T> T checked_cf_cast(CFTypeRef object)
314+
{
315+
if (!object)
316+
return nullptr;
317+
318+
if (CFGetTypeID(object) != CFTypeTrait<T>::typeID())
319+
WTFCrash();
320+
321+
return static_cast<T>(const_cast<CF_BRIDGED_TYPE(id) void*>(object));
276322
}
277323

324+
} // namespace WTF
325+
326+
#define WTF_DECLARE_CF_TYPE_TRAIT(ClassName) \
327+
template <> \
328+
struct WTF::CFTypeTrait<ClassName##Ref> { \
329+
static inline CFTypeID typeID(void) { return ClassName##GetTypeID(); } \
330+
};
331+
332+
WTF_DECLARE_CF_TYPE_TRAIT(CFArray);
333+
WTF_DECLARE_CF_TYPE_TRAIT(CFDictionary);
334+
335+
#define WTF_DECLARE_CF_MUTABLE_TYPE_TRAIT(ClassName, MutableClassName) \
336+
template <> \
337+
struct WTF::CFTypeTrait<MutableClassName##Ref> { \
338+
static inline CFTypeID typeID(void) { return ClassName##GetTypeID(); } \
339+
};
340+
341+
WTF_DECLARE_CF_MUTABLE_TYPE_TRAIT(CFArray, CFMutableArray);
342+
WTF_DECLARE_CF_MUTABLE_TYPE_TRAIT(CFDictionary, CFMutableDictionary);
343+
278344
using WTF::RetainPtr;
279345
using WTF::adoptNS;
280346
using WTF::adoptCF;
281347
using WTF::retainPtr;
282348
using WTF::downcast;
283349
using WTF::bridge_cast;
350+
using WTF::dynamic_cf_cast;
351+
using WTF::checked_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: 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() {

0 commit comments

Comments
 (0)