Libguestfs - Apr 2016 - [PATCH] Add safe wrapper around waitpid which deals with EINTR correctly.

As Eric Blake noted in:
https://www.redhat.com/archives/libguestfs/2016-April/msg00154.html
libguestfs doesn't correctly handle the case where waitpid receives a
SIGCHLD signal and the main program has registered a non-restartable
signal handler.

In this case waitpid would return -EINTR and we would print an error,
but actually we should retry this case.

This adds two new internal functions, one for handling waitpid and
printing (real) errors from it, the other for just waiting for a
process where we don't particularly care about errors.

Rich.

Thanks: Eric Blake.
---
 src/Makefile.am        |  1 +
 src/command.c          |  7 ++----
 src/guestfs-internal.h |  4 +++
 src/launch-direct.c    | 11 +++------
 src/launch-uml.c       | 11 +++------
 src/umask.c            | 10 ++------
 src/wait.c             | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 84 insertions(+), 27 deletions(-)
 create mode 100644 src/wait.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 34c4fa6..7cc80c4 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -131,6 +131,7 @@ libguestfs_la_SOURCES = \
 	structs-free.c \
 	tmpdirs.c \
 	umask.c \
+	wait.c \
 	whole-file.c \
 	libguestfs.syms
 
diff --git a/src/command.c b/src/command.c
index 866847d..e2bbf65 100644
--- a/src/command.c
+++ b/src/command.c
@@ -74,7 +74,6 @@
 #include <assert.h>
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <sys/wait.h>
 #include <sys/select.h>
 
 #ifdef HAVE_SYS_TIME_H
