On Mar 26, 2013, at 17:10 , 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. 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. > > 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 > } Ha. I would guess this is because VisitCXXNewExpr calls directly to bindLoc instead of going through evalBind, and so we miss the pointer-escape-on-bind callback. I can reproduce this with the existing MallocChecker by changing the inner 'new' to a malloc. > 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. I'll get to this case soon if you don't. Jordan
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
