[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

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

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

5

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! :-)

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];'

test/Analysis/dispatch-data-leak.m

+// RUN: %clang_cc1 -w -triple x86_64-apple-macosx10.12.0 -fblocks -analyze -analyzer-checker=core,osx.cocoa,unix.Malloc -verify %s
+// RUN: %clang_cc1 -w -triple x86_64-apple-macosx10.12.0 -fblocks -fobjc-arc -analyze -analyzer-checker=core,osx.cocoa,unix.Malloc -verify %s
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/system-header-simulator-objc.h"
+
+#define NON_ARC !__has_feature(objc_arc)
+
+#define DISPATCH_QUEUE_SERIAL NULL
+
+#define DISPATCH_DATA_DESTRUCTOR_DEFAULT NULL
+#define DISPATCH_DATA_DESTRUCTOR_FREE (_dispatch_data_destructor_free)
+#define DISPATCH_DATA_DESTRUCTOR_MUNMAP (_dispatch_data_destructor_munmap)
+
+#define OS_OBJECT_RETURNS_RETAINED __attribute__((__ns_returns_retained__))
+#define DISPATCH_RETURNS_RETAINED OS_OBJECT_RETURNS_RETAINED
+
+@protocol OS_dispatch_data <NSObject>
+@end
+@protocol OS_dispatch_queue <NSObject>
+@end
+@protocol OS_dispatch_queue_attr <NSObject>
+@end
+
+typedef NSObject<OS_dispatch_data> *dispatch_data_t;
+typedef NSObject<OS_dispatch_queue> *dispatch_queue_t;
+typedef NSObject<OS_dispatch_queue_attr> *dispatch_queue_attr_t;
+
+typedef void (^dispatch_block_t)(void);
+
+dispatch_queue_t dispatch_get_main_queue(void);
+
+DISPATCH_RETURNS_RETAINED dispatch_queue_t
+dispatch_queue_create(const char *_Nullable label,
+                      dispatch_queue_attr_t _Nullable attr);
+
+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 clang_analyzer_printState();
+
+char buf[1024];
+void find_all_three_leaks() {
+  char *malloc_buf;
+  dispatch_data_t data;
+  dispatch_queue_t q;
+
+  malloc_buf = malloc(1024);
+  data = dispatch_data_create(buf, 1024, dispatch_get_main_queue(), ^{}); // expected-warning{{Potential leak of memory pointed to by 'malloc_buf'}}
+#if NON_ARC
+  // expected-warning@-2{{Potential leak of an object stored into 'data'}}
+#endif
+  q = dispatch_queue_create("hello", DISPATCH_QUEUE_SERIAL);
+#if NON_ARC
+  // expected-warning@-2{{Potential leak of an object stored into 'q'}}
+#endif
+}