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

[Anton Yartsev]

Modified the patch to avoid bugs in case when a default zero index is 
returned by getAllocDeallocDataIdx()
>
> 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!)
>
>>> Jordan, what do you think about this?
>>>
>>>> +void MallocChecker::checkPostStmt(const CXXNewExpr *NE,
>>>> +  CheckerContext &C) const {NE
>>>> +
>>>> +  FunctionDecl *OperatorNew = NE->getOperatorNew();
>>>> +  if (!OperatorNew)
>>>> +    return;
>>>> +
>>>> +  // Skip custom new operators
>>>> +  if (!OperatorNew->isImplicit() &&
>>>> + 
>>>>  !C.getSourceManager().isInSystemHeader(OperatorNew->getLocStart()) &&
>>>> +      !NE->isGlobalNew())
>>>> +    return;
>>>> +
>>>> +  // Skip standard global placement operator 
>>>> new/new[](std::size_t, void * p);
>>>> +  // process all other standard new/new[] operators including 
>>>> placement
>>>> +  // operators new/new[](std::size_t, const std::nothrow_t&)
>>>> +  if (OperatorNew->isReservedGlobalPlacementOperator())
>>>> +    return;
>>>>
>>>> Is there a reason why we first process each operator new in 
>>>> "checkPostStmt(const callExpr" and finish processing in 
>>>> "checkPostStmt(const CXXNewExpr" ? I think the code would be 
>>>> simpler if everything could be done in a single callback.
>>> Code added to "checkPostStmt(const callExpr" is for processing 
>>> direct calls to operator new/delete functions, "checkPostStmt(const 
>>> CXXNewExpr" is for handling new expressions. Either first or second 
>>> callback is called in each particular case but not both of them. Am 
>>> I right?
>>>
>>
>> I see; makes sense. Please, add a comment in "checkPostStmt(const 
>> callExpr*".
>>
>>>>
>>>> +void MallocChecker::PrintExpectedAllocName(raw_ostream &os, 
>>>> CheckerContext &C,
>>>> + const Expr *E) const {
>>>> +  DeallocatorKind dKind = GetDeallocKind(C, E);
>>>> +
>>>> +  switch(dKind) {
>>>> +    case D_free: os << "malloc()"; return;
>>>> +    case D_delete: os << "operator new"; return;
>>>> +    case D_deleteArray: os << "operator new[]"; return;
>>>> +    case D_unknown: os << "unknown means"; return;
>>>>
>>>> This function is used to form user visible warnings. Do we ever 
>>>> expect it to print "unknown means"? Can this be based on the Kind 
>>>> stored inside of RefState, where there is no D_unknown?
>>> Right, changed the case to assert. There is actually implicit 
>>> D_unknown in RefState - case when 2nd and 3rd bits are set to zero.
>>>
>>> -- 
>>> Anton
>>> <MallocChecker_v3.patch>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>


-- 
Anton

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