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