[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 22:06, Anna Zaks wrote:
> Thanks Anton! The patch looks good overall. Have you evaluated it on 
> some real C++ codebases?
Not yet. Failed to launch scan-build with my Perl 5.16.2.

>
> Below are some comments.
>
> ---
> +  // The return value from operator new is already bound and we don't 
> want to
> +  // break this binding so we call MallocUpdateRefState instead of 
> MallocMemAux.
> +  State = MallocUpdateRefState(C, NE, State, NE->isArray() ? 
> AF_CXXNewArray
> +     : AF_CXXNew);
> Why is this different from handling malloc? MallocMemAux does break 
> the binding formed by default handling of malloc and forms a new 
> binding with more semantic information. (I am fine with addressing 
> this after the initial commit/commits.)
In case of 'new' the memory can be initialized to a non-default value 
(e.g. int *p = new int(3); ). The existing logic of MallocMemAux() 
breaks the binding and information about the initialization value is 
lost. This causes several tests to fail.
Changed the comment to be more informative.

>
> ---
>  def MallocOptimistic : Checker<"MallocWithAnnotations">,
> -  HelpText<"Check for memory leaks, double free, and use-after-free 
> problems. Assumes that all user-defined functions which might free a 
> pointer are annotated.">,
> +  HelpText<"Check for memory leaks, double free, and use-after-free 
> problems. Traces memory managed by malloc()/free(). Assumes that all 
> user-defined functions which might free a pointer are annotated.">,
>
> Shouldn't the MallocWithAnnotations only check the functions which are 
> explicitly annotated? I think it might be better to change the code 
> rather than the comment.
Currently MallocWithAnnotations is designed as extended unix.Malloc and 
it is not a single line change to make it distinct. Can we address this 
after primary commits?

>
> ---
> +  unsigned K : 2; // Kind enum, but stored as a bitfield.
> +  unsigned Family : 30; // Rest of 32-bit word, currently just an 
> allocation
> +                        // family.
>
> We usually add the comment on a line preceding the declaration, like this:
> +  // Kind enum, but stored as a bitfield.
> +  unsigned K : 2;
> +  // Rest of 32-bit word, currently just an allocation family.
> +  unsigned Family : 30;
>
> ---
> +  // Check if an expected deallocation function matches the real one.
> +  if (RsBase &&
> +      RsBase->getAllocationFamily() != AF_None &&
> +      RsBase->getAllocationFamily() != getAllocationFamily(C, 
> ParentExpr) ) {
> Is it possible to have AF_None family here? Shouldn't " 
> RsBase->getAllocationFamily() != AF_None" be inside an assert?
It is possible. In the example below 
initWithCharactersNoCopy:length:freeWhenDone takes ownership of memory 
allocated by unknown means, RefState with AF_None is bound to the call 
and after, when free() is processed, we have 
RsBase->getAllocationFamily() == AF_None.

void test(unichar *characters) {
   NSString *string = [[NSString alloc] 
initWithCharactersNoCopy:characters length:12 freeWhenDone:1];
   if (!string) {free(characters);}
}

>
> ---
> +// Used to check correspondence between allocators and deallocators.
> +enum AllocationFamily {
> The comment should describe what family is. It is a central notion for 
> the checker and I do not think we explain it anywhere.
>
> ---
> The patch is very long. Is it possible to split it up into smaller 
> chunks 
> (http://llvm.org/docs/DeveloperPolicy.html#incremental-development)?
Committed the initial refactoring as r176949.

Let's start! Attached is the cplusplus.NewDelete checker separated from 
the patch.

>
> Thanks,
> Anna.
> On Mar 12, 2013, at 8:56 AM, Anton Yartsev <anton.yartsev at gmail.com 
> <mailto:anton.yartsev at gmail.com>> wrote:
>
>> 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
>> <MallocChecker_v11.patch>
>
-- 
Anton

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130314/97338a10/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cplusplus.NewDelete_v1.patch
Type: text/x-diff
Size: 22018 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130314/97338a10/attachment.patch>