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>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony PERARD <anthony@xenproject.org>
Cc: Paul Durrant <paul@xen.org>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: xen-devel@lists.xenproject.org
---
 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: ");
         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);
             goto fail;
         }
-- 
2.46.0

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

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.