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

[Anton Yartsev]

On 28.03.2013 1:18, Anna Zaks wrote:
>
> On Mar 27, 2013, at 10:13 AM, Anton Yartsev <anton.yartsev at gmail.com 
> <mailto:anton.yartsev at gmail.com>> wrote:
>
>> 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!
>>
> Anton,
>
> I've realized that I need the part of your new/delete work that tracks 
> families for fixing this. Specifically, I want to assume that a const 
> pointer cannot be freed but could be deleted. Can you commit the 
> remaining patches? Specifically, I need the part that performs family 
> tracking, but you can commit the mismatched deallocators work as well.
Committed at r178250

-- 
Anton

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130328/2063b38a/attachment.html>