[analyzer] RetainCountChecker: The callback in dispatch_data_create() doesn't free the return symbol.

[NoQ]

Summary

The hack that was previously applied to CGBitmapContextCreateWithData() is now extended to another function, dispatch_data_create().

This function accepts a callback block, and the analyzer erroneously assumes that this callback may free up some data, which results in being unable to detect a leak of the dispatch_data_t object. In fact, the callback only frees the buffer that is passed into the function, but the returned object should be released separately unless in ARC mode.

[NoQ]

Update the comments in the surrounding code accordingly.

[zaks.anna]

test/Analysis/dispatch-data-leak.m

50 ↗

(On Diff #80264)

I think we should remove testing of malloc -it does not add anything here and is a bit confusing since one might think that malloc_"but is related to the dispatch test case.

[dcoughlin]

Looks good to me!

To ease the future maintenance burden I would suggest moving the test into 'retain-release-arc.m' unless it really needs to be in its own separate file.

test/Analysis/dispatch-data-leak.m

59 ↗

(On Diff #80264)

I think it would also be good to also add tests showing that we don't warn when the object is properly released. It would be good to test both ObjC -release (under MRR) and the dispatch_release() macro (under MRR and ARC).

[NoQ]

Add more tests.

Not sure we need to stay merged with retain-release-arc.m, as we're not really reusing many declarations across these files.

As well, handle the case when dispatch_retain and dispatch_release are regular functions.

[dcoughlin]

In D27409#614601, @NoQ wrote:

Not sure we need to stay merged with retain-release-arc.m, as we're not really reusing many declarations across these files.

I find it more helpful to organize tests around functionality ('retain count checker') than around bugs ('dispatch data leak', 'PRxyzab.c'). Organizing around functionality helps future contributors and maintainers know where to add a new test and where to look for existing tests. In my view this critical for developers who are new to the project or who are learning a new subsystem. It also typically reduces the number of 'clang' RUN lines, which have a cost in maintenance and running time.

[dcoughlin]

The analyzer currently doesn't do any checking for dispatch retain/release APIs in C. It similarly doesn't do any checking in Objective-C when OS_OBJECT_USE_OBJC is 0 (and thus the dispatch types are defined to their C-struct versions). This happens when the user explicitly sets -DOS_OBJECT_USE_OBJC=0 (to safely share dispatch between ARC and non-ARC projects) and also under certain combinations of deployment targets and architectures where the runtime support for the feature is not present (for example, earlier than iOS 6.0).

My feeling is that for this patch it is fine to continue the policy of not diagnosing dispatch leaks/overreleases when OS_OBJECT_USE_OBJC is 0. Adding this support to the retain count checker is a larger project and in my opinion it should be done separately. To that end, I would recommend removing the summary creation for dispatch_retain/dispatch_release().

That said, people do use these APIs in C (and with -DOS_OBJECT_USE_OBJC=0 in ObjC), so it would be great test to make sure that this patch don't introduce any new false positives in these situations. I think it important to suck in enough of the header typedefs used when -DOS_OBJECT_USE_OBJC=0 to write these tests to make sure we don't regress when using the C-based APIs.

test/Analysis/dispatch-data-leak.m

4 ↗

(On Diff #80432)

Looks like there is a typo here ('MARCOS' vs. 'MACROS')?

[NoQ]

Revert the attempt to support dispatch_{retain,release}. Disable the relevant tests.

Move tests to retain-release-arc.m.

[dcoughlin]

Ship it! :-)

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp

if (Call.hasNonZeroCallbackArg()) {
 
     // Special cases where the callback argument CANNOT free the return value.
     // This can generally only happen if we know that the callback will only be
     // called when the return value is already being deallocated.
     if (const SimpleFunctionCall *FC = dyn_cast<SimpleFunctionCall>(&Call)) {
       if (IdentifierInfo *Name = FC->getDecl()->getIdentifier()) {
         // When the CGBitmapContext is deallocated, the callback here will free
         // the associated data buffer.
-        if (Name->isStr("CGBitmapContextCreateWithData"))
+        // The callback in dispatch_data_create frees the buffer, but not
+        // the data object.
+        if (Name->isStr("CGBitmapContextCreateWithData") ||
+            Name->isStr("dispatch_data_create"))
           RE = S->getRetEffect();
       }
     }
 
     S = getPersistentSummary(RE, RecEffect, DefEffect);
   }
 
   // Special case '[super init];' and '[self init];'

cfe/trunk/test/Analysis/retain-release-arc.m

 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -analyze -analyzer-checker=core,osx.coreFoundation.CFRetainRelease,osx.cocoa.ClassRelease,osx.cocoa.RetainCount -fobjc-arc -fblocks -verify -Wno-objc-root-class %s -analyzer-output=text
 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -analyze -analyzer-checker=core,osx.coreFoundation.CFRetainRelease,osx.cocoa.ClassRelease,osx.cocoa.RetainCount -fblocks -verify -Wno-objc-root-class %s -analyzer-output=text
 
+typedef __typeof(sizeof(int)) size_t;
+
 #define HAS_ARC __has_feature(objc_arc)
 
 typedef unsigned long long CFOptionFlags;
 typedef signed long long CFIndex;
 
 typedef CFIndex CFPropertyListFormat; enum {
     kCFPropertyListOpenStepFormat = 1,
     kCFPropertyListXMLFormat_v1_0 = 100,

 + (id)alloc;
 + (id)new;
 - (void)dealloc;
 @end
 
 @interface NSDictionary : NSObject
 @end
 
+#define OS_OBJECT_RETURNS_RETAINED __attribute__((__ns_returns_retained__))
+#define DISPATCH_RETURNS_RETAINED OS_OBJECT_RETURNS_RETAINED
+
+@protocol OS_dispatch_object
+@end
+@protocol OS_dispatch_data <OS_dispatch_object>
+@end
+@protocol OS_dispatch_queue <OS_dispatch_object>
+@end
+
+typedef NSObject<OS_dispatch_object> *dispatch_object_t;
+typedef NSObject<OS_dispatch_data> *dispatch_data_t;
+typedef NSObject<OS_dispatch_queue> *dispatch_queue_t;
+
+typedef void (^dispatch_block_t)(void);
+
+dispatch_queue_t dispatch_get_main_queue(void);
+
+DISPATCH_RETURNS_RETAINED dispatch_data_t
+dispatch_data_create(const void *buffer, size_t size,
+                     dispatch_queue_t _Nullable queue,
+                     dispatch_block_t _Nullable destructor);
+
+void _dispatch_object_validate(dispatch_object_t object);
+
+#define dispatch_retain(object) \
+  __extension__({ dispatch_object_t _o = (object); \
+                  _dispatch_object_validate(_o); \
+                  (void)[_o retain]; })
+#define dispatch_release(object) \
+  __extension__({ dispatch_object_t _o = (object); \
+                  _dispatch_object_validate(_o); \
+                  [_o release]; })
+
+
 @interface SomeClass
 @end
 
 @implementation SomeClass
 - (NSDictionary *)copyTestWithBridgeReturningRetainable:(NSData *)plistData {
   CFErrorRef error;
   CFDictionaryRef testDict = CFPropertyListCreateWithData(kCFAllocatorDefault, (__bridge CFDataRef)plistData, 0, 0, &error);
 #if HAS_ARC

 
 - (CFDictionaryRef)copyTestReturningCoreFoundation:(NSData *)plistData {
   CFErrorRef error;
   CFDictionaryRef testDict = CFPropertyListCreateWithData(kCFAllocatorDefault, (__bridge CFDataRef)plistData, 0, 0, &error);
   return testDict;
 }
 @end
 
+int buf[1024];
+
+void libdispatch_leaked_data() {
+  dispatch_data_t data = dispatch_data_create(buf, 1024,
+                                              dispatch_get_main_queue(), ^{});
+}
+#if !HAS_ARC
+  // expected-warning@-2{{Potential leak of an object stored into 'data'}}
+  // expected-note@-5{{Call to function 'dispatch_data_create' returns an Objective-C object with a +1 retain count}}
+  // expected-note@-4{{Object leaked: object allocated and stored into 'data' is not referenced later in this execution path and has a retain count of +1}}
+#endif
+
+void libdispatch_dispatch_released_data() {
+  dispatch_data_t data = dispatch_data_create(buf, 1024,
+                                              dispatch_get_main_queue(), ^{});
+#if !HAS_ARC
+  dispatch_release(data); // no-warning
+#endif
+}
+
+void libdispatch_objc_released_data() {
+  dispatch_data_t data = dispatch_data_create(buf, 1024,
+                                              dispatch_get_main_queue(), ^{});
+#if !HAS_ARC
+  [data release]; // no-warning
+#endif
+}
+
+void libdispatch_leaked_retained_data() {
+  dispatch_data_t data = dispatch_data_create(buf, 1024,
+                                              dispatch_get_main_queue(), ^{});
+#if !HAS_ARC
+  dispatch_retain(data);
+  [data release];
+#endif
+}
+#if !HAS_ARC
+// expected-warning@-2{{Potential leak of an object stored into 'data'}}
+// expected-note@-9{{Call to function 'dispatch_data_create' returns an Objective-C object with a +1 retain count}}
+// expected-note@-7{{Reference count incremented. The object now has a +2 retain count}}
+// expected-note@-7{{Reference count decremented. The object now has a +1 retain count}}
+// expected-note@-6{{Object leaked: object allocated and stored into 'data' is not referenced later in this execution path and has a retain count of +1}}
+#endif