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

[Anton Yartsev]

> 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?


>
> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150718/2f8c18f0/attachment.html>