Re: [Xen-devel] [PATCH] arm: Coverity 1469342 correct find_*_bit() functions use

[Artem Mygaiev <artem_mygaiev@xxxxxxxx>]

Hi,

On 24.05.18 17:22, Julien Grall wrote:
> On 24/05/18 15:07, Artem Mygaiev wrote:
> > Hi Julien
> Hi Artem,
>
> > On 24.05.18 16:49, Julien Grall wrote:
> > > Hi Artem,
> > >
> > > Thank you for the report.
> > >
> > > On 24/05/18 14:20, Artem Mygaiev wrote:
> > > > vgic_vcpu_pending_irq() uses find_next_bit() library function with 
> > > > single 'unsigned long' variable, while it is designed to work with 
> > > > memory regions. Nothing wrong is happening since 'offset' is set to 
> > > > 0 (other values could lead to memory corruption), but it would be 
> > > > more correct to use the find_first_bit() function instead.
> > > I don't understand the commit message. It is fine to use other offset 
> > > than 0 in find_next_bit as long as it is smaller than 32. There would 
> > > be no corruption happening.
> > > Furthermore, find_first_bit(&apr, 32, 0) and find_next_bit(&apr, 32) 
> > > are equivalent because the former is just a macro using the latter 
> > > (see include/asm-arm/arm64/bitops.h).
> > > So as it is the patch is not solving anything. However, I think this 
> > > is just a false positive. Coverity should be able to guess that it 
> > > will not go past the array (BITOP_WORD will turned into 0).
> > Absolutely agree with you. Probably my message was not clear enough - 
> > with this particular patch I am not trying to fix a memory corruption, 
> > there is no memory corruption in the code now. It is just the use of 
> > functions: find_first_bit() is a better fit since the 
> > vgic_vcpu_pending_irq() function does not need to go over memory 
> > region and checks only one 32-bit variable. I have mentioned Coverity 
> > issue here because this was a false positive detected after today's 
> > test run.
> Feel free to resend this patch as a clean-up. I always found the use of 
> find_next_bit confusing over find_first_bit.
> However, I don't think coverity should be mention in the commit message 
> or title. This is making thing very confusing as, IHMO, this is a false 
> positive. We tend to only mention coverity when this is either going to 
> silent coverity or fix the defect.
Ok, I'll adjust the subject & commit message, and re-send. JFYI, I've 
marked this one as False Positive in Coverity; there were three more 
suspects produced in today's run, but nothing really worth attention.
> Cheers,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel