Hi
On Fri, Nov 1, 2024 at 7:26 PM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Tue, 15 Oct 2024 at 09:52, <marcandre.lur...@redhat.com> wrote:
> >
> > From: Roman Penyaev <r.peni...@gmail.com>
> >
> > With bitset management now it becomes feasible to implement
> > the logic of detaching frontends from multiplexer.
> >
> > Signed-off-by: Roman Penyaev <r.peni...@gmail.com>
> > Cc: "Marc-André Lureau" <marcandre.lur...@redhat.com>
> > Cc: qemu-devel@nongnu.org
> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > Message-ID: <20241014152408.427700-8-r.peni...@gmail.com>
>
> Hi; Coverity reports an issue with this patch. I think
> it's probably a bit confused, but on the other hand
> I read the code and am also a bit confused so we could
> probably adjust it to be clearer...
>
> > +bool mux_chr_detach_frontend(MuxChardev *d, unsigned int tag)
> > +{
> > + unsigned int bit;
> > +
> > + bit = find_next_bit(&d->mux_bitset, MAX_MUX, tag);
>
> Why are we calling find_next_bit() here? Coverity
> points out that it can return MAX_MUX, which means that
> if the caller passed in MAX_MUX then we will try to
> dereference d->backends[MAX_MUX] which is off the
> end of the array.
>
> What I was expecting this code to do was to check
> "is the bit actually currently set?", which would be
> if (!(d->mux_bitset & (1 << bit))) {
> ...
> }
>
It's actually sort of checking the tag bit is set:
bit = find_next_bit(&d->mux_bitset, MAX_MUX, tag);
if (bit != tag) { // <- here
return false;
The function should probably assert(tag < MAX_MUX) though, that will help
coverity I guess
> Why do we want to find the next bit set after the
> 'tag' bit ?
>
> > + if (bit != tag) {
> > + return false;
> > + }
> > +
> > + d->mux_bitset &= ~(1 << bit);
> > + d->backends[bit] = NULL;
> > +
> > + return true;
> > +}
>
> (This is CID 1563776.)
>
> thanks
> -- PMM
>
>
--
Marc-André Lureau
