[PATCH][Review request] unix.Malloc checker improvement: +handling new/delete, +memory.MismatchedFree, +memory.MismatchedDelete, +improved display names for allocators/deallocators
Anna Zaks
ganna at apple.com
Fri Mar 22 10:22:41 PDT 2013
On Mar 21, 2013, at 8:01 PM, Anton Yartsev <anton.yartsev at gmail.com> wrote:
> 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?
Anton,
Jordan has noticed that you have Windows line endings ('\r') in the new file. Please, make sure you do not commit these.
Otherwise, we are OK with post commit review.
I am curious if you were able to find any real bugs with the new checker.
Thanks!
Anna.
>
>>
>> Thanks,
>> Anna.
>>
>>
>> On Mar 14, 2013, at 8:38 AM, Anton Yartsev <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 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.
>>
>> 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> 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
> <cplusplus.NewDelete_v2.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130322/d2682d6d/attachment.html>
More information about the cfe-commits
mailing list