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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
