[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:45, Jordan Rose wrote:
>
> On Mar 27, 2013, at 5:32 , Anton Yartsev <anton.yartsev at gmail.com 
> <mailto:anton.yartsev at gmail.com>> wrote:
>
>> 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.
>
> +  // Skip all operator new/delete methods (including static ones).
>
> All operator new/delete methods are implicitly static, so you can just 
> leave out the parenthetical. On the other hand, it might be worth 
> noting that we realize this is an approximation that might not 
> correctly model a custom global allocator.
Fixed at r178244. The note gone to ExprEngine::VisitCXXNewExpr().

-- 
Anton

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