<div dir="ltr"><br><div>Thanks Anton/Gabor for the ideas ! I will be out of the office until the 29th of July unfortunately but I will try to propose a patch when back if the issue is still open.</div><div><br></div><div>Regards,</div><div><br></div><div>Pierre Gousseau</div><div>SN Systems - Sony Computer Entertainment Group</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 17 July 2015 at 18:28, Anton Yartsev <span dir="ltr"><<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span class="">
<div>On 18.07.2015 2:56, Gábor Horváth
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi!<br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On 17 July 2015 at 16:15, Anton
Yartsev <span dir="ltr"><<a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span>
<blockquote type="cite">
<div dir="ltr">Thanks Anton for looking at the
issue, modelling "memcpy" seems like the right
solution, are you saying that code written to
model "assignment" could be reused to model
"memcpy" ?</div>
</blockquote>
</span> Just an idea, the memcpy action looks similar to
assignment at first glance.<br>
Here is a comment from CString checker relevant to the
topic:<br>
// Invalidate the destination (regular invalidation
without pointer-escaping<br>
// the address of the top-level region).<br>
// FIXME: Even if we can't perfectly model the copy,
we should see if we<br>
// can use LazyCompoundVals to copy the source
values into the destination.<br>
// This would probably remove any existing bindings
past the end of the<br>
// copied region, but that's still an improvement
over blank invalidation.<span><br>
<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div><br>
</div>
<div>The patch you provided definitely fixes the
issue thanks, on the other hand if I understand
correctly, it seems to me it can lead to new
false positives, as the analyzer would wrongly
assume that 'a.data''s content remains unchanged
after the memcpy call ?</div>
<div><br>
</div>
<div>I am thinking it might be possible to modify
the region store invalidation worker to restrict
the invalidation in the case of arrays by adding
a new flag like you did, for example
TK_InvalidateArrayOnly ?</div>
</div>
</blockquote>
</span> Here an issue is related to the struct, not
array. However arrays are also in the risk group.<br>
I thought of something like
TK_DoNotInvalidateSuperRegion. Worker's AddToWorkList()
automatically adds superregion to worklist:<br>
bool AddToWorkList(const MemRegion *R) {<br>
const MemRegion *BaseR = R->getBaseRegion();<br>
return AddToWorkList(WorkListElement(BaseR),
getCluster(BaseR));<br>
}<br>
With TK_DoNotInvalidateSuperRegion we could selectively
localize invalidation to a particular subregion, not the
whole superregion. This could work both for classes and
arrays. Perhaps additional coding in
invalidateRegionsWorker::VisitCluster() is required.<br>
What do you think?<br>
</div>
</blockquote>
<div><br>
</div>
<div>I think, in general it depends on what is the relation
between the region and the super region. IIRC casts are
modeled as subregions. So in case of casts every super
region should be invalidated transitively. But the
invalidation should stop on the first element region or
field region or something like that. And you also need to
take pointers into account. In case the invalidated data
contains pointers, it might point to the super region.</div>
</div>
</div>
</div>
</blockquote></span>
Thanks for the thoughts! Definetely TK_DoNotInvalidateSuperRegion
should be attached selectively (struct fields, array elements, maybe
something else?). Perhaps casts may need to be handled somewhere in
the CString checker buffers processing to be more adequate,
additional testing with mamcpy and casts required.<br>
The case with pointers seem to be unrelated to the issue and might
be a good idea for a separate enhancement.<br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
<br>
<blockquote type="cite"><span class=""><span>
<div dir="ltr">
<div><br>
</div>
<div>Thanks for your help,</div>
<div><br>
</div>
<div>Pierre Gousseau</div>
<div>SN Systems - Sony Computer Entertainment
Group</div>
<div>
<div>
<div><br>
</div>
</div>
</div>
</div>
</span>
</span><div class="gmail_extra"><br>
<div class="gmail_quote"><span class=""><span>On 16 July
2015 at 20:59, Anton Yartsev <span dir="ltr"><<a href="mailto:anton.yartsev@gmail.com" target="_blank"></a><a href="mailto:anton.yartsev@gmail.com" target="_blank">anton.yartsev@gmail.com</a>></span>
wrote:<br>
</span>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span class=""><span>
<div>Perhaps the ideal solution is to model
"memcpy" properly (similar to
assignment?). As you correctly noticed
currently we just invalidate the
destination buffer in the CString checker
and the invalidation worker invalidates
the whole struct (do not exactly know the
reason for this).<br>
While experimented wrote a patch
(attached) that fixes the issue by adding
a trait for the struct region that
prevents the region from being
invalidated. The solution does not cause
regressions in the clang testsuite. Do not
know if the solution is acceptable as a
short term fix. Please review!<br>
<br>
</div>
</span>
</span><div>
<div>
<blockquote type="cite"><span class=""><span>
<div dir="ltr">Ping !
<div>Adding analyzer experts to cc.</div>
<div><br>
</div>
<div>Regards,</div>
<div><br>
</div>
<div>
<div style="font-size:12.8000001907349px">Pierre
Gousseau</div>
<div style="font-size:12.8000001907349px">SN
Systems - Sony Computer
Entertainment</div>
</div>
</div>
</span>
</span><div class="gmail_extra"><br>
<div class="gmail_quote"><span class=""><span>On 2 July 2015 at 09:06,
Pierre Gousseau <span dir="ltr"><<a href="mailto:pierregousseau14@gmail.com" target="_blank"></a><a href="mailto:pierregousseau14@gmail.com" target="_blank">pierregousseau14@gmail.com</a>></span>
wrote:<br>
</span>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div>Dear All,</div>
<div><br>
</div>
<div>I have been looking into
PR22954 which has been kindly
raised by krzystof at <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D22954&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3oG6GUNn7_wosSM&m=qsY-43O-niX0gu75vW3KBWErA_b6qCG4GsehJVHmnDk&s=ECHghFnHCPXTnHtD9zfGgWmC3s9hhQRDkLFIFCB7p_I&e=" target="_blank"></a><a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D22954&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3oG6GUNn7_wosSM&m=qsY-43O-niX0gu75vW3KBWErA_b6qCG4GsehJVHmnDk&s=ECHghFnHCPXTnHtD9zfGgWmC3s9hhQRDkLFIFCB7p_I&e=" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=22954</a>
and being new to this area of
Clang I would appreciate any
tips on how to fix it.</div><span class="">
<span>
<div><br>
</div>
<div>To me the root of the
issue seems to originate
from the CString checker as
it performs invalidation of
the destination buffer.</div>
<div>Given the snippet below:</div>
<div>-----------------</div>
<div>struct aa { char *s; char
data[32];};</div>
<div>...</div>
<div>a.s = malloc(nbytes);</div>
<div>memcpy(a.data, source,
len);</div>
<div>...</div>
<div>-----------------</div>
<div>As the CString checker
handles the memcpy call, it
requests the invalidation of
the 'a.data' region. But the
invalidation worker seems to
consider that the whole
memory region of 'a' has to
be invalidated. The Malloc
checker is not made aware of
this causing the false
positive.</div>
<div><br>
</div>
<div>It seems a short term fix
could be to detect this
specific case and have the
CString checker notify the
Malloc checker that it
should stop tracking 'a.s'.</div>
<div>However this solution
would reduce the number of
genuine defects detected.</div>
<div><br>
</div>
<div>So I would be grateful if
someone could give some
hints on how to provide the
right solution.</div>
<div><br>
</div>
<div>Regards,</div>
<div><br>
</div>
<div>Pierre Gousseau</div>
<div>SN Systems - Sony
Computer Entertainment</div>
</span></span></div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
<span><font color="#888888">
<br>
</font></span></div>
<span><font color="#888888">
</font></span></div>
<span><font color="#888888"> <span><font color="#888888">
<pre cols="72">--
Anton</pre>
</font></span></font></span></div>
<span><font color="#888888"> </font></span></blockquote>
<span><font color="#888888"> </font></span></div>
<span><font color="#888888"> <br>
</font></span></div>
<span><font color="#888888"> </font></span></blockquote>
<span><font color="#888888"> <br>
<br>
<pre cols="72">--
Anton</pre>
</font></span></div><span class="">
<br>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
<br>
</span></blockquote>
</div>
<br><span class="HOEnZb"><font color="#888888">
</font></span></div><span class="HOEnZb"><font color="#888888">
</font></span></div><span class="HOEnZb"><font color="#888888">
</font></span></blockquote><span class="HOEnZb"><font color="#888888">
<br>
<br>
<pre cols="72">--
Anton</pre>
</font></span></div>
</blockquote></div><br></div>