@@ -692,10 +691,8 @@ wait_command (struct command *cmd)
 {
   int status;
 
-  if (waitpid (cmd->pid, &status, 0) == -1) {
-    perrorf (cmd->g, "waitpid");
+  if (guestfs_int_waitpid (cmd->g, cmd->pid, &status,
"command") == -1)
     return -1;
-  }
 
   cmd->pid = 0;
 
@@ -902,7 +899,7 @@ guestfs_int_cmd_close (struct command *cmd)
   free (cmd->outbuf.buffer);
 
   if (cmd->pid > 0)
-    waitpid (cmd->pid, NULL, 0);
+    guestfs_int_waitpid_noerror (cmd->pid);
 
   for (child_rlimit = cmd->child_rlimits; child_rlimit != NULL;
        child_rlimit = child_rlimit_next) {
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index bf107d0..7b3927b 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -918,4 +918,8 @@ extern int guestfs_int_validate_guid (const char *);
 /* umask.c */
 extern int guestfs_int_getumask (guestfs_h *g);
 
+/* wait.c */
+extern int guestfs_int_waitpid (guestfs_h *g, pid_t pid, int *status, const
char *errmsg);
+extern void guestfs_int_waitpid_noerror (pid_t pid);
+
 #endif /* GUESTFS_INTERNAL_H_ */
diff --git a/src/launch-direct.c b/src/launch-direct.c
index 322737d..ee0a855 100644
--- a/src/launch-direct.c
+++ b/src/launch-direct.c
@@ -26,7 +26,6 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <sys/types.h>
-#include <sys/wait.h>
 #include <sys/stat.h>
 #include <signal.h>
 #include <sys/socket.h>
@@ -851,8 +850,8 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
     close (sv[0]);
   if (data->pid > 0) kill (data->pid, 9);
   if (data->recoverypid > 0) kill (data->recoverypid, 9);
-  if (data->pid > 0) waitpid (data->pid, NULL, 0);
-  if (data->recoverypid > 0) waitpid (data->recoverypid, NULL, 0);
+  if (data->pid > 0) guestfs_int_waitpid_noerror (data->pid);
+  if (data->recoverypid > 0) guestfs_int_waitpid_noerror
(data->recoverypid);
   data->pid = 0;
   data->recoverypid = 0;
   memset (&g->launch_t, 0, sizeof g->launch_t);
@@ -1484,16 +1483,14 @@ shutdown_direct (guestfs_h *g, void *datav, int
check_for_errors)
 
   /* Wait for subprocess(es) to exit. */
   if (g->recovery_proc /* RHBZ#998482 */ && data->pid > 0) {
-    if (waitpid (data->pid, &status, 0) == -1) {
-      perrorf (g, "waitpid (qemu)");
+    if (guestfs_int_waitpid (g, data->pid, &status, "qemu") ==
-1)
       ret = -1;
-    }
     else if (!WIFEXITED (status) || WEXITSTATUS (status) != 0) {
       guestfs_int_external_command_failed (g, status, g->hv, NULL);
       ret = -1;
     }
   }
-  if (data->recoverypid > 0) waitpid (data->recoverypid, NULL, 0);
+  if (data->recoverypid > 0) guestfs_int_waitpid_noerror
(data->recoverypid);
 
   data->pid = data->recoverypid = 0;
 
diff --git a/src/launch-uml.c b/src/launch-uml.c
index 318081c..8c48795 100644
--- a/src/launch-uml.c
+++ b/src/launch-uml.c
@@ -26,7 +26,6 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/socket.h>
-#include <sys/wait.h>
 #include <sys/signal.h>
 #include <libintl.h>
 
@@ -470,8 +469,8 @@ launch_uml (guestfs_h *g, void *datav, const char *arg)
     close (dsv[0]);
   if (data->pid > 0) kill (data->pid, SIGKILL);
   if (data->recoverypid > 0) kill (data->recoverypid, SIGKILL);
-  if (data->pid > 0) waitpid (data->pid, NULL, 0);
-  if (data->recoverypid > 0) waitpid (data->recoverypid, NULL, 0);
+  if (data->pid > 0) guestfs_int_waitpid_noerror (data->pid);
+  if (data->recoverypid > 0) guestfs_int_waitpid_noerror
(data->recoverypid);
   data->pid = 0;
   data->recoverypid = 0;
   memset (&g->launch_t, 0, sizeof g->launch_t);
@@ -535,10 +534,8 @@ shutdown_uml (guestfs_h *g, void *datav, int
check_for_errors)
 
   /* Wait for subprocess(es) to exit. */
   if (data->pid > 0) {
-    if (waitpid (data->pid, &status, 0) == -1) {
-      perrorf (g, "waitpid (vmlinux)");
+    if (guestfs_int_waitpid (g, data->pid, &status, "vmlinux")
== -1)
       ret = -1;
-    }
     /* Note it's normal for the pre-3.11 vmlinux process to exit with
      * status "killed by signal 15" (where 15 == SIGTERM).  Post 3.11
      * the exit status can normally be 1.
@@ -552,7 +549,7 @@ shutdown_uml (guestfs_h *g, void *datav, int
check_for_errors)
       ret = -1;
     }
   }
-  if (data->recoverypid > 0) waitpid (data->recoverypid, NULL, 0);
+  if (data->recoverypid > 0) guestfs_int_waitpid_noerror
(data->recoverypid);
 
   data->pid = data->recoverypid = 0;
 
diff --git a/src/umask.c b/src/umask.c
index 834ef43..b8748e8 100644
--- a/src/umask.c
+++ b/src/umask.c
@@ -29,7 +29,6 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <sys/wait.h>
 
 #include "ignore-value.h"
 
@@ -91,18 +90,13 @@ guestfs_int_getumask (guestfs_h *g)
   if (read (fd[0], &mask, sizeof mask) != sizeof mask) {
     perrorf (g, "read");
     close (fd[0]);
-    while (waitpid (pid, NULL, 0) == -1 && errno == EINTR)
-      ;
+    guestfs_int_waitpid_noerror (pid);
     return -1;
   }
   close (fd[0]);
 
- again:
-  if (waitpid (pid, &status, 0) == -1) {
-    if (errno == EINTR) goto again;
-    perrorf (g, "waitpid");
+  if (guestfs_int_waitpid (g, pid, &status, "umask") == -1)
     return -1;
-  }
   else if (!WIFEXITED (status) || WEXITSTATUS (status) != 0) {
     guestfs_int_external_command_failed (g, status, "umask", NULL);
     return -1;
diff --git a/src/wait.c b/src/wait.c
new file mode 100644
index 0000000..bdfb0b9
--- /dev/null
+++ b/src/wait.c
@@ -0,0 +1,67 @@
+/* libguestfs
+ * Copyright (C) 2016 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+
+/**
+ * A safe version of L<waitpid(3)> which retries if C<EINTR> is
+ * returned.
+ *
+ * I<Note:> this only needs to be used in the library, or in programs
+ * that install a non-restartable C<SIGCHLD> handler (which is not the
+ * case for any current libguestfs virt tools).
+ *
+ * If the main program installs a SIGCHLD handler and sets it to be
+ * non-restartable, then what can happen is the library is waiting in
+ * a wait syscall, the child exits, C<SIGCHLD> is sent to the process,
+ * and the wait syscall returns C<EINTR>.  Since the library cannot
+ * control the signal handler, we have to instead restart the wait
+ * syscall, which is the purpose of this wrapper.
+ */
+int
+guestfs_int_waitpid (guestfs_h *g, pid_t pid, int *status, const char *errmsg)
+{
+ again:
+  if (waitpid (pid, status, 0) == -1) {
+    if (errno == EINTR)
+      goto again;
+    perrorf (g, "%s: waitpid", errmsg);
+    return -1;
+  }
+  return 0;
+}
+
+/**
+ * Like C<guestfs_int_waitpid>, but ignore errors.
+ */
+void
+guestfs_int_waitpid_noerror (pid_t pid)
+{
+  while (waitpid (pid, NULL, 0) == -1 && errno == EINTR)
+    ;
+}
-- 
2.7.4

On 04/14/2016 11:26 AM, Richard W.M. Jones wrote:
> Thanks: Eric Blake.
> ---
>  src/Makefile.am        |  1 +
>  src/command.c          |  7 ++----
>  src/guestfs-internal.h |  4 +++
>  src/launch-direct.c    | 11 +++------
>  src/launch-uml.c       | 11 +++------
>  src/umask.c            | 10 ++------
>  src/wait.c             | 67
++++++++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 84 insertions(+), 27 deletions(-)
>  create mode 100644 src/wait.c
I haven't hung around libguestfs enough to know your standards for
declaring something reviewed, but looks good to me.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org