From 6ac84e99d98cd9d1cd9681f954f005e60f5a6193 Mon Sep 17 00:00:00 2001
From: Artem Dergachev <artem.dergachev@gmail.com>
Date: Fri, 13 Oct 2017 19:10:42 +0000
Subject: [PATCH] [analyzer] RetainCount: Ignore annotations on user-made
 CFRetain wrappers.

It is not uncommon for the users to make their own wrappers around
CoreFoundation's CFRetain and CFRelease functions that are defensive
against null references. In such cases CFRetain is often incorrectly
marked as CF_RETURNS_RETAINED. Ignore said annotation and treat such
wrappers similarly to the regular CFRetain.

rdar://problem/31699502
Differential Revision: https://reviews.llvm.org/D38877


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@315736 91177308-0d34-0410-b5e6-96231b3b80d8
---
 .../Checkers/RetainCountChecker.cpp           |  5 ++
 test/Analysis/retain-release-safe.c           | 72 +++++++++++++++++++
 2 files changed, 77 insertions(+)
 create mode 100644 test/Analysis/retain-release-safe.c

diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
index 09a64899df..4db83af17f 100644
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -1170,6 +1170,11 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) {
       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
diff --git a/test/Analysis/retain-release-safe.c b/test/Analysis/retain-release-safe.c
new file mode 100644
index 0000000000..de74355242
--- /dev/null
+++ b/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.
+}
-- 
2.40.0