[PATCH] D38877: [analyzer] RetainCount: Accept "safe" CFRetain wrappers.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
Herald added subscribers: szepet, xazax.hun.

CoreFoundation users often look for a "safe" wrapper around `CFRetain` and `CFRelease` that would gracefully accept (and ignore) a null CF reference. Users are trying to annotate it with `CF_RETURNS_RETAINED`, which misleads the analyzer into believing that the function kind of "creates" the object with a +1 retain count (such annotation is normally used on functions that create or copy objects), while in fact it increments the retain count on an existing object.

When modeling such function, RetainCountChecker first realizes that this function is merely a `CFRetain` (due to naming convention) and increments the reference count, and then sees the annotation and throws away the existing retain count information and resets the reference count to +1. This causes obvious problems, for example if the object is safely-retained and released more than once, it'd cause various kinds of overrelease warnings.

Tell the analyzer not to look at annotations for functions that he already identified as some form of `CFRetain`.


https://reviews.llvm.org/D38877

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
  test/Analysis/retain-release-safe.c


Index: test/Analysis/retain-release-safe.c
===================================================================
--- /dev/null
+++ test/Analysis/retain-release-safe.c
@@ -0,0 +1,72 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.coreFoundation.CFRetainRelease,osx.cocoa.RetainCount -verify %s
+
+#pragma clang arc_cf_code_audited begin
+typedef const void * CFTypeRef;
+extern CFTypeRef CFRetain(CFTypeRef cf);
+extern void CFRelease(CFTypeRef cf);
+#pragma clang arc_cf_code_audited end
+
+#define CF_RETURNS_RETAINED __attribute__((cf_returns_retained))
+#define CF_CONSUMED __attribute__((cf_consumed))
+
+extern CFTypeRef CFCreate() CF_RETURNS_RETAINED;
+
+// A "safe" variant of CFRetain that doesn't crash when a null pointer is
+// retained. This is often defined by users in a similar manner. The
+// CF_RETURNS_RETAINED annotation is misleading here, because the function
+// is not supposed to return an object with a +1 retain count. Instead, it
+// is supposed to return an object with +(N+1) retain count, where N is
+// the original retain count of 'cf'. However, there is no good annotation
+// to use in this case, and it is pointless to provide such annotation
+// because the only use cases would be CFRetain and SafeCFRetain.
+// So instead we teach the analyzer to be able to accept such code
+// and ignore the misplaced annotation.
+CFTypeRef SafeCFRetain(CFTypeRef cf) CF_RETURNS_RETAINED {
+  if (cf) {
+    return CFRetain(cf);
+  }
+  return cf;
+}
+
+// A "safe" variant of CFRelease that doesn't crash when a null pointer is
+// released. The CF_CONSUMED annotation seems reasonable here.
+void SafeCFRelease(CFTypeRef CF_CONSUMED cf) {
+  if (cf)
+    CFRelease(cf); // no-warning (when inlined)
+}
+
+void escape(CFTypeRef cf);
+
+void makeSureTestsWork() {
+  CFTypeRef cf = CFCreate();
+  CFRelease(cf);
+  CFRelease(cf); // expected-warning{{Reference-counted object is used after it is released}}
+}
+
+// Make sure we understand that the second SafeCFRetain doesn't return an
+// object with +1 retain count, which we won't be able to release twice.
+void falseOverrelease(CFTypeRef cf) {
+  SafeCFRetain(cf);
+  SafeCFRetain(cf);
+  SafeCFRelease(cf);
+  SafeCFRelease(cf); // no-warning after inlining this.
+}
+
+// Regular CFRelease() should behave similarly.
+void sameWithNormalRelease(CFTypeRef cf) {
+  SafeCFRetain(cf);
+  SafeCFRetain(cf);
+  CFRelease(cf);
+  CFRelease(cf); // no-warning
+}
+
+// Make sure we understand that the second SafeCFRetain doesn't return an
+// object with +1 retain count, which would no longer be owned by us after
+// it escapes to escape() and released once.
+void falseReleaseNotOwned(CFTypeRef cf) {
+  SafeCFRetain(cf);
+  SafeCFRetain(cf);
+  escape(cf);
+  SafeCFRelease(cf);
+  SafeCFRelease(cf); // no-warning after inlining this.
+}
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1170,6 +1170,11 @@
       if (cocoa::isRefType(RetTy, "CF", FName)) {
         if (isRetain(FD, FName)) {
           S = getUnarySummary(FT, cfretain);
+          // CFRetain isn't supposed to be annotated. However, this may as well
+          // be a user-made "safe" CFRetain function that is incorrectly
+          // annotated as cf_returns_retained due to lack of better options.
+          // We want to ignore such annotation.
+          AllowAnnotations = false;
         } else if (isAutorelease(FD, FName)) {
           S = getUnarySummary(FT, cfautorelease);
           // The headers use cf_consumed, but we can fully model CFAutorelease


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38877.118891.patch
Type: text/x-patch
Size: 3717 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171013/5972ced3/attachment.bin>