On Mar 26, 2013, at 6:10 PM, Anna Zaks <[email protected]> wrote:
>
> On Mar 26, 2013, at 5:10 PM, Anton Yartsev <[email protected]> wrote:
>
>> On 25.03.2013 21:39, Anna Zaks wrote:
>>> On Mar 25, 2013, at 9:30 AM, Jordan Rose <[email protected]> wrote:
>>>
>>>>
>>>> On Mar 25, 2013, at 8:01 , Anton Yartsev <[email protected]> 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.
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
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
