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

[Anna Zaks]

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)

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.

On Mar 8, 2013, at 10:40 AM, Anton Yartsev <anton.yartsev at gmail.com> wrote:

> On 08.03.2013 5:09, Jordan Rose wrote:
>> 
>> On Mar 7, 2013, at 15:53 , Anton Yartsev <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> 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.
> 

checkPointerEscape is not intended for reporting warnings - thus, the absence of the parameters which would allow you to do that. It is only intended to notify the checker about pointers escaping. The checkers would usually decide to stop tracking such pointers while processing the callback.

In the MallocChecker, the doesNotFreeMemory helps us to decide if we should keep tracking the escaping symbols. For example, sometimes, we know that a call will not free memory even if a pointer escapes. In other cases, we decide that even if the call could free memory, we are going to model that explicitly later on (the malloc functions). Thanks for renaming the function to better reflect what it's doing!

> Should we finish with the main patch first or start digging this, how do you think?
> 

Would be great to finish this one first. I think we are very close.

> Waiting for your feedback.
> -- 
> Anton
> <MallocCheckerRefactoring_v2.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130308/50d59963/attachment.html>