Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails

[Daniel P . Berrangé]

On Wed, Sep 05, 2018 at 12:17:24PM +0800, Fei Li wrote:
> Thanks for the review! :)
> 
> 
> On 09/04/2018 07:26 PM, Daniel P. Berrangé wrote:
> > On Tue, Sep 04, 2018 at 07:08:18PM +0800, Fei Li wrote:
> > > Currently, when qemu_signal_init() fails it only returns a non-zero
> > > value but without propagating any Error. But its callers need a
> > > non-null err when runs error_report_err(err), or else 0->msg occurs.
> > > 
> > > To avoid such segmentation fault, add a new Error parameter to make
> > > the call trace to propagate the err to the final caller.
> > > 
> > > Signed-off-by: Fei Li <address@hidden>
> > > ---
> > >   include/qemu/osdep.h |  2 +-
> > >   util/compatfd.c      | 17 ++++++++++++-----
> > >   util/main-loop.c     | 11 +++++++----
> > >   3 files changed, 20 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index a91068df0e..09ed85fcb8 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
> > >                                additional fields in the future) */
> > >   };
> > > -int qemu_signalfd(const sigset_t *mask);
> > > +int qemu_signalfd(const sigset_t *mask, Error **errp);
> > >   void sigaction_invoke(struct sigaction *action,
> > >                         struct qemu_signalfd_siginfo *info);
> > >   #endif
> > > diff --git a/util/compatfd.c b/util/compatfd.c
> > > index 980bd33e52..65501de622 100644
> > > --- a/util/compatfd.c
> > > +++ b/util/compatfd.c
> > > @@ -16,6 +16,7 @@
> > >   #include "qemu/osdep.h"
> > >   #include "qemu-common.h"
> > >   #include "qemu/thread.h"
> > > +#include "qapi/error.h"
> > >   #include <sys/syscall.h>
> > > @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
> > >       }
> > >   }
> > > -static int qemu_signalfd_compat(const sigset_t *mask)
> > > +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
> > >   {
> > >       struct sigfd_compat_info *info;
> > >       QemuThread thread;
> > > @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
> > >       info = malloc(sizeof(*info));
> > >       if (info == NULL) {
> > > +        error_setg(errp, "Failed to malloc in %s", __func__);
> > I'd avoid using __func__ in error messages. Instead
> > 
> >     "Failed to allocate signalfd memory"
> Ok, thanks.
> > 
> > >           errno = ENOMEM;
> > >           return -1;
> > >       }
> > >       if (pipe(fds) == -1) {
> > > +        error_setg(errp, "Failed to create a pipe in %s", __func__);
> >      "Failed to create signalfd pipe"
> OK.
> > 
> > >           free(info);
> > >           return -1;
> > >       }
> > > @@ -94,17 +97,21 @@ static int qemu_signalfd_compat(const sigset_t *mask)
> > >       return fds[0];
> > >   }
> > > -int qemu_signalfd(const sigset_t *mask)
> > > +int qemu_signalfd(const sigset_t *mask, Error **errp)
> > >   {
> > > -#if defined(CONFIG_SIGNALFD)
> > >       int ret;
> > > +    Error *local_err = NULL;
> > > +#if defined(CONFIG_SIGNALFD)
> > >       ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8);
> > >       if (ret != -1) {
> > >           qemu_set_cloexec(ret);
> > >           return ret;
> > >       }
> > >   #endif
> > > -
> > > -    return qemu_signalfd_compat(mask);
> > > +    ret = qemu_signalfd_compat(mask, &local_err);
> > > +    if (local_err) {
> > > +        error_propagate(errp, local_err);
> > > +    }
> > Using a local_err is not required - you can just pass errp stright
> > to qemu_signalfd_compat() and then check
> > 
> >     if (ret < 0)
> For the use of a local error object & error_propagate call, I'd like to
> explain here. :)
> In our code, the initial caller passes two kinds of Error to the call trace,
> one is
> something like &error_abort and &error_fatal, the other is NULL.
> 
> For the former, the exit() occurs in the functions where
> error_handle_fatal() is called
> (e.g. called by error_propagate/error_setg/...). The patch3: qemu_init_vcpu
> is the case,
> that means the system will exit in the final callee: qemu_thread_create(),
> instead of
> the initial caller pc_new_cpu(). In such case, I think propagating seems
> more reasonable.

I don't really agree. It is preferrable to abort immediately at the deepest
place which raises the error. The stack trace will thus show the full call
chain leading upto the problem.

> How do you think passing errp straightly for the latter case, and use a
> local error object &
> error_propagate for the former case? This is a distinct treatment, but would
> shorten the code.

It is inappropriate to second-guess whether the caller is a passing in
NULL or &error_abort, or another Error object. What is passed in can
change at any time in the future. 

We should only ever use a local error where the local method has a need
to look at the error contents before returning to the caller. Any other
case should just use the errp directly.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|