<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 27.03.2013 5:23, Jordan Rose wrote:<br>
</div>
<blockquote
cite="mid:9BEA794A-8D25-4F43-B742-A62F39837FFD@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<br>
<div>
<div>On Mar 26, 2013, at 17:10 , 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">
<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>
</blockquote>
Updated the logic, improved test coverage.<br>
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.<br>
<br>
<blockquote
cite="mid:9BEA794A-8D25-4F43-B742-A62F39837FFD@apple.com"
type="cite">
<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>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>