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

[Anton Yartsev]

>
> 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.

> +  const FunctionDecl *getCalleeDecl() const { return CalleeDecl; }
> Do we only store the name of the allocator declaration here? 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.
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?

>
> +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

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