<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Mar 26, 2013, at 17:10 , Anton Yartsev <<a href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 25.03.2013 21:39, Anna Zaks wrote:<br>
</div>
<blockquote cite="mid:9084B92B-44BB-497B-9F7D-2A1AAA599822@apple.com" type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div>
<div>
<div>On Mar 25, 2013, at 9:30 AM, Jordan Rose <<a moz-do-not-send="true" href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="font-family: Helvetica; font-size: 12px;
font-style: normal; font-variant: normal; font-weight:
normal; letter-spacing: normal; line-height: normal;
orphans: auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal; widows: auto;
word-spacing: 0px; -webkit-text-size-adjust: auto;
-webkit-text-stroke-width: 0px;">
<div><br class="Apple-interchange-newline">
On Mar 25, 2013, at 8:01 , Anton Yartsev <<a moz-do-not-send="true" href="mailto:anton.yartsev@gmail.com">anton.yartsev@gmail.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Committed as r177849</div>
<br>
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.<br>
<br>
There are two types of false-positives that form the
majority of reports:<br>
1) Illustrated by the following test (added similar
test to NewDelete-checker-test.mm):<br>
int *global;<br>
void testMemIsOnHeap() {<br>
int *p = new int; // FIXME: currently not heap
allocated!<span class="Apple-converted-space"> </span><br>
if (global != p) // evaluates to UnknownVal() rather
then 'true'<br>
global = p;<br>
} // report false-positive leak<br>
<br>
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))<br>
<br>
Attached is the proposed patch that fixes these
issues. </div>
</blockquote>
<div><br>
</div>
<div>There are two reasons I didn't use
getConjuredHeapSymbol when I originally put in this
code:</div>
<div>(1) It handles all CXXNewExprs, even if the allocator
is not one of the global ones.</div>
<div>(2) Heap symbols weren't used yet (Anna added them
later for MallocChecker).</div>
<div><br>
</div>
<div>Obviously #2 is bogus now. #1 worries me a bit
because it requires duplicating some of the checks you
just added to MallocChecker.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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<span class="Apple-converted-space"> </span><i>not</i> into
AST) and then conjure a heap symbol if we know we can.</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
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().<br>
New fix attached.<br></div></blockquote><div><br></div><div>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).</div><div><br></div><div>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.</div><div><br></div><div>Also, in theory we'd have to check placement new, but that's handled specially below anyway.</div><div><br></div><br><blockquote type="cite"><div text="#000000" bgcolor="#FFFFFF">
<br>
Evolved another issue, that I first thought to be related to case
1), here is the minimal test:<br>
struct S {<br>
int **p;<br>
};<br>
void testOk(S &s) {<br>
new(s.p) (int*)(new int); // false-positive leak<br>
}<br>
void testLeak() {<br>
S s;<br>
new(s.p) (int*)(new int); // real leak<br>
} </div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote type="cite"><div text="#000000" bgcolor="#FFFFFF">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.<br></div></blockquote></div><br><div>I'll get to this case soon if you don't.</div><div><br></div><div>Jordan</div></body></html>