<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 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>
<br>
<blockquote
cite="mid:9084B92B-44BB-497B-9F7D-2A1AAA599822@apple.com"
type="cite">
<div>
<div>
<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>
</div>
<br>
<blockquote type="cite">
<div text="#000000" bgcolor="#FFFFFF">2) The second type
of false-positives is illustrated by the following
minimal test:<br>
void f(const int & x);<br>
<br>
void test() {<br>
int *p = (int *)malloc(sizeof(int));<br>
f(*p);<br>
} // report false-positive leak<br>
<br>
report-218274.html shows how it looks like in reality.<br>
Haven't addressed this yet. Removing 'const' from the
declaration eliminates the leak report. </div>
</blockquote>
<div><br>
</div>
<div>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)</div>
<div><br>
</div>
<div>Anna, any thoughts on this? Should these also count
as "pointer escaped" even though their contents have not
been invalidated?</div>
<div><br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>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<span style="font-family: Menlo;">? </span>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.</div>
</div>
</div>
</blockquote>
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 :)<br>
In addition, the "Kind" approach is relatively new, so hopefully a
few users of pointerEscape be affected. <br>
<br>
<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>
}<br>
<br>
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>
<br>
<pre class="moz-signature" cols="72">--
Anton</pre>
</body>
</html>