diff mbox series

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

Message ID 20230321083322.663561-2-clg@kaod.org
State New
Headers show
Series Fixes for GCC13 | expand

Commit Message

Cédric Le Goater March 21, 2023, 8:33 a.m. UTC 
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.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 util/async.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini March 21, 2023, 10:22 a.m. UTC | #1 
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.

I'd rather remove the warning.

Paolo
Daniel P. Berrangé March 21, 2023, 10:30 a.m. UTC | #2 
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. 

Personally I find the code in this method pretty obtuse. It is hard to
reason about it to convince yourself that it is safe to be adding the
local variable to the global linked list and have it removed again
before returning.

Stefan has explained why it is correct, but I tend to think of the compiler
warning here as a sign that the code might be better to be written in a
different way that is more obviously correct. If this really is the best
way to write this method though, an alternative could be selectively
disabling the warning with a local pragma, along with adding a comment
to the method to explain why this unusual code pattern is indeed safe.

With regards,
Daniel
Paolo Bonzini March 21, 2023, 10:57 a.m. UTC | #3 
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
Stefan Hajnoczi March 21, 2023, 11:15 a.m. UTC | #4 
On Tue, Mar 21, 2023 at 09:33:20AM +0100, 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.
> 
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  util/async.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Thanks!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Paolo Bonzini March 21, 2023, 11:57 a.m. UTC | #5 
On 3/21/23 09:33, Cédric Le Goater wrote:
> +static void aio_bh_slice_insert(AioContext *ctx, BHListSlice *slice)
> +{
> +    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, slice, next);
> +}
> +
>   /* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
>   int aio_bh_poll(AioContext *ctx)
>   {
> @@ -164,7 +169,13 @@ int aio_bh_poll(AioContext *ctx)
>   
>       /* Synchronizes with QSLIST_INSERT_HEAD_ATOMIC in aio_bh_enqueue().  */
>       QSLIST_MOVE_ATOMIC(&slice.bh_list, &ctx->bh_list);
> -    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
> +
> +    /*
> +     * GCC13 [-Werror=dangling-pointer=] complains that the local variable
> +     * 'slice' is being stored in a global list in 'ctx->bh_slice_list'.
> +     * Use a helper to silent the compiler
> +     */
> +    aio_bh_slice_insert(ctx, &slice);
>   
>       while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
>           QEMUBH *bh;
> -- 

Sorry, but an API that has "insert" and not "remove", and where the 
argument is *expected to be* a local variable (which must be removed to 
avoid a dangling pointer---and the warning is exactly 
-Wdangling-pointer), ranks at least -7 in the bad API ranking[1].

I tried wrapping the BHListSlice and BHListSlice* into an iterator 
struct (which is also really overkill, but at least---in theory---it's 
idiomatic), but the code was hard to follow.

The fact that the workaround is so ugly, in my opinion, points even more 
strongly at the compiler being in the wrong here.

Paolo

[1] http://sweng.the-davies.net/Home/rustys-api-design-manifesto
Cédric Le Goater March 21, 2023, 12:16 p.m. UTC | #6 
On 3/21/23 12:57, Paolo Bonzini wrote:
> On 3/21/23 09:33, Cédric Le Goater wrote:
>> +static void aio_bh_slice_insert(AioContext *ctx, BHListSlice *slice)
>> +{
>> +    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, slice, next);
>> +}
>> +
>>   /* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
>>   int aio_bh_poll(AioContext *ctx)
>>   {
>> @@ -164,7 +169,13 @@ int aio_bh_poll(AioContext *ctx)
>>       /* Synchronizes with QSLIST_INSERT_HEAD_ATOMIC in aio_bh_enqueue().  */
>>       QSLIST_MOVE_ATOMIC(&slice.bh_list, &ctx->bh_list);
>> -    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
>> +
>> +    /*
>> +     * GCC13 [-Werror=dangling-pointer=] complains that the local variable
>> +     * 'slice' is being stored in a global list in 'ctx->bh_slice_list'.
>> +     * Use a helper to silent the compiler
>> +     */
>> +    aio_bh_slice_insert(ctx, &slice);
>>       while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
>>           QEMUBH *bh;
>> -- 
> 
> Sorry, but an API that has "insert" and not "remove", and where the argument is *expected to be* a local variable (which must be removed to avoid a dangling pointer---and the warning is exactly -Wdangling-pointer), ranks at least -7 in the bad API ranking[1].

:)

> I tried wrapping the BHListSlice and BHListSlice* into an iterator struct (which is also really overkill, but at least---in theory---it's idiomatic), but the code was hard to follow.
> 
> The fact that the workaround is so ugly, in my opinion, points even more strongly at the compiler being in the wrong here.

It was initially called slice_dangling_pointer_fixup() how's that ?

An alternative could be :

@@ -164,7 +164,14 @@ int aio_bh_poll(AioContext *ctx)
  
      /* Synchronizes with QSLIST_INSERT_HEAD_ATOMIC in aio_bh_enqueue().  */
      QSLIST_MOVE_ATOMIC(&slice.bh_list, &ctx->bh_list);
+    /*
+     * GCC13 [-Werror=dangling-pointer=] complains that the local variable
+     * 'slice' is being stored in the global list 'ctx->bh_slice_list'.
+     */
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdangling-pointer="
      QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
+#pragma GCC diagnostic pop
  
      while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
          QEMUBH *bh;

May be that's more explicit. I wonder if we need to ifdef clang also.

Thanks,

C.
Paolo Bonzini March 21, 2023, 1:02 p.m. UTC | #7 
On 3/21/23 13:16, Cédric Le Goater wrote:
> 
> +    /*
> +     * GCC13 [-Werror=dangling-pointer=] complains that the local variable
> +     * 'slice' is being stored in the global list 'ctx->bh_slice_list'.
> +     */
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wdangling-pointer="
>       QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
> +#pragma GCC diagnostic pop
> 
>       while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
>           QEMUBH *bh;

Yeah, that's clearer.  Maybe even add "but the list is emptied before 
this function returns".

> May be that's more explicit. I wonder if we need to ifdef clang also.

I think clang understand the GCC pragma as well.

Paolo
diff mbox series

Patch

diff --git a/util/async.c b/util/async.c
index 21016a1ac7..45be1ed218 100644
--- a/util/async.c
+++ b/util/async.c
@@ -155,6 +155,11 @@  void aio_bh_call(QEMUBH *bh)
     bh->cb(bh->opaque);
 }
 
+static void aio_bh_slice_insert(AioContext *ctx, BHListSlice *slice)
+{
+    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, slice, next);
+}
+
 /* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
 int aio_bh_poll(AioContext *ctx)
 {
@@ -164,7 +169,13 @@  int aio_bh_poll(AioContext *ctx)
 
     /* Synchronizes with QSLIST_INSERT_HEAD_ATOMIC in aio_bh_enqueue().  */
     QSLIST_MOVE_ATOMIC(&slice.bh_list, &ctx->bh_list);
-    QSIMPLEQ_INSERT_TAIL(&ctx->bh_slice_list, &slice, next);
+
+    /*
+     * GCC13 [-Werror=dangling-pointer=] complains that the local variable
+     * 'slice' is being stored in a global list in 'ctx->bh_slice_list'.
+     * Use a helper to silent the compiler
+     */
+    aio_bh_slice_insert(ctx, &slice);
 
     while ((s = QSIMPLEQ_FIRST(&ctx->bh_slice_list))) {
         QEMUBH *bh;