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

[Jordan Rose]

On Mar 27, 2013, at 5:32 , Anton Yartsev <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> 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> wrote:
>>>> 
>>>>> 
>>>>> On Mar 25, 2013, at 8:01 , Anton Yartsev <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.


+  symVal = IsStandardGlobalOpNewFunction
+           ? svalBuilder.getConjuredHeapSymbolVal(CNE, LCtx, blockCount)
+           : svalBuilder.conjureSymbolVal(0, CNE, LCtx, CNE->getType(), 
+                                            blockCount);

I'm not sure what prevailing LLVM style is for long ?: lines, but given that the conjureSymbolVal call ends up wrapping as well, I think it might be worth just splitting this into a full if-statement.

Other than that this looks good to me!


> 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.


You can check if it has to do with redeclarations of the allocator function, but I wouldn't expect that. Definitely something we need to fix before putting out another open-source checker build.

I'll get the bind fix in today.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130327/1fddbbc3/attachment.html>