[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators

[Anton Yartsev]

On 08.03.2013 5:09, Jordan Rose wrote:
>
> On Mar 7, 2013, at 15:53 , Anton Yartsev <anton.yartsev at gmail.com 
> <mailto:anton.yartsev at gmail.com>> wrote:
>
>> On 07.03.2013 21:29, Jordan Rose wrote:
>>> On Mar 7, 2013, at 4:47 , Anton Yartsev <anton.yartsev at gmail.com 
>>> <mailto:anton.yartsev at gmail.com>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130308/dae41066/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: MallocCheckerRefactoring_v2.patch
Type: text/x-diff
Size: 8105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130308/dae41066/attachment.patch>