Author: dergachev Date: Fri Oct 13 12:10:42 2017 New Revision: 315736 URL: http://llvm.org/viewvc/llvm-project?rev=315736&view=rev Log: [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 Added: cfe/trunk/test/Analysis/retain-release-safe.c Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp?rev=315736&r1=315735&r2=315736&view=diff ============================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp Fri Oct 13 12:10:42 2017 @@ -1170,6 +1170,11 @@ RetainSummaryManager::getFunctionSummary 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 Added: cfe/trunk/test/Analysis/retain-release-safe.c URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/retain-release-safe.c?rev=315736&view=auto ============================================================================== --- cfe/trunk/test/Analysis/retain-release-safe.c (added) +++ cfe/trunk/test/Analysis/retain-release-safe.c Fri Oct 13 12:10:42 2017 @@ -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. +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
