[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
Mon Mar 25 08:21:38 PDT 2013
On 22.03.2013 21:22, Anna Zaks wrote:
>
> On Mar 21, 2013, at 8:01 PM, Anton Yartsev <anton.yartsev at gmail.com
> <mailto: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.
Committed as r177849
Manage to find several random real bugs (report-843813.html,
report-230257.html, report-444177.html, report-737879.html, recursive
case in report-727931.html), but for now it is hard to detect real bugs
among tons of false-positives.
There are two types of false-positives that form the majority of reports:
1) Illustrated by the following test (added similar test to
NewDelete-checker-test.mm):
int *global;
void testMemIsOnHeap() {
int *p = new int; // FIXME: currently not heap allocated!
if (global != p) // evaluates to UnknownVal() rather then 'true'
global = p;
} // report false-positive leak
As I understand the problem is that currently a memory region for 'new'
is not a heap region and this lead to false-positives like
report-863595.html and others. (e.g. that causes 'global != p' evaluate
to UnknownVal() rather then 'true' (logic of
SimpleSValBuilder::evalBinOpLL))
Attached is the proposed patch that fixes these issues.
2) The second type of false-positives is illustrated by the following
minimal test:
void f(const int & x);
void test() {
int *p = (int *)malloc(sizeof(int));
f(*p);
} // report false-positive leak
report-218274.html shows how it looks like in reality.
Haven't addressed this yet. Removing 'const' from the declaration
eliminates the leak report.
I had a cold and my performance decreased significantly so sorry for
delays and inattention :)
>
>>>
>>> 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
>> <cplusplus.NewDelete_v2.patch>
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130325/ff429b88/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reports.zip
Type: application/x-zip-compressed
Size: 148838 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130325/ff429b88/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: NewOnHeap.patch
Type: text/x-diff
Size: 1600 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130325/ff429b88/attachment.patch>
More information about the cfe-commits
mailing list