[cfe-dev] [Analyzer] Tips on how to fix PR22954 ? (false positive memory leak warning)
Anton Yartsev
anton.yartsev at gmail.com
Fri Jul 17 18:28:56 PDT 2015
On 18.07.2015 2:56, Gábor Horváth wrote:
> Hi!
>
> On 17 July 2015 at 16:15, Anton Yartsev <anton.yartsev at gmail.com
> <mailto: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 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.
The case with pointers seem to be unrelated to the issue and might be a
good idea for a separate enhancement.
>
>
>>
>> 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
>> <mailto: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
>>> <mailto:pierregousseau14 at gmail.com>> wrote:
>>>
>>> Dear All,
>>>
>>> I have been looking into PR22954 which has been kindly
>>> raised by krzystof at
>>> 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 <mailto:cfe-dev at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
--
Anton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150718/f826d626/attachment.html>
More information about the cfe-dev
mailing list