block: Fix NULL dereference on empty drive error

[Qemu-devel] [PATCH] block: Fix NULL dereference on empty drive error

Posted by Kevin Wolf 7 years, 7 months ago

blk_error_action() sends a BLOCK_IO_ERROR QMP event which includes the
node name of its root node. If the BlockBackend represents an empty
drive, there is no root node, so we should not try to access its node
name. Make the field optional in the event and include it only when
the BlockBackend isn't empty.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---

Stefan, this is needed for your patch that reverts the workaround in the
IDE flush code. Without it, make check seems to succeed, but if you look
closer, qemu actually segfaults.

qapi/block-core.json  | 6 ++++--
 block/block-backend.c | 5 +++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5c5921bfb7..00475f08d4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3676,7 +3676,8 @@
 #
 # @node-name: node name. Note that errors may be reported for the root node
 #             that is directly attached to a guest device rather than for the
-#             node where the error occurred. (Since: 2.8)
+#             node where the error occurred. The node name is not present if
+#             the drive is empty. (Since: 2.8)
 #
 # @operation: I/O operation
 #
@@ -3707,7 +3708,8 @@
 #
 ##
 { 'event': 'BLOCK_IO_ERROR',
-  'data': { 'device': 'str', 'node-name': 'str', 'operation': 'IoOperationType',
+  'data': { 'device': 'str', '*node-name': 'str',
+            'operation': 'IoOperationType',
             'action': 'BlockErrorAction', '*nospace': 'bool',
             'reason': 'str' } }
 
diff --git a/block/block-backend.c b/block/block-backend.c
index a775a3dd2f..a4421252f8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1615,10 +1615,11 @@ static void send_qmp_error_event(BlockBackend *blk,
                                  bool is_read, int error)
 {
     IoOperationType optype;
+    BlockDriverState *bs = blk_bs(blk);
 
     optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
-    qapi_event_send_block_io_error(blk_name(blk),
-                                   bdrv_get_node_name(blk_bs(blk)), optype,
+    qapi_event_send_block_io_error(blk_name(blk), !!bs,
+                                   bs ? bdrv_get_node_name(bs) : NULL, optype,
                                    action, blk_iostatus_is_enabled(blk),
                                    error == ENOSPC, strerror(error),
                                    &error_abort);
-- 
2.13.6

Re: [Qemu-devel] [PATCH] block: Fix NULL dereference on empty drive error

Posted by Eric Blake 7 years, 7 months ago

On 03/05/2018 09:05 AM, Kevin Wolf wrote:
> blk_error_action() sends a BLOCK_IO_ERROR QMP event which includes the
> node name of its root node. If the BlockBackend represents an empty
> drive, there is no root node, so we should not try to access its node
> name. Make the field optional in the event and include it only when
> the BlockBackend isn't empty.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> 
> Stefan, this is needed for your patch that reverts the workaround in the
> IDE flush code. Without it, make check seems to succeed, but if you look
> closer, qemu actually segfaults.
> 
> qapi/block-core.json  | 6 ++++--
>   block/block-backend.c | 5 +++--
>   2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5c5921bfb7..00475f08d4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3676,7 +3676,8 @@
>   #
>   # @node-name: node name. Note that errors may be reported for the root node
>   #             that is directly attached to a guest device rather than for the
> -#             node where the error occurred. (Since: 2.8)
> +#             node where the error occurred. The node name is not present if
> +#             the drive is empty. (Since: 2.8)

Making an output field change from always present to sometimes absent 
might break older clients that expected to be able to parse the field 
unconditionally.  Would it be better to keep the 'node-name' field 
mandatory in the output but make it an empty string?

Then again, since the field was not present prior to 2.8, but the event 
itself is older, we can argue that clients of older qemu have to be 
prepared for the field to not be present.  So I think I can live with 
this change as-is.

>   #
>   # @operation: I/O operation
>   #
> @@ -3707,7 +3708,8 @@
>   #
>   ##
>   { 'event': 'BLOCK_IO_ERROR',
> -  'data': { 'device': 'str', 'node-name': 'str', 'operation': 'IoOperationType',
> +  'data': { 'device': 'str', '*node-name': 'str',
> +            'operation': 'IoOperationType',
>               'action': 'BlockErrorAction', '*nospace': 'bool',
>               'reason': 'str' } }
>   

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] block: Fix NULL dereference on empty drive error

Posted by Kevin Wolf 7 years, 7 months ago

Am 05.03.2018 um 17:10 hat Eric Blake geschrieben:
> On 03/05/2018 09:05 AM, Kevin Wolf wrote:
> > blk_error_action() sends a BLOCK_IO_ERROR QMP event which includes the
> > node name of its root node. If the BlockBackend represents an empty
> > drive, there is no root node, so we should not try to access its node
> > name. Make the field optional in the event and include it only when
> > the BlockBackend isn't empty.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > 
> > Stefan, this is needed for your patch that reverts the workaround in the
> > IDE flush code. Without it, make check seems to succeed, but if you look
> > closer, qemu actually segfaults.
> > 
> > qapi/block-core.json  | 6 ++++--
> >   block/block-backend.c | 5 +++--
> >   2 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 5c5921bfb7..00475f08d4 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3676,7 +3676,8 @@
> >   #
> >   # @node-name: node name. Note that errors may be reported for the root node
> >   #             that is directly attached to a guest device rather than for the
> > -#             node where the error occurred. (Since: 2.8)
> > +#             node where the error occurred. The node name is not present if
> > +#             the drive is empty. (Since: 2.8)
> 
> Making an output field change from always present to sometimes absent
> might break older clients that expected to be able to parse the field
> unconditionally.  Would it be better to keep the 'node-name' field
> mandatory in the output but make it an empty string?

I considered that, but how likely is it that a client can handle an
empty string instead of a valid node name, but can't handle an absent
field? I assume that such clients would probably break either way. And
in that case I preferred to use the cleaner design.

> Then again, since the field was not present prior to 2.8, but the event
> itself is older, we can argue that clients of older qemu have to be prepared
> for the field to not be present.  So I think I can live with this change
> as-is.

Right, that too. If libvirt can deal with it (and I suppose it can
because it doesn't really use node names yet), we should be okay.
> 
> >   #
> >   # @operation: I/O operation
> >   #
> > @@ -3707,7 +3708,8 @@
> >   #
> >   ##
> >   { 'event': 'BLOCK_IO_ERROR',
> > -  'data': { 'device': 'str', 'node-name': 'str', 'operation': 'IoOperationType',
> > +  'data': { 'device': 'str', '*node-name': 'str',
> > +            'operation': 'IoOperationType',
> >               'action': 'BlockErrorAction', '*nospace': 'bool',
> >               'reason': 'str' } }
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks.

Kevin