[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators
Anton Yartsev
anton.yartsev at gmail.com
Thu Mar 21 20:01:47 PDT 2013
On 14.03.2013 21:42, Anna Zaks wrote:
> One more concern. I do not see any tests with path notes. It's very
> important to test not only the existence of the warning, but also the
> notes along the path.
> - malloc-path.c currently tests the path notes produced by the
> warnings from the MallocChecker
> - test/Analysis/diagnostics/deref-track-symbolic-region.c is a good
> example of how to test for the notes in text and plist formats(used to
> draw the path in Xcode).
>
> I'd add a new test file and test the diagnostics in both text and
> plist format. I am OK with this test and any related work going in as
> a separate commit.
New test added, MallocChecker::MallocBugVisitor::isAllocated() and
MallocChecker::MallocBugVisitor::isReleased() changed a little.
Nothing else changed since the last patch.
OK to commit?
>
> Thanks,
> Anna.
>
>
> On Mar 14, 2013, at 8:38 AM, Anton Yartsev <anton.yartsev at gmail.com
> <mailto:anton.yartsev at gmail.com>> wrote:
>
>> 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.
>>
>
> Let's do this ASAP. It might point out issues we have not thought of yet.
>
>>>
>>> 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 ofMallocMemAux()
>> breaks the binding and information about the initialization value is
>> lost. This causes several tests to fail.
>> Changed the comment to be more informative.
>
> I see. I am concerned about the inconsistency of processing malloc and
> new. On the other hand, it probably does not matter right now.
>
>>
>>>
>>> ---
>>> 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?
>>
>
> Addressing after the initial patch is fine.
>
>>>
>>> ---
>>> + 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.
>
> My understanding is that this API assumes that the memory belongs to
> the malloc family. So, for example, we would warn on freeing with a
> regular "free" after freeing with "initWithCharactersNoCopy..
> freeWhenDone".
>
> If the family is unknown, we should not be tracking the memory at all.
Great idea, I'll include corresponding changes in the next patch devoted
to unix.MismatchedDeallocator
>
>>
>> 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
>> <cplusplus.NewDelete_v1.patch>
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130322/fc019076/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cplusplus.NewDelete_v2.patch
Type: text/x-diff
Size: 37212 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130322/fc019076/attachment.patch>
More information about the cfe-commits
mailing list