[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/
On 04/14/2016 07:57 AM, Richard W.M. Jones wrote:
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.
But without looping on EINTR... + + /* Read the umask. */ + if (read (fd[0], &mask, sizeof mask) != sizeof mask) { + perrorf (g, "read"); + close (fd[0]); + waitpid (pid, NULL, 0); I think it's enough to make this line: while (waitpid (pid, &status, 0) == -1 && errno == EINTR); + return -1; as you've already reported the initial error, and are merely focused on cleanup of the child. + } + close (fd[0]); + + again: + if (waitpid (pid, &status, 0) == -1) { + if (errno == EINTR) goto again; + perrorf (g, "waitpid"); + return -1; Anything else, like trying to reuse the again: loop, seems like it be more complicated control flow. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Apr 14, 2016 at 08:04:39AM -0600, Eric Blake wrote:
On 04/14/2016 07:57 AM, Richard W.M. Jones wrote:
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.
But without looping on EINTR...
Thanks - I pushed it with your suggested loop added. BTW we don't loop in waitpid/EINTR anywhere else in libguestfs, but I guess this is something we should be doing. Can you tell us why it's necessary? Rich.
+ + /* Read the umask. */ + if (read (fd[0], &mask, sizeof mask) != sizeof mask) { + perrorf (g, "read"); + close (fd[0]); + waitpid (pid, NULL, 0);
I think it's enough to make this line:
while (waitpid (pid, &status, 0) == -1 && errno == EINTR);
+ return -1;
as you've already reported the initial error, and are merely focused on cleanup of the child.
+ } + close (fd[0]); + + again: + if (waitpid (pid, &status, 0) == -1) { + if (errno == EINTR) goto again; + perrorf (g, "waitpid"); + return -1;
Anything else, like trying to reuse the again: loop, seems like it be more complicated control flow.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On 04/14/2016 08:11 AM, Richard W.M. Jones wrote:
On Thu, Apr 14, 2016 at 08:04:39AM -0600, Eric Blake wrote:
On 04/14/2016 07:57 AM, Richard W.M. Jones wrote:
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.
But without looping on EINTR...
Thanks - I pushed it with your suggested loop added.
BTW we don't loop in waitpid/EINTR anywhere else in libguestfs, but I guess this is something we should be doing. Can you tell us why it's necessary?
Depending on what else the program linking with your library has done, waitpid() has some scenarios where it is allowed to fail with EINTR (off the top of my head, it has to do with whether a SIGCHLD handler was installed with SA_RESTART or not). When you are not in a library context, you can control things so that system calls never fail with EINTR (the OS does the retry for you); but in a library, you can't assume that context. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
BTW this causes valgrind to fail in a particularly strange way. I filed a bug upstream with a minimal reproducer. Do you have any ideas? https://bugs.kde.org/show_bug.cgi?id=361810 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On 04/15/2016 07:22 AM, Richard W.M. Jones wrote:
BTW this causes valgrind to fail in a particularly strange way. I filed a bug upstream with a minimal reproducer. Do you have any ideas?
Weird - it looks like valgrind is somehow calling lseek on stdin, wiping out the buffer that it previously read. Since it doesn't happen when the program is run standalone, I'm thinking this is a valgrind bug. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones