[Libguestfs] [PATCH v3 libguestfs] launch: Implement a safer getumask.
Richard W.M. Jones
rjones at redhat.com
Thu Apr 14 13:57:09 UTC 2016
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/
-------------- next part --------------
>From afcd32134e4cf2bae07a7489ce4dca9e5fa037d7 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones at redhat.com>
Date: Wed, 13 Apr 2016 22:28:31 +0100
Subject: [PATCH] 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 | 111 +++++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 121 insertions(+), 37 deletions(-)
create mode 100644 src/umask.c
diff --git a/docs/C_SOURCE_FILES b/docs/C_SOURCE_FILES
index 731df76..6f6002c 100644
--- a/docs/C_SOURCE_FILES
+++ b/docs/C_SOURCE_FILES
@@ -261,6 +261,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..d44fc86
--- /dev/null
+++ b/src/umask.c
@@ -0,0 +1,111 @@
+/* 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]);
+ waitpid (pid, NULL, 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
More information about the Libguestfs
mailing list