r176744 - [analyzer] Be more consistent about Objective-C methods that free memory.
Jordan Rose
jordan_rose at apple.com
Fri Mar 8 16:59:11 PST 2013
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;
More information about the cfe-commits
mailing list