Re: [PATCH for-8.0 1/3] async: Suppress GCC13 false positive in aio_bh_poll()

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

Il mar 21 mar 2023, 11:30 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:

> On Tue, Mar 21, 2023 at 11:22:33AM +0100, Paolo Bonzini wrote:
> > On 3/21/23 09:33, Cédric Le Goater wrote:
> > > From: Cédric Le Goater<clg@redhat.com>
> > >
> > > GCC13 reports an error :
> > >
> > > ../util/async.c: In function ‘aio_bh_poll’:
> > > include/qemu/queue.h:303:22: error: storing the address of local
> variable ‘slice’ in ‘*ctx.bh_slice_list.sqh_last’
> [-Werror=dangling-pointer=]
> > >    303 |     (head)->sqh_last = &(elm)->field.sqe_next;
>           \
> > >        |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
> > > ../util/async.c:169:5: note: in expansion of macro
> ‘QSIMPLEQ_INSERT_TAIL’
> > >    169 |     QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
> > >        |     ^~~~~~~~~~~~~~~~~~~~
> > > ../util/async.c:161:17: note: ‘slice’ declared here
> > >    161 |     BHListSlice slice;
> > >        |                 ^~~~~
> > > ../util/async.c:161:17: note: ‘ctx’ declared here
> > >
> > > But the local variable 'slice' is removed from the global context list
> > > in following loop of the same routine. Add an intermediate helper to
> > > silent GCC. No functional change.
> >
> > Before doing this, I would like to see a case where this bug was _not_
> > caught by either Coverity (which is currently offline but I'm fixing it
> > right now) or just cursory review.
>
> IMHO coverity is not a substitute for this, because it is only available
> post merge, while the GCC warning is available to all maintainers on
> every build. As for code review, mistakes inevitably happen.
>

Okay, then I would like to see a single SIGSEGV in QEMU that was caused by
a local variable making its way to a global pointer.

As to this specific case, we could add a bool removed flag to BHListSlice
and assert it before aio_bh_poll() returns, but I think even that is
overkill.

Paolo

[-- Attachment #2: Type: text/html, Size: 2731 bytes --]