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

[Anton Yartsev]

On 27.03.2013 5:23, Jordan Rose wrote:
>
> On Mar 26, 2013, at 17:10 , Anton Yartsev <anton.yartsev at gmail.com 
> <mailto:anton.yartsev at gmail.com>> wrote:
>
>> On 25.03.2013 21:39, Anna Zaks wrote:
>>> On Mar 25, 2013, at 9:30 AM, Jordan Rose <jordan_rose at apple.com 
>>> <mailto:jordan_rose at apple.com>> wrote:
>>>
>>>>
>>>> On Mar 25, 2013, at 8:01 , Anton Yartsev <anton.yartsev at gmail.com 
>>>> <mailto:anton.yartsev at gmail.com>> wrote:
>>>>
>>>>> Committed as r177849
>>>>>
>>>>> Manage to find several random real bugs (report-843813.html, 
>>>>> report-230257.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.
>>>>
>>>> There are two reasons I didn't use getConjuredHeapSymbol when I 
>>>> originally put in this code:
>>>> (1) It handles all CXXNewExprs, even if the allocator is not one of 
>>>> the global ones.
>>>> (2) Heap symbols weren't used yet (Anna added them later for 
>>>> MallocChecker).
>>>>
>>>> Obviously #2 is bogus now. #1 worries me a bit because it requires 
>>>> duplicating some of the checks you just added to MallocChecker.
>>>>
>>>> In the long run, the design would be to get the appropriate memory 
>>>> from the allocator call, and we have PR12014's restructuring of the 
>>>> CFG blocking that. I'm not sure if we'd then move the heap symbol 
>>>> logic into a checker, or if it would still stay in Core.
>>>>
>>>> In the short term, I guess the best idea is to duplicate some of 
>>>> the checks (or refactor them to a helper function 
>>>> somewhere...though probably/not/ into AST) and then conjure a heap 
>>>> symbol if we know we can.
>> Failed to find any suitable place other then AST :) Eventually 
>> noticed, that actually only a single check should be duplicated. 
>> Decided to leave the wide comment added when I tried to find the 
>> proper place for isStandardNewDelete().
>> New fix attached.
>
> I don't think this is safe -- what if the user has custom libraries in 
> their system include path? We can really only assume heap allocation 
> for the implicit case and for ::operator new(std::size_t) and 
> ::operator new(std::size_t, std::nothrow_t).
>
> However, I think just checking "global namespace" might be a good 
> enough approximation. If the user overrides one of the default 
> allocators, it ought to behave like the real one, so we'd only be 
> messing up if they had a new global "allocate from pool" or something.
>
> Also, in theory we'd have to check placement new, but that's handled 
> specially below anyway.
Updated the logic, improved test coverage.
Evolved one more problem to solve: if overloaded standard operator new 
is defined and is called as a function, then it is not recognized as 
overloaded operator for some reason. Tests testOpNewArray() and 
testOpNew() in NewDelete-custom.cpp cover these issue.

>
>
>>
>> Evolved another issue, that I first thought to be related to case 1), 
>> here is the minimal test:
>> struct S {
>>   int **p;
>> };
>> void testOk(S &s) {
>>   new(s.p) (int*)(new int); // false-positive leak
>> }
>> void testLeak() {
>>   S s;
>>   new(s.p) (int*)(new int); // real leak
>> }
>
> Ha. I would guess this is because VisitCXXNewExpr calls directly 
> to bindLoc instead of going through evalBind, and so we miss the 
> pointer-escape-on-bind callback. I can reproduce this with the 
> existing MallocChecker by changing the inner 'new' to a malloc.
>
>> Haven't addressed these yet. The leak is reported for cases of the 
>> form 'small_vector.push_back(new Something)', where push_back() uses 
>> placement new to store data.
>
> I'll get to this case soon if you don't.
>
> Jordan

-- 
Anton

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