[Libguestfs] [PATCH libguestfs] launch: Implement a safer getumask.
Eric Blake
eblake at redhat.com
Wed Apr 13 21:53:27 UTC 2016
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20160413/55571f2c/attachment.sig>
More information about the Libguestfs
mailing list