Libguestfs - Apr 2016 - [PATCH v2 libguestfs] launch: Implement a safer getumask.

The current implementation of getumask involves writing a file with
mode 0777 and then testing what mode was created by the kernel.  This
doesn't work properly if the user set a per-mount umask (or fmask/
dmask).

This alternative method was suggested by Josh Stone.  By forking, we
can use the thread-unsafe method (calling umask) and pass the result
back over a pipe.

This change also fixes another problem: mode_t is unsigned, so cannot
be used to return an error indication (ie. -1).  Return a plain int
instead.

Thanks: Josh Stone, Jiri Jaburek, Eric Blake.
---
 src/Makefile.am        |   1 +
 src/guestfs-internal.h |   3 ++
 src/launch.c           |  41 ++--------------
 src/umask.c            | 128 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+), 37 deletions(-)
 create mode 100644 src/umask.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 3b4cd10..34c4fa6 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -130,6 +130,7 @@ libguestfs_la_SOURCES = \
 	structs-copy.c \
 	structs-free.c \
 	tmpdirs.c \
+	umask.c \
 	whole-file.c \
 	libguestfs.syms
 
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 61f384c..bf107d0 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -915,4 +915,7 @@ void guestfs_int_init_unix_backend (void)
__attribute__((constructor));
 /* guid.c */
 extern int guestfs_int_validate_guid (const char *);
 
+/* umask.c */
+extern int guestfs_int_getumask (guestfs_h *g);
+
 #endif /* GUESTFS_INTERNAL_H_ */
diff --git a/src/launch.c b/src/launch.c
index a4326fe..460ed29 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -50,8 +50,6 @@ static struct backend {
   const struct backend_ops *ops;
 } *backends = NULL;
 
-static mode_t get_umask (guestfs_h *g);
-
 int
 guestfs_impl_launch (guestfs_h *g)
 {
@@ -75,6 +73,7 @@ guestfs_impl_launch (guestfs_h *g)
       guestfs_version (g);
     struct backend *b;
     CLEANUP_FREE char *backend = guestfs_get_backend (g);
+    int mask;
 
     debug (g, "launch: program=%s", g->program);
     if (STRNEQ (g->identifier, ""))
@@ -87,7 +86,9 @@ guestfs_impl_launch (guestfs_h *g)
     debug (g, "launch: backend=%s", backend);
 
     debug (g, "launch: tmpdir=%s", g->tmpdir);
-    debug (g, "launch: umask=0%03o", get_umask (g));
+    mask = guestfs_int_getumask (g);
+    if (mask >= 0)
+      debug (g, "launch: umask=0%03o", (unsigned) mask);
     debug (g, "launch: euid=%ju", (uintmax_t) geteuid ());
   }
 
@@ -475,40 +476,6 @@ guestfs_int_create_socketname (guestfs_h *g, const char
*filename,
 }
 
 /**
- * glibc documents, but does not actually implement, a L<getumask(3)>
- * call.
- *
- * This function implements a thread-safe way to get the umask.  Note
- * this is only called when C<g-E<gt>verbose> is true and after
- * C<g-E<gt>tmpdir> has been created.
- */
-static mode_t
-get_umask (guestfs_h *g)
-{
-  mode_t ret;
-  int fd;
-  struct stat statbuf;
-  CLEANUP_FREE char *filename = safe_asprintf (g, "%s/umask-check",
g->tmpdir);
-
-  fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY|O_CLOEXEC, 0777);
-  if (fd == -1)
-    return -1;
-
-  if (fstat (fd, &statbuf) == -1) {
-    close (fd);
-    return -1;
-  }
-
-  close (fd);
-
-  ret = statbuf.st_mode;
-  ret &= 0777;
-  ret = ret ^ 0777;
-
-  return ret;
-}
-
-/**
  * When the library is loaded, each backend calls this function to
  * register itself in a global list.
  */
