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

[Anton Yartsev]

On 28.02.2013 21:11, Jordan Rose wrote:
>
> On Feb 28, 2013, at 4:26 , Anton Yartsev <anton.yartsev at gmail.com 
> <mailto:anton.yartsev at gmail.com>> wrote:
>>>> +void testDeleteOp1() {
>>>> +  int *p = (int *)malloc(sizeof(int));
>>>> +  operator delete(p); // FIXME: should complain "Argument to 
>>>> operator delete() was allocated by malloc(), not operator new"
>>>> +}
>>> Hm. Any idea why this is not working? Is it not showing up as a 
>>> standard operator delete?
>> Nasty error.
>> There appears to be no RefState attached to a symbolic region for 'p' 
>> when operator delete(p) is processed by FreeMemAux(). Everything 
>> works fine if the call to operator delete is replaced by a delete 
>> expression or free().
>> Debugging of the following example
>> void test() {
>>  int *p = (int *)malloc(sizeof(int));
>>  operator delete(p); // case 1
>>  free(p); // case2
>> }
>> showed that in both cases all the addresses of SVals and regions 
>> remain the same from one call to FreeMemAux() to another but in the 
>> first case "const RefState *RsBase = 
>> State->get<RegionState>(SymBase);" returns 0.
>> The same thing happens for Objective-C methods.
>> The problem seem to lie somewhere in the guts of the analyzer.
>
> Aha. Haven't read the new patch yet, but I think I figured this one 
> out. We're doing the free checks in a /post-CallExpr/ check, but by 
> that point we've already evaluated the function, which causes the 
> pointers to escape, which causes checkPointerEscape to clear out the 
> RefState for these objects. Note this part in doesNotFreeMemory():
>
> // If it's one of the allocation functions we can reason about, we model
> // its behavior explicitly.
> if (isMemFunction(FD, ASTC))
> returntrue;
>
> You should just be able to add isStandardNewDelete to isMemFunction. 
> As for Objective-C methods, the same thing applies: if they're one of 
> the methods we handle in checkPostObjCMessage(), we can actually 
> return true from doesNotFreeMemory(), but we need to keep the existing 
> escaping behavior for methods we don't handle.
Right. Fixed it.
I guess that all methods with the first selector ending with NoCopy and 
'freeWhenDone' param set to 1 should be handled by 
checkPostObjCMessage(). Am I right?
Also propose to fix confusing name of doesNotFreeMemory() (that also 
return true for calls that does free memory, but interesting to us) and 
incorrect logic of checking Objective-C messages in 
checkPostObjCMessage() that conflicts with the logic in 
checkPostObjCMessage().
All this solutions are included in the attached patch.

-- 
Anton

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