Libguestfs - Apr 2016 - Re: [PATCH 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            | 112 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 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..e01ebbe
--- /dev/null
+++ b/src/umask.c
@@ -0,0 +1,112 @@
+/* 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 <sys/stat.h>
+#include <sys/types.h>
+#include <sys/wait.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]);
+
+    mask = umask (0);
+    if (mask == -1) {
+      perror ("umask");
+      _exit (EXIT_FAILURE);
+    }
+
+    if (write (fd[1], &mask, sizeof mask) != sizeof mask) {
+      perror ("write");
+      _exit (EXIT_FAILURE);
+    }
+    if (close (fd[1]) == -1) {
+      perror ("close");
+      _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]);
+
+  if (waitpid (pid, &status, 0) == -1) {
+    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 03:43 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).
> 
> 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.
> ---
> +/**
> + * 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]);
> +
> +    mask = umask (0);
> +    if (mask == -1) {
> +      perror ("umask");
perror() is NOT async-safe (it manipulates stdio, and could permanently
wedgelock the child if you happened to fork() while some other thread
was also using stdio).  Safe error reporting in a fork()d child is
basically impossible; the best you can do is write() something back to
the parent and let the parent do the error reporting. :(
> +      _exit (EXIT_FAILURE);
> +    }
> +
> +    if (write (fd[1], &mask, sizeof mask) != sizeof mask) {
> +      perror ("write");
> +      _exit (EXIT_FAILURE);
> +    }
> +    if (close (fd[1]) == -1) {
> +      perror ("close");
Again, more bad use of perror.
> +      _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]);
> +
> +  if (waitpid (pid, &status, 0) == -1) {
> +    perrorf (g, "waitpid");
> +    return -1;
> +  }
Doesn't this need to loop on EINTR, rather than immediately giving up?
> +  else if (!WIFEXITED (status) || WEXITSTATUS (status) != 0) {
> +    guestfs_int_external_command_failed (g, status, "umask",
NULL);
> +    return -1;
> +  }
> +
> +  return mask;
> +}
> 
-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

On 04/13/2016 03:53 PM, Eric Blake wrote:
>> +    mask = umask (0);
>> +    if (mask == -1) {
>> +      perror ("umask");
> 
> perror() is NOT async-safe (it manipulates stdio, and could permanently
> wedgelock the child if you happened to fork() while some other thread
> was also using stdio).  Safe error reporting in a fork()d child is
> basically impossible; the best you can do is write() something back to
> the parent and let the parent do the error reporting. :(
The official list is at:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04
at the end of subheading 2.4.3

although you'll want to also consult

http://austingroupbugs.net/view.php?id=692

for several functions (like strlen()) that were accidentally omitted
from the list and will appear when POSIX 2008 TC 2 comes out later this
year.

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

On Wed, Apr 13, 2016 at 03:53:27PM -0600, Eric Blake
wrote:
> > +    if (mask == -1) {
> > +      perror ("umask");
> 
> perror() is NOT async-safe (it manipulates stdio, and could permanently
> wedgelock the child if you happened to fork() while some other thread
> was also using stdio).  Safe error reporting in a fork()d child is
> basically impossible; the best you can do is write() something back to
> the parent and let the parent do the error reporting. :(
Yup, I hate POSIX too.  I'll change these to a write() call.
> > +  if (waitpid (pid, &status, 0) == -1) {
> > +    perrorf (g, "waitpid");
> > +    return -1;
> > +  }
> 
> Doesn't this need to loop on EINTR, rather than immediately giving up?
I think that's a systematic error throughout libguestfs.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html