r176744 - [analyzer] Be more consistent about Objective-C methods that free memory.
Anton Yartsev
anton.yartsev at gmail.com
Sun Mar 10 10:12:05 PDT 2013
system-header-simulator-objc.h is included twice - from
system-header-simulator-for-malloc.h and malloc.mm test itself, may one
of includes is unnecessary?
> Author: jrose
> Date: Fri Mar 8 18:59:10 2013
> New Revision: 176744
>
> URL: http://llvm.org/viewvc/llvm-project?rev=176744&view=rev
> Log:
> [analyzer] Be more consistent about Objective-C methods that free memory.
>
> Previously, MallocChecker's pointer escape check and its post-call state
> update for Objective-C method calls had a fair amount duplicated logic
> and not-entirely-consistent checks. This commit restructures all this to
> be more consistent and possibly allow us to be more aggressive in warning
> about double-frees.
>
> New policy (applies to system header methods only):
> (1) If this is a method we know about, model it as taking/holding ownership
> of the passed-in buffer.
> (1a) ...unless there's a "freeWhenDone:" parameter with a zero (NO) value.
> (2) If there's a "freeWhenDone:" parameter (but it's not a method we know
> about), treat the buffer as escaping if the value is non-zero (YES) and
> non-escaping if it's zero (NO).
> (3) If the first selector piece ends with "NoCopy" (but it's not a method we
> know about and there's no "freeWhenDone:" parameter), treat the buffer
> as escaping.
>
> The reason that (2) and (3) don't explicitly model the ownership transfer is
> because we can't be sure that they will actually free the memory using free(),
> and we wouldn't want to emit a spurious "mismatched allocator" warning
> (coming in Anton's upcoming patch). In the future, we may have an idea of a
> "generic deallocation", i.e. we assume that the deallocator is correct but
> still continue tracking the region so that we can warn about double-frees.
>
> Patch by Anton Yartsev, with modifications from me.
>
> Added:
> cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-malloc.h
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> cfe/trunk/test/Analysis/malloc.mm
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=176744&r1=176743&r2=176744&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Fri Mar 8 18:59:10 2013
> @@ -168,12 +168,13 @@ public:
> private:
> void initIdentifierInfo(ASTContext &C) const;
>
> + ///@{
> /// Check if this is one of the functions which can allocate/reallocate memory
> /// pointed to by one of its arguments.
> bool isMemFunction(const FunctionDecl *FD, ASTContext &C) const;
> bool isFreeFunction(const FunctionDecl *FD, ASTContext &C) const;
> bool isAllocationFunction(const FunctionDecl *FD, ASTContext &C) const;
> -
> + ///@}
> static ProgramStateRef MallocMemReturnsAttr(CheckerContext &C,
> const CallExpr *CE,
> const OwnershipAttr* Att);
> @@ -218,10 +219,13 @@ private:
> bool checkUseAfterFree(SymbolRef Sym, CheckerContext &C,
> const Stmt *S = 0) const;
>
> - /// Check if the function is not known to us. So, for example, we could
> - /// conservatively assume it can free/reallocate it's pointer arguments.
> - bool doesNotFreeMemory(const CallEvent *Call,
> - ProgramStateRef State) const;
> + /// Check if the function is known not to free memory, or if it is
> + /// "interesting" and should be modeled explicitly.
> + ///
> + /// We assume that pointers do not escape through calls to system functions
> + /// not handled by this checker.
> + bool doesNotFreeMemOrInteresting(const CallEvent *Call,
> + ProgramStateRef State) const;
>
> static bool SummarizeValue(raw_ostream &os, SVal V);
> static bool SummarizeRegion(raw_ostream &os, const MemRegion *MR);
> @@ -495,14 +499,30 @@ void MallocChecker::checkPostStmt(const
> C.addTransition(State);
> }
>
> -static bool isFreeWhenDoneSetToZero(const ObjCMethodCall &Call) {
> +static bool isKnownDeallocObjCMethodName(const ObjCMethodCall &Call) {
> + // If the first selector piece is one of the names below, assume that the
> + // object takes ownership of the memory, promising to eventually deallocate it
> + // with free().
> + // Ex: [NSData dataWithBytesNoCopy:bytes length:10];
> + // (...unless a 'freeWhenDone' parameter is false, but that's checked later.)
> + StringRef FirstSlot = Call.getSelector().getNameForSlot(0);
> + if (FirstSlot == "dataWithBytesNoCopy" ||
> + FirstSlot == "initWithBytesNoCopy" ||
> + FirstSlot == "initWithCharactersNoCopy")
> + return true;
> +
> + return false;
> +}
> +
> +static Optional<bool> getFreeWhenDoneArg(const ObjCMethodCall &Call) {
> Selector S = Call.getSelector();
> +
> + // FIXME: We should not rely on fully-constrained symbols being folded.
> for (unsigned i = 1; i < S.getNumArgs(); ++i)
> if (S.getNameForSlot(i).equals("freeWhenDone"))
> - if (Call.getArgSVal(i).isConstant(0))
> - return true;
> + return !Call.getArgSVal(i).isZeroConstant();
>
> - return false;
> + return None;
> }
>
> void MallocChecker::checkPostObjCMessage(const ObjCMethodCall &Call,
> @@ -510,25 +530,20 @@ void MallocChecker::checkPostObjCMessage
> if (C.wasInlined)
> return;
>
> - // If the first selector is dataWithBytesNoCopy, assume that the memory will
> - // be released with 'free' by the new object.
> - // Ex: [NSData dataWithBytesNoCopy:bytes length:10];
> - // Unless 'freeWhenDone' param set to 0.
> - // TODO: Check that the memory was allocated with malloc.
> - bool ReleasedAllocatedMemory = false;
> - Selector S = Call.getSelector();
> - if ((S.getNameForSlot(0) == "dataWithBytesNoCopy" ||
> - S.getNameForSlot(0) == "initWithBytesNoCopy" ||
> - S.getNameForSlot(0) == "initWithCharactersNoCopy") &&
> - !isFreeWhenDoneSetToZero(Call)){
> - unsigned int argIdx = 0;
> - ProgramStateRef State = FreeMemAux(C, Call.getArgExpr(argIdx),
> - Call.getOriginExpr(), C.getState(), true,
> - ReleasedAllocatedMemory,
> - /* RetNullOnFailure*/ true);
> + if (!isKnownDeallocObjCMethodName(Call))
> + return;
>
> - C.addTransition(State);
> - }
> + if (Optional<bool> FreeWhenDone = getFreeWhenDoneArg(Call))
> + if (!*FreeWhenDone)
> + return;
> +
> + bool ReleasedAllocatedMemory;
> + ProgramStateRef State = FreeMemAux(C, Call.getArgExpr(0),
> + Call.getOriginExpr(), C.getState(),
> + /*Hold=*/true, ReleasedAllocatedMemory,
> + /*RetNullOnFailure=*/true);
> +
> + C.addTransition(State);
> }
>
> ProgramStateRef MallocChecker::MallocMemReturnsAttr(CheckerContext &C,
> @@ -1356,12 +1371,8 @@ ProgramStateRef MallocChecker::evalAssum
> return state;
> }
>
> -// Check if the function is known to us. So, for example, we could
> -// conservatively assume it can free/reallocate its pointer arguments.
> -// (We assume that the pointers cannot escape through calls to system
> -// functions not handled by this checker.)
> -bool MallocChecker::doesNotFreeMemory(const CallEvent *Call,
> - ProgramStateRef State) const {
> +bool MallocChecker::doesNotFreeMemOrInteresting(const CallEvent *Call,
> + ProgramStateRef State) const {
> assert(Call);
>
> // For now, assume that any C++ call can free memory.
> @@ -1378,24 +1389,23 @@ bool MallocChecker::doesNotFreeMemory(co
> if (!Call->isInSystemHeader() || Call->hasNonZeroCallbackArg())
> return false;
>
> - Selector S = Msg->getSelector();
> -
> - // Whitelist the ObjC methods which do free memory.
> - // - Anything containing 'freeWhenDone' param set to 1.
> - // Ex: dataWithBytesNoCopy:length:freeWhenDone.
> - for (unsigned i = 1; i < S.getNumArgs(); ++i) {
> - if (S.getNameForSlot(i).equals("freeWhenDone")) {
> - if (Call->getArgSVal(i).isConstant(1))
> - return false;
> - else
> - return true;
> - }
> - }
> + // If it's a method we know about, handle it explicitly post-call.
> + // This should happen before the "freeWhenDone" check below.
> + if (isKnownDeallocObjCMethodName(*Msg))
> + return true;
>
> - // If the first selector ends with NoCopy, assume that the ownership is
> - // transferred as well.
> - // Ex: [NSData dataWithBytesNoCopy:bytes length:10];
> - StringRef FirstSlot = S.getNameForSlot(0);
> + // If there's a "freeWhenDone" parameter, but the method isn't one we know
> + // about, we can't be sure that the object will use free() to deallocate the
> + // memory, so we can't model it explicitly. The best we can do is use it to
> + // decide whether the pointer escapes.
> + if (Optional<bool> FreeWhenDone = getFreeWhenDoneArg(*Msg))
> + return !*FreeWhenDone;
> +
> + // If the first selector piece ends with "NoCopy", and there is no
> + // "freeWhenDone" parameter set to zero, we know ownership is being
> + // transferred. Again, though, we can't be sure that the object will use
> + // free() to deallocate the memory, so we can't model it explicitly.
> + StringRef FirstSlot = Msg->getSelector().getNameForSlot(0);
> if (FirstSlot.endswith("NoCopy"))
> return false;
>
> @@ -1504,11 +1514,11 @@ ProgramStateRef MallocChecker::checkPoin
> const InvalidatedSymbols &Escaped,
> const CallEvent *Call,
> PointerEscapeKind Kind) const {
> - // If we know that the call does not free memory, keep tracking the top
> - // level arguments.
> + // If we know that the call does not free memory, or we want to process the
> + // call later, keep tracking the top level arguments.
> if ((Kind == PSK_DirectEscapeOnCall ||
> Kind == PSK_IndirectEscapeOnCall) &&
> - doesNotFreeMemory(Call, State)) {
> + doesNotFreeMemOrInteresting(Call, State)) {
> return State;
> }
>
>
> Added: cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-malloc.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-malloc.h?rev=176744&view=auto
> ==============================================================================
> --- cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-malloc.h (added)
> +++ cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-malloc.h Fri Mar 8 18:59:10 2013
> @@ -0,0 +1,34 @@
> +// Like the compiler, the static analyzer treats some functions differently if
> +// they come from a system header -- for example, it is assumed that system
> +// functions do not arbitrarily free() their parameters, and that some bugs
> +// found in system headers cannot be fixed by the user and should be
> +// suppressed.
> +#pragma clang system_header
> +
> +typedef __typeof(sizeof(int)) size_t;
> +void *malloc(size_t);
> +void *calloc(size_t, size_t);
> +void free(void *);
> +
> +
> +#if __OBJC__
> +
> +#import "system-header-simulator-objc.h"
> +
> + at interface Wrapper : NSData
> +- (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)len;
> + at end
> +
> + at implementation Wrapper
> +- (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)len {
> + return [self initWithBytesNoCopy:bytes length:len freeWhenDone:1]; // no-warning
> +}
> + at end
> +
> + at interface CustomData : NSData
> ++ (id)somethingNoCopy:(char *)bytes;
> ++ (id)somethingNoCopy:(void *)bytes length:(NSUInteger)length freeWhenDone:(BOOL)freeBuffer;
> ++ (id)something:(char *)bytes freeWhenDone:(BOOL)freeBuffer;
> + at end
> +
> +#endif
>
> Modified: cfe/trunk/test/Analysis/malloc.mm
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/malloc.mm?rev=176744&r1=176743&r2=176744&view=diff
> ==============================================================================
> --- cfe/trunk/test/Analysis/malloc.mm (original)
> +++ cfe/trunk/test/Analysis/malloc.mm Fri Mar 8 18:59:10 2013
> @@ -1,19 +1,6 @@
> // RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc -analyzer-store=region -verify -fblocks %s
> -#include "Inputs/system-header-simulator-objc.h"
> -
> -typedef __typeof(sizeof(int)) size_t;
> -void *malloc(size_t);
> -void free(void *);
> -
> - at interface Wrapper : NSData
> -- (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)len;
> - at end
> -
> - at implementation Wrapper
> -- (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)len {
> - return [self initWithBytesNoCopy:bytes length:len freeWhenDone:1]; // no-warning
> -}
> - at end
> +#import "Inputs/system-header-simulator-objc.h"
> +#import "Inputs/system-header-simulator-for-malloc.h"
>
> // Done with headers. Start testing.
> void testNSDatafFreeWhenDoneNoError(NSUInteger dataLength) {
> @@ -79,6 +66,11 @@ void testNSStringFreeWhenDoneNO2(NSUInte
> NSString *nsstr = [[NSString alloc] initWithCharactersNoCopy:data length:dataLength freeWhenDone:0]; // expected-warning{{leak}}
> }
>
> +void testOffsetFree() {
> + int *p = (int *)malloc(sizeof(int));
> + NSData *nsdata = [NSData dataWithBytesNoCopy:++p length:sizeof(int) freeWhenDone:1]; // expected-warning{{Argument to free() is offset by 4 bytes from the start of memory allocated by malloc()}}
> +}
> +
> void testRelinquished1() {
> void *data = malloc(42);
> NSData *nsdata = [NSData dataWithBytesNoCopy:data length:42 freeWhenDone:1];
> @@ -92,6 +84,31 @@ void testRelinquished2() {
> [NSData dataWithBytesNoCopy:data length:42]; // expected-warning {{Attempt to free released memory}}
> }
>
> +void testNoCopy() {
> + char *p = (char *)calloc(sizeof(int), 1);
> + CustomData *w = [CustomData somethingNoCopy:p]; // no-warning
> +}
> +
> +void testFreeWhenDone() {
> + char *p = (char *)calloc(sizeof(int), 1);
> + CustomData *w = [CustomData something:p freeWhenDone:1]; // no-warning
> +}
> +
> +void testFreeWhenDonePositive() {
> + char *p = (char *)calloc(sizeof(int), 1);
> + CustomData *w = [CustomData something:p freeWhenDone:0]; // expected-warning{{leak}}
> +}
> +
> +void testFreeWhenDoneNoCopy() {
> + int *p = (int *)malloc(sizeof(int));
> + CustomData *w = [CustomData somethingNoCopy:p length:sizeof(int) freeWhenDone:1]; // no-warning
> +}
> +
> +void testFreeWhenDoneNoCopyPositive() {
> + int *p = (int *)malloc(sizeof(int));
> + CustomData *w = [CustomData somethingNoCopy:p length:sizeof(int) freeWhenDone:0]; // expected-warning{{leak}}
> +}
> +
> // Test CF/NS...NoCopy. PR12100: Pointers can escape when custom deallocators are provided.
> void testNSDatafFreeWhenDone(NSUInteger dataLength) {
> CFStringRef str;
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
Anton
More information about the cfe-commits
mailing list