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

[Anton Yartsev]

Attached is the new version of the patch, several comments below.
>
> On Feb 22, 2013, at 7:24 , Anton Yartsev <anton.yartsev at gmail.com 
> <mailto:anton.yartsev at gmail.com>> wrote:
>
>> On 22.02.2013 9:30, Anna Zaks wrote:
>>>
>>> On Feb 21, 2013, at 6:26 PM, Anna Zaks <ganna at apple.com 
>>> <mailto:ganna at apple.com>> wrote:
>>>
>>>>
>>>> On Feb 21, 2013, at 6:00 PM, Anton Yartsev <anton.yartsev at gmail.com 
>>>> <mailto:anton.yartsev at gmail.com>> wrote:
>>>>
>>>>>>
>>>>>> On Feb 19, 2013, at 10:18 PM, Anton Yartsev 
>>>>>> <anton.yartsev at gmail.com <mailto:anton.yartsev at gmail.com>> wrote:
>>>>>>
>>>>>>> Hi, Jordan. Thanx for the review!
>>>>>>>
>>>>>>> Attached is the new version of the patch with all the comments 
>>>>>>> addressed. Also added support for directly called operator 
>>>>>>> new()/new[]() and operator delete()
>>>>>>>
>>>>>>> There is currently one problem with handling of operator 
>>>>>>> delete(). The following test
>>>>>>>
>>>>>>> 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"
>>>>>>> }
>>>>>>>
>>>>>>> produce no warnings as attached RefState seem to be missing at 
>>>>>>> the point when checkPostStmt(const CallExpr *CE, CheckerContext 
>>>>>>> &C) callback is called for operator delete(p).
>>>>>>> I haven't investigated the problem deeply yet, intend to address 
>>>>>>> it parallel with the review.
>>>>>>>
>>>>>>>> +  if (NE->getNumPlacementArgs())
>>>>>>>> +    return;
>>>>>>>> +  // skip placement new operators as they may not allocate memory
>>>>>>>>
>>>>>>>> Two comments here:
>>>>>>>> - Please make sure all comments are complete, capitalized, and 
>>>>>>>> punctuated sentences. (This has the important one, 
>>>>>>>> "complete"....just missing capitalization and punctuation.)
>>>>>>> I'll try. Unfortunately I am not as good in English as I want to 
>>>>>>> be, so sorry for my grammar, syntax, and punctuation.
>>>>>>>
>>>>>>> -- 
>>>>>>> Anton
>>>>>>>
>>>>>>> <MallocChecker_v2.patch>
>>>>>>
>>>>>
>>>>> Hi Anna. Thanks for your comments! Attached is the new patch.
>>>>>
>>>>>> Just adding another kind as extra enumeration values does not 
>>>>>> seem right. Another option is to make Kind be a pointer to a 
>>>>>> static array, which contains objects recording all necessary info 
>>>>>> about each kind (MacOSKeychainAPIChecker uses this approach). 
>>>>>> This is probably an overkill for now, but is another option.
>>>>> Not sure that I have got an idea.
>>>>> The memory and deallocator kind are both set for a RefState. Do 
>>>>> you mean creating a static array with 'memory kinds' * 
>>>>> 'deallocator kind' elements for all possible combinations? Also 
>>>>> there is no necessary info other then the kind itself.
>>>>> Left for now.
>>>>
>>>> I think of ToBeReleasedWith* as being different types of Allocate; 
>>>> I don't think they should be separate values in the same enum. It's 
>>>> also unfortunate to have to copy the constant values in both places 
>>>> - DeallocatorKind and RefState::Kind. Maybe you could restructure 
>>>> this similarly to how this is done in SVals.h?
>>>>>
>>>>>> +  const FunctionDecl *getCalleeDecl() const { return CalleeDecl; }
>>>>>> Do we only store the name of the allocator declaration here?
>>>>
>>>> If the Decl is always an allocator Decl, we should change the name 
>>>> of the method to be more descriptive.
>>>>>> Do we need to store this in the state? (Growing states implies 
>>>>>> memory overhead.) Can't this be easily implied from the kind?
>>>>> Kind can't give us information about the name of an allocator that 
>>>>> can be malloc(), realloc(), a user function with ownership_takes 
>>>>> attribute, etc.
>>>>> One solution to avoid keeping a CalleeDecl in RefState is to 
>>>>> rollback to CallExpr::getDirectCallee() from 
>>>>> CheckerContext::getCalleeDec() and to print "malloc()" in case of 
>>>>> indirect calls.
>>>>
>>>> Ok, I see.
>>>
>>> After thinking about it some more, I do not think we should add an 
>>> extra pointer to the state to differentiate between few allocator 
>>> functions. In the case when we do not have ownership attributes, we 
>>> only have few possible allocators, whose names we know ahead of 
>>> time. In case we support ownership attributes, we are likely to have 
>>> few allocator functions whose names we could just store in the 
>>> checker state on the first encounter (like we store the 
>>> IdentifierInfo).
>>>
>>> In addition, we could change the ownership attributes in such a way 
>>> that each allocator would have a corresponding deallocator; for 
>>> example, if we wanted to check matching allocators and deallocators. 
>>> Annotated deallocators won't necessarily be one of the functions you 
>>> know at compile time, so the DeallocatorKind enum would not cover 
>>> it. I think, it's best if we kept a table on a side that would store 
>>> this info (allocation function name, deallocator) and refer to the 
>>> entries in the table from the state. (Take a look at 
>>> MacOSKeychainAPIChecker - it's very similar to malloc, but it 
>>> handles different allocator/deallocator pairs. I think a similar 
>>> solution could work in this case as well. Other solutions that 
>>> address these issues are welcome as well!)
>> Attached is the patch that uses approach with a dynamic table that 
>> holds both allocator name and expected deallocator kind. This 
>> approach allows to keep any allocator names, either known or new 
>> ones. The table could be easily extended to hold additional data such 
>> as info about special deallocators, etc.
>
> Please do not do this. The analyzer is not thread-safe today, but 
> that's no reason why we should make it harder to make it thread-safe 
> tomorrow.
>
> Stepping back, there are many problems with this, the main one being 
> that just keeping track of the function name isn't really good enough. 
> MacOSKeychainAPIChecker can get away with it because its functions are
>
> I think all Anna meant is to have a /static/ table, like 
> MacOSKeychainAPIChecker. If/when we support allocator families defined 
> at runtime, we can do this properly, with a mutable table as a field 
> of the checker, but for now this is both overkill and error-prone.
>

>
> Capitalization, period.
>
Checked and fixed everything, sorry.


> This is /still/ the wrong check. You need to check if there are 
> /any/ placement arguments, and you need to check that the Decl is not 
> part of a class. Both are easy:
Changed logic to skip all placement operators except the standard 
placement nothrow versions that we know to allocate/deallocate memory.


>
> +  if (isa<ObjCMessageExpr>(E))
> +    return D_free;
>
> There are no Objective-C messages that free memory immediately; just 
> take this out.
>
Returning DK_free for Objective-C messages just mean that they belong to 
the 'malloc/free' family. Consider the following example:
void test() {
   int *p = (int *)malloc(sizeof(int));
   [NSData dataWithBytesNoCopy:bytes length:sizeof(int)];
}
Here we just check that dataWithBytesNoCopy:length: is from 
'malloc/free' family and keep silent.
This also implements '// TODO: Check that the memory was allocated with 
malloc.' from the checkPostObjCMessage().
Do you want to handle this situation in some other way?

-- 
Anton

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