[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