[cfe-dev] [Analyzer] Tips on how to fix PR22954 ? (false positive memory leak warning)

[Gábor Horváth]

Hi!

On 17 July 2015 at 16:15, Anton Yartsev <anton.yartsev at gmail.com> wrote:

>  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" ?
>
> Just an idea, the memcpy action looks similar to assignment at first
> glance.
> Here is a comment from CString checker relevant to the topic:
>     // Invalidate the destination (regular invalidation without
> pointer-escaping
>     // the address of the top-level region).
>     // FIXME: Even if we can't perfectly model the copy, we should see if
> we
>     // can use LazyCompoundVals to copy the source values into the
> destination.
>     // This would probably remove any existing bindings past the end of the
>     // copied region, but that's still an improvement over blank
> invalidation.
>
>
>
>  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 ?
>
>  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 ?
>
> Here an issue is related to the struct, not array. However arrays are also
> in the risk group.
> I thought of something like TK_DoNotInvalidateSuperRegion. Worker's
> AddToWorkList() automatically adds superregion to worklist:
>   bool AddToWorkList(const MemRegion *R) {
>     const MemRegion *BaseR = R->getBaseRegion();
>     return AddToWorkList(WorkListElement(BaseR), getCluster(BaseR));
>   }
> 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.
> What do you think?
>

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.


>
>
>
>  Thanks for your help,
>
>  Pierre Gousseau
> SN Systems - Sony Computer Entertainment Group
>
>
> On 16 July 2015 at 20:59, Anton Yartsev <anton.yartsev at gmail.com> wrote:
>
>>  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).
>> 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!
>>
>>   Ping !
>> Adding analyzer experts to cc.
>>
>>  Regards,
>>
>>  Pierre Gousseau
>> SN Systems - Sony Computer Entertainment
>>
>> On 2 July 2015 at 09:06, Pierre Gousseau < <pierregousseau14 at gmail.com>
>> pierregousseau14 at gmail.com> wrote:
>>
>>>  Dear All,
>>>
>>>  I have been looking into PR22954 which has been kindly raised by
>>> krzystof at
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D22954&d=AwMDaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3oG6GUNn7_wosSM&m=7uflCt79iL-ecQRK6131Rm86ntE89BlzH5o_4W-AR6Y&s=h7QrSYXR7kaCxfn1t5ZNKbPzt0d9OQiKzOV3wgW-knI&e=>
>>> https://llvm.org/bugs/show_bug.cgi?id=22954 and being new to this area
>>> of Clang I would appreciate any tips on how to fix it.
>>>
>>>  To me the root of the issue seems to originate from the CString
>>> checker as it performs invalidation of the destination buffer.
>>> Given the snippet below:
>>> -----------------
>>> struct aa { char *s; char data[32];};
>>> ...
>>> a.s = malloc(nbytes);
>>> memcpy(a.data, source, len);
>>> ...
>>> -----------------
>>> 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.
>>>
>>>  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'.
>>> However this solution would reduce the number of genuine defects
>>> detected.
>>>
>>>  So I would be grateful if someone could give some hints on how to
>>> provide the right solution.
>>>
>>>  Regards,
>>>
>>>  Pierre Gousseau
>>> SN Systems - Sony Computer Entertainment
>>>
>>
>>
>>
>>   --
>> Anton
>>
>>
>
>
> --
> Anton
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150717/0909268d/attachment.html>