diff --git a/src/umask.c b/src/umask.c
new file mode 100644
index 0000000..49c5a4b
--- /dev/null
+++ b/src/umask.c
@@ -0,0 +1,128 @@
+/* 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
+ */
+
+/**
+ * Return current umask in a thread-safe way.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include "ignore-value.h"
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+
+/**
+ * glibc documents, but does not actually implement, a L<getumask(3)>
+ * call.
+ *
+ * This function implements an expensive, but thread-safe way to get
+ * the current process's umask.
+ *
+ * Returns the current process's umask.  On failure, returns C<-1>
and
+ * sets the error in the guestfs handle.
+ *
+ * Thanks to: Josh Stone, Jiri Jaburek, Eric Blake.
+ */
+int
+guestfs_int_getumask (guestfs_h *g)
+{
+  pid_t pid;
+  int fd[2], r;
+  int mask;
+  int status;
+  const char *str, *err;
+
+  r = pipe2 (fd, O_CLOEXEC);
+  if (r == -1) {
+    perrorf (g, "pipe2");
+    return -1;
+  }
+
+  pid = fork ();
+  if (pid == -1) {
+    perrorf (g, "fork");
+    close (fd[0]);
+    close (fd[1]);
+    return -1;
+  }
+  if (pid == 0) {
+    /* The child process must ONLY call async-safe functions. */
+    close (fd[0]);
+
+    mask = umask (0);
+    if (mask == -1) {
+      str = "umask: ";
+      err = strerror (errno);
+      ignore_value (write (2, str, strlen (str)));
+      ignore_value (write (2, err, strlen (err)));
+      _exit (EXIT_FAILURE);
+    }
+
+    if (write (fd[1], &mask, sizeof mask) != sizeof mask) {
+      str = "write: ";
+      err = strerror (errno);
+      ignore_value (write (2, str, strlen (str)));
+      ignore_value (write (2, err, strlen (err)));
+      _exit (EXIT_FAILURE);
+    }
+    if (close (fd[1]) == -1) {
+      str = "close: ";
+      err = strerror (errno);
+      ignore_value (write (2, str, strlen (str)));
+      ignore_value (write (2, err, strlen (err)));
+      _exit (EXIT_FAILURE);
+    }
+
+    _exit (EXIT_SUCCESS);
+  }
+
+  /* Parent. */
+  close (fd[1]);
+
+  /* Read the umask. */
+  if (read (fd[0], &mask, sizeof mask) != sizeof mask) {
+    perrorf (g, "read");
+    close (fd[0]);
+    return -1;
+  }
+  close (fd[0]);
+
+ again:
+  if (waitpid (pid, &status, 0) == -1) {
+    if (errno == EINTR) goto again;
+    perrorf (g, "waitpid");
+    return -1;
+  }
+  else if (!WIFEXITED (status) || WEXITSTATUS (status) != 0) {
+    guestfs_int_external_command_failed (g, status, "umask", NULL);
+    return -1;
+  }
+
+  return mask;
+}
-- 
2.7.4

On 04/13/2016 04:14 PM, Richard W.M. Jones wrote:
> The current implementation of getumask involves writing a file with
> mode 0777 and then testing what mode was created by the kernel.  This
> doesn't work properly if the user set a per-mount umask (or fmask/
> dmask).
> 
> +int
> +guestfs_int_getumask (guestfs_h *g)
> +{
> +  pid_t pid;
> +  int fd[2], r;
> +  int mask;
> +  int status;
> +  const char *str, *err;
> +
> +  r = pipe2 (fd, O_CLOEXEC);
> +  if (r == -1) {
> +    perrorf (g, "pipe2");
> +    return -1;
> +  }
> +
> +  pid = fork ();
> +  if (pid == -1) {
> +    perrorf (g, "fork");
> +    close (fd[0]);
> +    close (fd[1]);
> +    return -1;
> +  }
> +  if (pid == 0) {
> +    /* The child process must ONLY call async-safe functions. */
> +    close (fd[0]);
> +
> +    mask = umask (0);
> +    if (mask == -1) {
Bzzt.  umask() can't fail, and has no reserved bit pattern for failure.
 errno is undefined after umask().
> +      str = "umask: ";
> +      err = strerror (errno);
Bzzt. strerror() is not async-signal safe (it can malloc() when passed
certain errno values, plus it has to return localized results and who
knows what that might entail).  And did I mention it is undefined at
this point?
> +      ignore_value (write (2, str, strlen (str)));
> +      ignore_value (write (2, err, strlen (err)));
> +      _exit (EXIT_FAILURE);
> +    }
> +
> +    if (write (fd[1], &mask, sizeof mask) != sizeof mask) {
> +      str = "write: ";
> +      err = strerror (errno);
Bzzt. The safest way to do strerror() is in the context of the parent.
All the more the child can do is write something that the parent then
knows to decode, like the value of errno.

But why bother?  Since umask() can't fail, the ONLY thing that can fail
is write().  If you can't write() umask to the parent, what makes you
think you can write() the error message to the parent?

I think the exit status is good enough - either you successfully write
the umask to the parent and exit with status 0, or you fail and exit
with status non-zero.  Yeah, you've lost the ability to report the errno
of why the write() failed, but what are you really losing in that
EXTREMELY unlikely scenario?


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