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

[Fei Li]

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.

For the latter, suppose we pass errp straightly. As NULL is passed, the 
error occurs in the
final callee will automatically "propagating" to the initial caller, and 
let it handle this error.
The patch1: qemu_signal_init is this case, let the caller of 
qemu_init_main_loop handle.
In such case, the error_propagate is indeed needless.

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.

Have a nice day, thanks again
Fei
> > +    return ret;
> >   }
> > diff --git a/util/main-loop.c b/util/main-loop.c
> > index affe0403c5..20f6ad3849 100644
> > --- a/util/main-loop.c
> > +++ b/util/main-loop.c
> > @@ -71,10 +71,11 @@ static void sigfd_handler(void *opaque)
> >       }
> >   }
> >   
> > -static int qemu_signal_init(void)
> > +static int qemu_signal_init(Error **errp)
> >   {
> >       int sigfd;
> >       sigset_t set;
> > +    Error *local_err = NULL;
> >   
> >       /*
> >        * SIG_IPI must be blocked in the main thread and must not be caught
> > @@ -94,9 +95,10 @@ static int qemu_signal_init(void)
> >       pthread_sigmask(SIG_BLOCK, &set, NULL);
> >   
> >       sigdelset(&set, SIG_IPI);
> > -    sigfd = qemu_signalfd(&set);
> > +    sigfd = qemu_signalfd(&set, &local_err);
> >       if (sigfd == -1) {
> >           fprintf(stderr, "failed to create signalfd\n");
> > +        error_propagate(errp, local_err);
> >           return -errno;
> >       }
> Again, no need for local_err - just pass errp stright in
>
> >   
> > @@ -109,7 +111,7 @@ static int qemu_signal_init(void)
> >   
> >   #else /* _WIN32 */
> >   
> > -static int qemu_signal_init(void)
> > +static int qemu_signal_init(Error **errp)
> >   {
> >       return 0;
> >   }
> > @@ -148,8 +150,9 @@ int qemu_init_main_loop(Error **errp)
> >   
> >       init_clocks(qemu_timer_notify_cb);
> >   
> > -    ret = qemu_signal_init();
> > +    ret = qemu_signal_init(&local_error);
> >       if (ret) {
> > +        error_propagate(errp, local_error);
> >           return ret;
> >       }
> Again, no need for local_error.
>
>
> Regards,
> Daniel