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

[Roger Pau Monné]

On Thu, Jan 09, 2025 at 11:13:45AM +0100, Anthony PERARD wrote:
> 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.

The call to xs_node_scanf() will go away in the next patch replaced by
qemu_xen_xs_read(), at which point errp will never be initialized.

I can change the order of the patches if that makes it easier.

> 
> >          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.

Thanks, Roger.