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

[Anton Yartsev]

On 12.03.2013 2:24, Jordan Rose wrote:
> Looking over this one last time...
>
>> -  os << "Argument to free() is offset by "
>> +  os << "Argument to ";
>> +  if (!printAllocDeallocName(os, C, DeallocExpr))
>> +    os << "deallocator";
>> +  os << " is offset by "
>>       << offsetBytes
>>       << " "
>>       << ((abs(offsetBytes) > 1) ? "bytes" : "byte")
>> -     << " from the start of memory allocated by malloc()";
>> +     << " from the start of ";
>> +  if (AllocExpr) {
>> +    SmallString<100> TempBuf;
>> +    llvm::raw_svector_ostream TempOs(TempBuf);
>> +    if (printAllocDeallocName(TempOs, C, AllocExpr))
>> +        os << "memory allocated by " << TempOs.str();
>> +      else
>> +        os << "allocated memory";
>> +  } else
>> +    printExpectedAllocName(os, C, DeallocExpr);
>> +
>
> The way you've set it up, AllocExpr will never be NULL (which is good, 
> because otherwise you'd get "...from the start of malloc()" rather 
> than "from the start of memory allocated by malloc()").
Strange logic. Fixed.

>
>
>> + at interface Wrapper : NSData
>> +- (id)initWithBytesNoCopy:(void *)bytes length:(NSUInteger)len;
>> + at end
>
> As I discovered with the rest of the ObjC patch, this isn't a great 
> test case because the analyzer doesn't consider it a system method. 
> However, I don't see you use it anywhere in the file anyway, so you 
> can probably just remove it.
>
>
>> +void testNew11(NSUInteger dataLength) {
>> +  int *data = new int;
>> +  NSData *nsdata = [NSData dataWithBytesNoCopy:data 
>> length:sizeof(int) freeWhenDone:1]; // expected-warning{{Memory 
>> allocated by 'new' should be deallocated by 'delete', not 
>> +dataWithBytesNoCopy:length:freeWhenDone:}}
>> +}
>
> Hm, that is rather unwieldy, but what bothers me more is that 
> +dataWithBytesNoCopy:length:freeWhenDone: /doesn't/ free the memory; 
> it just takes ownership of it. I guess it's okay to leave that as a 
> FIXME for now, but in the long run we should say something like 
> "+dataWithBytesNoCopy:length:freeWhenDone: cannot take ownership of 
> memory allocated by 'new'." (In the "hold" cases, most likely the user 
> wasn't intending to free
>
> But, this doesn't have to block the patch; you/we can fix it post-commit.
>
>
>> +  delete x; // FIXME: Shoud detect pointer escape and keep silent. 
>> checkPointerEscape() is not currently invoked for delete.
>
> Pedantic note: the real issue here is that we don't model delete at 
> all (see ExprEngine::VisitCXXDeleteExpr). The correct model won't 
> explicitly invoke checkPointerEscape, but it will construct a 
> CallEvent for the deletion operator and then try to evaluate that 
> call—or at least invalidate the arguments like VisitCXXNewExpr does 
> for placement args—which will cause the argument region to get 
> invalidated and checkPointerEscape to be triggered.
>
> Jordan
Updated patch attached.

-- 
Anton

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