On 08.03.2013 5:09, Jordan Rose wrote:
On Mar 7, 2013, at 15:53 , Anton Yartsev <[email protected]
<mailto:[email protected]>> wrote:
On 07.03.2013 21:29, Jordan Rose wrote:
On Mar 7, 2013, at 4:47 , Anton Yartsev <[email protected]
<mailto:[email protected]>> wrote:
Can we just do the Objective-C part first? Can you send that patch too?
Attached. I'll update the main patch after this one gets in.
Looking at this again, I think the old logic was right (or at least
more conservative). It says that any method with "freeWhenDone" in the
name can take ownership (if the param is true), and additionally that
any method with "NoCopy" and /no/ "freeWhenDone" will also take ownership.
Anna's right that this needs a test case too.
Jordan
Really, the old logic is more detailed. Restored the old logic, only
applied the fix.
> As such, the patch needs a test case. I think you should be able to
test for double free here.
First I tried to design a testcase giving different results for double
free before and after the fix, but failed - the overall logic of the
checker gives the same result. Managed to write sensitive test for
offset free.
There seems to be other two problems: first pointed to by the test case
testRelinquished2() (before the patch dataWithBytesNoCopy:length: had
been processed by checkPostObjCMessage and the test passed) and the
second - by testNoCopy() and testFreeWhenDone() test cases.
The first problem is that checkPointerEscape() does not report "Attempt
to free released/non-owned" (see the FIXME in checkPointerEscape())
The second one is that checkPointerEscape() removes RefState and does
not store any info that memory is already released, so freeing released
memory by free() is not detected later.
The first problem can be easily eliminated if the CheckerContext is
passed to checkPointerEscape(), the second one I have not meditated yet.
Should we finish with the main patch first or start digging this, how do
you think?
Waiting for your feedback.
--
Anton
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp (revision 176667)
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp (working copy)
@@ -168,12 +168,14 @@
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;
-
+ bool isObjCNoCopyFreeWhenDone(const ObjCMethodCall &Call) const;
+ ///@}
static ProgramStateRef MallocMemReturnsAttr(CheckerContext &C,
const CallExpr *CE,
const OwnershipAttr* Att);
@@ -220,8 +222,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 doesNotFreeMemOrKnown(const CallEvent *Call,
+ ProgramStateRef State) const;
static bool SummarizeValue(raw_ostream &os, SVal V);
static bool SummarizeRegion(raw_ostream &os, const MemRegion *MR);
@@ -439,6 +441,23 @@
return false;
}
+bool MallocChecker::isObjCNoCopyFreeWhenDone(const ObjCMethodCall &Call) const {
+ // If the first selector ends with NoCopy, 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).endswith("NoCopy"))
+ return false;
+
+ for (unsigned i = 1; i < S.getNumArgs(); ++i)
+ if (S.getNameForSlot(i).equals("freeWhenDone"))
+ return !Call.getArgSVal(i).isConstant(0);
+
+ return false;
+}
+
void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const {
if (C.wasInlined)
return;
@@ -495,40 +514,23 @@
C.addTransition(State);
}
-static bool isFreeWhenDoneSetToZero(const ObjCMethodCall &Call) {
- Selector S = Call.getSelector();
- for (unsigned i = 1; i < S.getNumArgs(); ++i)
- if (S.getNameForSlot(i).equals("freeWhenDone"))
- if (Call.getArgSVal(i).isConstant(0))
- return true;
-
- return false;
-}
-
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 (!isObjCNoCopyFreeWhenDone(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 +1362,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::doesNotFreeMemOrKnown(const CallEvent *Call,
+ ProgramStateRef State) const {
assert(Call);
// For now, assume that any C++ call can free memory.
@@ -1379,6 +1381,7 @@
return false;
Selector S = Msg->getSelector();
+ StringRef FirstSlot = S.getNameForSlot(0);
// Whitelist the ObjC methods which do free memory.
// - Anything containing 'freeWhenDone' param set to 1.
@@ -1386,7 +1389,8 @@
for (unsigned i = 1; i < S.getNumArgs(); ++i) {
if (S.getNameForSlot(i).equals("freeWhenDone")) {
if (Call->getArgSVal(i).isConstant(1))
- return false;
+ // 'NoCopy' + 'freeWhenDone==1' is the case we want to track
+ return FirstSlot.endswith("NoCopy");
else
return true;
}
@@ -1395,7 +1399,6 @@
// 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 (FirstSlot.endswith("NoCopy"))
return false;
@@ -1508,7 +1511,7 @@
// level arguments.
if ((Kind == PSK_DirectEscapeOnCall ||
Kind == PSK_IndirectEscapeOnCall) &&
- doesNotFreeMemory(Call, State)) {
+ doesNotFreeMemOrKnown(Call, State)) {
return State;
}
@@ -1520,6 +1523,9 @@
if (const RefState *RS = State->get<RegionState>(sym)) {
if (RS->isAllocated())
State = State->remove<RegionState>(sym);
+ else {
+ // FIXME: report Attempt to free released/non-owned
+ }
}
}
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)dataWithBytes:(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];
@@ -89,9 +98,21 @@
void *data = malloc(42);
NSData *nsdata;
free(data);
- [NSData dataWithBytesNoCopy:data length:42]; // expected-warning {{Attempt to free released memory}}
+ [NSData dataWithBytesNoCopy:data length:42]; // FIXME: attempt to free released memory
}
+void testNoCopy() {
+ int *p = (int *)malloc(sizeof(int));
+ NSData *nsdata = [NSData dataWithBytesNoCopy:p length:sizeof(int)];
+ free(p); // FIXME: attempt to free released memory
+}
+
+void testFreeWhenDone() {
+ int *p = (int *)malloc(sizeof(int));
+ I *w = [I dataWithBytes:p length:sizeof(int) freeWhenDone:1];
+ free(p); // FIXME: attempt to free released memory
+}
+
// 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
