On 09.03.2013 2:51, Anton Yartsev wrote:
On 08.03.2013 23:25, Jordan Rose wrote:
On Mar 8, 2013, at 11:22 , Anna Zaks <[email protected]
<mailto:[email protected]>> wrote:
Anton,
We've briefly discussed this with Jordan. Below are the cases we
should handle:
Case 1: The selector starts with "dataWithBytesNoCopy" or
"initWithBytesNoCopy" or "initWithCharactersNoCopy" and
"freeWhenDone" is not set to "0".
We should assume that the call preforms hold action from malloc
family. (So the pointer should not escape and we should model this
during ObjCMsgCall processing)
To explain the rationalization here, Anna pointed out that while it's
/likely/ that methods with a "freeWhenDone:" or "...NoCopy:" selector
piece all behave as ownership-holders, we can't actually prove it. In
particular, if/when MallocChecker gains the ability to reason about
custom allocators, someone could very well use "...NoCopy" to mean "I
will free this using my custom deallocator", and we don't want to
produce a bogus allocator mismatch bug in that case.
By the way, by "the selector starts with __", we mean the existing
logic of "the first selector piece is __", not "the first selector
piece starts with __".
Thanks for bearing with us.
Jordan
Aaaa, got it!
Updated the patch.
> Case 1: The selector starts with "dataWithBytesNoCopy" or
"initWithBytesNoCopy" or "initWithCharactersNoCopy" and "freeWhenDone"
is not set to "0".
> We should assume that the call preforms hold action from malloc
family. (So the pointer should not escape and we should model this
during ObjCMsgCall processing)
>
> Case 2: The selector contains "freeWhenDone", which is set to '0'.
> The call results in no escape of the pointer.
>
> Case 3: Otherwise (this is not Case 1 nor Case 3) and the selector
ends with "NoCopy"
> The pointer should escape.
>
> Please, work off your previous patch, but change the logic to handle
these cases and include the tests. Also, I think it's
> important to include a commit message with your patch so that there
would be no ambiguity in what the patch is trying to accomplish.
> Usually, the commit message and the tests would resolve that.
I'll put this cases in a commit message.
--
Anton
Or even with a better name for doesNotFreeMemory() and comment.
--
Anton
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp (revision 176667)
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp (working copy)
@@ -168,12 +168,13 @@
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);
@@ -220,8 +221,8 @@
/// 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;
+ 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,6 +496,20 @@
C.addTransition(State);
}
+static bool isKnownDeallocObjCMethodName(const ObjCMethodCall &Call) {
+ // If the first selector is one of the selectors above, 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.
+ Selector S = Call.getSelector();
+ if (S.getNameForSlot(0) == "dataWithBytesNoCopy" ||
+ S.getNameForSlot(0) == "initWithBytesNoCopy" ||
+ S.getNameForSlot(0) == "initWithCharactersNoCopy")
+ return true;
+
+ return false;
+}
+
static bool isFreeWhenDoneSetToZero(const ObjCMethodCall &Call) {
Selector S = Call.getSelector();
for (unsigned i = 1; i < S.getNumArgs(); ++i)
@@ -507,28 +522,22 @@
void MallocChecker::checkPostObjCMessage(const ObjCMethodCall &Call,
CheckerContext &C) const {
+
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) ||
+ isFreeWhenDoneSetToZero(Call))
+ return;
- C.addTransition(State);
- }
+ bool ReleasedAllocatedMemory;
+ unsigned int argIdx = 0;
+ ProgramStateRef State = FreeMemAux(C, Call.getArgExpr(argIdx),
+ Call.getOriginExpr(), C.getState(), true,
+ ReleasedAllocatedMemory,
+ /* RetNullOnFailure*/ true);
+
+ C.addTransition(State);
}
ProgramStateRef MallocChecker::MallocMemReturnsAttr(CheckerContext &C,
@@ -1360,8 +1369,8 @@
// 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.
@@ -1380,18 +1389,17 @@
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;
- }
- }
+ bool FreeWhenDoneSetToZero = isFreeWhenDoneSetToZero(*Msg);
+ // Process later
+ if (isKnownDeallocObjCMethodName(*Msg) && !FreeWhenDoneSetToZero)
+ return true;
+
+ // Does not free memory.
+ // Ex: dataWithBytesNoCopy:length:freeWhenDone.
+ if (FreeWhenDoneSetToZero)
+ return true;
+
// If the first selector ends with NoCopy, assume that the ownership is
// transferred as well.
// Ex: [NSData dataWithBytesNoCopy:bytes length:10];
@@ -1504,11 +1512,11 @@
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;
}
Index: test/Analysis/malloc.mm
===================================================================
--- test/Analysis/malloc.mm (revision 176627)
+++ test/Analysis/malloc.mm (working copy)
@@ -15,6 +15,10 @@
}
@end
+@interface I : NSData
++ (id)somethingNoCopy:(void *)bytes length:(NSUInteger)length freeWhenDone:(BOOL)freeBuffer;
+@end
+
// Done with headers. Start testing.
void testNSDatafFreeWhenDoneNoError(NSUInteger dataLength) {
unsigned char *data = (unsigned char *)malloc(42);
@@ -79,6 +83,11 @@
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 +101,11 @@
[NSData dataWithBytesNoCopy:data length:42]; // expected-warning {{Attempt to free released memory}}
}
+void testFreeWhenDone() {
+ int *p = (int *)malloc(sizeof(int));
+ I *w = [I somethingNoCopy:p length:sizeof(int) freeWhenDone:1]; // no-warning
+}
+
// Test CF/NS...NoCopy. PR12100: Pointers can escape when custom deallocators are provided.
void testNSDatafFreeWhenDone(NSUInteger dataLength) {
CFStringRef str;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
