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

[Fam Zheng <famz@redhat.com>]

On Thu, 09/13 16:46, Fei Li wrote:
> 
> 
> On 09/12/2018 03:55 PM, Fam Zheng wrote:
> > On Fri, 09/07 21:38, 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.
> > > 
> > > This patch also adds the omitted error handling when creating signalfd
> > > pipe fails in qemu_signalfd_compat().
> > > 
> > > Signed-off-by: Fei Li <fli@suse.com>
> > > ---
> > >   include/qemu/osdep.h |  2 +-
> > >   util/compatfd.c      |  9 ++++++---
> > >   util/main-loop.c     | 10 +++++-----
> > >   3 files changed, 12 insertions(+), 9 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..d3ed890405 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 allocate signalfd memory");
> > >           errno = ENOMEM;
> > >           return -1;
> > >       }
> > >       if (pipe(fds) == -1) {
> > > +        error_setg(errp, "Failed to create signalfd pipe");
> > >           free(info);
> > >           return -1;
> > >       }
> > > @@ -94,7 +97,7 @@ 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;
> > Extend the context:
> > 
> >         ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8);
> >         if (ret != -1) {
> >             qemu_set_cloexec(ret);
> >             return ret;
> >         }
> > 
> > This error path need error_setg() as well.
> Thanks for the comment. :)
> Like adding as below?
> 
>      if (ret != -1) {
>          qemu_set_cloexec(ret);
>          return ret;
> +    } else {
> +        error_setg(errp, "syscall SYS_signalfd failed: %s",
> strerror(errno));
>      }
>  #endif
> 
>     return qemu_signalfd_compat(mask, errp);
> 
> 
> If yes, I'd like to confirm the logic:

Oh, I missed the fact that qemu_signalfd_compat is a fallback if
qemu_set_cloexec() fails. So no, your original patch is good, ignore what I
said, there's no need to add the extra error_setg here. If you do so and
qemu_signalfd_compat() fails, error_setg() in it will hit the assertion failure.

Fam