[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 20:58, Anna Zaks wrote:
>
> On Mar 26, 2013, at 6:10 PM, Anna Zaks <ganna at apple.com 
> <mailto:ganna at apple.com>> wrote:
>
>>
>> On Mar 26, 2013, at 5:10 PM, 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.
>>>
>>>>>
>>>>>
>>>>>> 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.
>>>>>
>>>>> Interesting. You can't change a const region (and pedantically you 
>>>>> can't free() it either), but you certainly can 'delete' it. (C++11 
>>>>> [expr.delete]p2)
>>>>>
>>>>> Anna, any thoughts on this? Should these also count as "pointer 
>>>>> escaped" even though their contents have not been invalidated?
>>>>>
>>>>
>>>> I think handling this similarly to pointer escape is the best. 
>>>> However, I am concerned that if we simply extend pointer escape to 
>>>> trigger on another "Kind" of escape, all the other users of 
>>>> pointerEscape will need to know about it (and do nothing for this 
>>>> kind of escape). How about a new checker callback, which will rely 
>>>> on the same core code as _checkPointerEscape? This way the checker 
>>>> developers would not need to know about this special case, and we 
>>>> can reuse all the code that determines when the escape should be 
>>>> triggered.
>>> As for me it would be better to leave the single callback as this is 
>>> just another type of pointer escape, if I understand correctly. Have 
>>> no other arguments for this :)
>>> In addition, the "Kind" approach is relatively new, so hopefully a 
>>> few users of pointerEscape be affected.
>> The main concern is that every user of the callback will now have to 
>> check if the kind is ConstPointer (or whatever we call it, maybe 
>> multiple kinds..) and do nothing in that case. So every user will 
>> need to know about this special case and handle it specially.
> Anton,
>
> If you don't mind, I am going to work on this one. I have some spare 
> time and we'd like to get the new/delete checker out in the next open 
> source build.
I am OK with this, thanx!

>
> Cheers,
> Anna.
>
>>>
>>>
>>> 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
>>> }
>>>
>>> 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.
>>>
>>> -- 
>>> Anton
>>> <FixForCase1.patch>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>

-- 
Anton

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130327/3c076425/attachment.html>