Re: [PATCH 1/2] xen/console: fix error handling in xen_console_device_create()

[Anthony PERARD]

On Tue, Jan 07, 2025 at 10:31:39AM +0100, Roger Pau Monne wrote:
> The usage of error_prepend() in some of the error contexts of
> xen_console_device_create() is incorrect, as `errp` hasn't been initialized.
> This leads to the following segmentation fault on error paths resulting from
> xenstore reads:
> 
> Program terminated with signal SIGSEGV, Segmentation fault.
> Address not mapped to object.
>     fmt=0x15c4dfeade42 "failed to read console device type: ", 
> ap=0x15cd0165ab50)
>     at ../qemu-xen-dir-remote/util/error.c:142
> 142         g_string_append(newmsg, (*errp)->msg);
> [...]
> (gdb) bt
>     (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device 
> type: ", ap=0x15cd0165ab50) at ../qemu-xen-dir-remote/util/error.c:142
>     (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device 
> type: ")
>     at ../qemu-xen-dir-remote/util/error.c:152
>     (backend=0x43944de00660, opts=0x43944c929000, errp=0x15cd0165ae10)
>     at ../qemu-xen-dir-remote/hw/char/xen_console.c:555
> 
> Replace usages of error_prepend() with error_setg() where appropriate.
> 
> Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  hw/char/xen_console.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index ef0c2912efa1..af706c7ef440 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -551,7 +551,7 @@ static void xen_console_device_create(XenBackendInstance 
> *backend,
>      }
>  
>      if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> -        error_prepend(errp, "failed to read console device type: ");
> +        error_setg(errp, "failed to read console device type: ");

According to error_setg() doc, *errp must be NULL but xs_node_scanf may
set it. Looking at the implementation, error_setg() seems to simply
discard this new error message if *errp is already set.

Currently, when there's an I/O error, we get something like:
    failed to read console device type: failed to read from /xenstore/path: 
doesn't exist
and when the format scan failed:
    SEGV

With this patch, when there's an I/O error, I think we get something
like:
    failed to read from /xenstore/path: doesn't exist
and when the format scan failed:
    failed to read console device type: 


So I think we'll want to distiguish between IO error from
xs_node_scanf() and format error, first one returns EOF (like vsscanf)
and second one returns a value >= 0 but we expect exactly 1.


>          goto fail;
>      }
>  
> @@ -582,7 +582,7 @@ static void xen_console_device_create(XenBackendInstance 
> *backend,
>      } else if (number) {
>          cd = serial_hd(number);
>          if (!cd) {
> -            error_prepend(errp, "console: No serial device #%ld found: ",
> +            error_setg(errp, "console: No serial device #%ld found: ",
>                            number);

This change looks correct, ableit we could remove ":  " from the end of
the string since they shouldn't be anything after it.


Cheers,

-- 
Anthony PERARD