Libguestfs - Apr 2016 - Re: [PATCH v3 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.
---
 docs/C_SOURCE_FILES    |   1 +
 po/POTFILES            |   1 +
 src/Makefile.am        |   1 +
 src/guestfs-internal.h |   3 ++
 src/launch.c           |  41 ++----------------
 src/umask.c            | 110 +++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 120 insertions(+), 37 deletions(-)
 create mode 100644 src/umask.c

diff --git a/docs/C_SOURCE_FILES b/docs/C_SOURCE_FILES
index a477744..f828911 100644
--- a/docs/C_SOURCE_FILES
+++ b/docs/C_SOURCE_FILES
@@ -269,6 +269,7 @@ src/structs-free.c
 src/structs-print.c
 src/test-utils.c
 src/tmpdirs.c
+src/umask.c
 src/utils.c
 src/whole-file.c
 test-tool/test-tool.c
diff --git a/po/POTFILES b/po/POTFILES
index 9e4d9cc..23599b9 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -359,6 +359,7 @@ src/structs-free.c
 src/structs-print.c
 src/test-utils.c
 src/tmpdirs.c
+src/umask.c
 src/utils.c
 src/whole-file.c
 test-tool/test-tool.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..3fc7bc6
--- /dev/null
+++ b/src/umask.c
@@ -0,0 +1,110 @@
+/* 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 <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;
+
+  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]);
+
+    /* umask can't fail. */
+    mask = umask (0);
+
+    if (write (fd[1], &mask, sizeof mask) != sizeof mask)
+      _exit (EXIT_FAILURE);
+    if (close (fd[1]) == -1)
+      _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/14/2016 03:59 AM, 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).
> 
> 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.
> ---
> +guestfs_int_getumask (guestfs_h *g)
> +{
> +  if (pid == 0) {
> +    /* The child process must ONLY call async-safe functions. */
> +    close (fd[0]);
> +
> +    /* umask can't fail. */
> +    mask = umask (0);
> +
> +    if (write (fd[1], &mask, sizeof mask) != sizeof mask)
> +      _exit (EXIT_FAILURE);
> +    if (close (fd[1]) == -1)
> +      _exit (EXIT_FAILURE);
> +
> +    _exit (EXIT_SUCCESS);
Yay - the child looks good now.
> +  }
> +
> +  /* Parent. */
> +  close (fd[1]);
> +
> +  /* Read the umask. */
> +  if (read (fd[0], &mask, sizeof mask) != sizeof mask) {
> +    perrorf (g, "read");
> +    close (fd[0]);
> +    return -1;
Oops - this strands a child process.  You have to reap the child, even
if the read() failed.
> +  }
> +  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;
> +  }
Getting closer.

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

On Thu, Apr 14, 2016 at 07:38:23AM -0600, Eric Blake
wrote:
> > +  /* Read the umask. */
> > +  if (read (fd[0], &mask, sizeof mask) != sizeof mask) {
> > +    perrorf (g, "read");
> > +    close (fd[0]);
> > +    return -1;
> 
> Oops - this strands a child process.  You have to reap the child, even
> if the read() failed.
Bleah that was stupid.  Try attached version - the only difference is
I added a waitpid call to the error path above.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/