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

[Fei Li]

On 09/20/2018 11:28 AM, Peter Xu wrote:
On Wed, Sep 19, 2018 at 09:35:17PM +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.

This patch also adds the omitted error handling when creating signalfd
pipe fails in qemu_signalfd_compat().

Signed-off-by: Fei Li <f...@suse.com>
Hi, Fei,

Please do s/comc/com/ in the CC list, then I can receive your
emails. :)
So sorry for the misspelling, somehow an unanticipated "xx.com"c is sent. :(

Could you explain a bit how the segfault is triggered?  malloc() and
pipe() aren't something that will fault easily to me, so I would think
we just assert (especially it's in a very early phase of the process).

Thanks,
This is actually triggered after the 7th patch is applied, when I do the 
test
by hard coding the returning value of pthread_create() as EPERM or EINVAL.
Considering there is already an "exit()“ if qemu_init_main_loop() fails, let
us use this exit()?

Have a nice day, thanks for the review. :)
Fei

---
  include/qemu/osdep.h |  2 +-
  util/compatfd.c      |  9 ++++++---
  util/main-loop.c     | 11 +++++------
  3 files changed, 12 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..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;
@@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
      }
  #endif
  
-    return qemu_signalfd_compat(mask);
+    return qemu_signalfd_compat(mask, errp);
  }
diff --git a/util/main-loop.c b/util/main-loop.c
index affe0403c5..b783f3c806 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
      }
  }
  
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
  {
      int sigfd;
      sigset_t set;
@@ -94,10 +94,9 @@ static int qemu_signal_init(void)
      pthread_sigmask(SIG_BLOCK, &set, NULL);
  
      sigdelset(&set, SIG_IPI);
-    sigfd = qemu_signalfd(&set);
+    sigfd = qemu_signalfd(&set, errp);
      if (sigfd == -1) {
-        fprintf(stderr, "failed to create signalfd\n");
-        return -errno;
+        return -1;
      }
  
      fcntl_setfl(sigfd, O_NONBLOCK);
@@ -109,7 +108,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,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
  
      init_clocks(qemu_timer_notify_cb);
  
-    ret = qemu_signal_init();
+    ret = qemu_signal_init(errp);
      if (ret) {
          return ret;
      }
--
2.13.7


Regards,