[Libguestfs] [PATCH v2 libguestfs] launch: Implement a safer getumask.

[Eric Blake]

On 04/13/2016 04:14 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).
> 

> +int
> +guestfs_int_getumask (guestfs_h *g)
> +{
> +  pid_t pid;
> +  int fd[2], r;
> +  int mask;
> +  int status;
> +  const char *str, *err;
> +
> +  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) {

Bzzt.  umask() can't fail, and has no reserved bit pattern for failure.
 errno is undefined after umask().

> +      str = "umask: ";
> +      err = strerror (errno);

Bzzt. strerror() is not async-signal safe (it can malloc() when passed
certain errno values, plus it has to return localized results and who
knows what that might entail).  And did I mention it is undefined at
this point?

> +      ignore_value (write (2, str, strlen (str)));
> +      ignore_value (write (2, err, strlen (err)));
> +      _exit (EXIT_FAILURE);
> +    }
> +
> +    if (write (fd[1], &mask, sizeof mask) != sizeof mask) {
> +      str = "write: ";
> +      err = strerror (errno);

Bzzt. The safest way to do strerror() is in the context of the parent.
All the more the child can do is write something that the parent then
knows to decode, like the value of errno.

But why bother?  Since umask() can't fail, the ONLY thing that can fail
is write().  If you can't write() umask to the parent, what makes you
think you can write() the error message to the parent?

I think the exit status is good enough - either you successfully write
the umask to the parent and exit with status 0, or you fail and exit
with status non-zero.  Yeah, you've lost the ability to report the errno
of why the write() failed, but what are you really losing in that
EXTREMELY unlikely scenario?


-- 
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/3f3cdec5/attachment.sig>