[libvirt] [PATCH v2 1/2] virfile: Adjust error path for virFileOpenForked

[Michal Privoznik]

On 30.01.2015 18:52, John Ferlan wrote:
> Rather than have a dummy waitpid loop and return of the failure status
> from recvfd, adjust the logic to save the recvfd error & fd and then
> in priority order:
> 
> - if waitpid failed, use that errno value
> - waitpid succeeded, but if the child exited abnormally, report failure
> (use EACCES to report as return failure, since either EACCES or EPERM is
> what caused us to fall into the fork+setuid path)
> - waitpid succeeded, but if the child reported non-zero status, report
> failure (use the errno value that the child encoded into exit status)
> - waitpid succeeded, but if recvfd failed, report recvfd_errno
> - waitpid and recvfd succeeded, use the fd
> 
> NOTE: Original logic to retry the open and force owner mode was
> "documented" as only being attempted if we had already tried opening
> with the fork+setuid, but checked flags vs. VIR_FILE_OPEN_NOFORK which
> is counter to how we would get to that point. So that code was removed.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/util/virfile.c | 65 +++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 4024f3d..e6e13bc 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2034,6 +2034,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
>  {
>      pid_t pid;
>      int waitret, status, ret = 0;
> +    int recvfd_errno;
>      int fd = -1;
>      int pair[2] = { -1, -1 };
>      gid_t *groups;
> @@ -2124,15 +2125,11 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
>          fd = recvfd(pair[0], 0);
>      } while (fd < 0 && errno == EINTR);
>      VIR_FORCE_CLOSE(pair[0]); /* NB: this preserves errno */
> +    recvfd_errno = errno;

>From recvfd() source, it seems like errno is set iff an error occurred.
Upon success, errno is left untouched. Therefore we may set recvfd_errno
to 'random' value. Therefore I think you should set recvfd_errno iff fd
< 0. If you do this, you need to initialize recvfd_errno too. This may
look like overkill, since there is no visible path, how an errno could
be set, and the control could still reach this point. But rather be safe
than sorry.
>  
> -    /* gnulib will return ENOTCONN in certain instances */
> -    if (fd < 0 && !(errno == EACCES || errno == ENOTCONN)) {
> -        ret = -errno;
> -        while (waitpid(pid, NULL, 0) == -1 && errno == EINTR);
> -        return ret;
> -    }
> -
> -    /* wait for child to complete, and retrieve its exit code */
> +    /* wait for child to complete, and retrieve its exit code
> +     * if waitpid fails, use that status
> +     */
>      while ((waitret = waitpid(pid, &status, 0)) == -1 && errno == EINTR);
>      if (waitret == -1) {
>          ret = -errno;
> @@ -2142,28 +2139,40 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
>          VIR_FORCE_CLOSE(fd);
>          return ret;
>      }
> -    if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES ||
> -        fd == -1) {
> -        /* fall back to the simpler method, which works better in
> -         * some cases */
> +
> +    /*
> +     * If waitpid succeeded, but if the child exited abnormally or
> +     * reported non-zero status, report failure.
> +     */
> +    if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
> +        char *msg = virProcessTranslateStatus(status);
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("child failed to create '%s': %s"),
> +                       path, msg);
>          VIR_FORCE_CLOSE(fd);
> -        if (flags & VIR_FILE_OPEN_NOFORK) {
> -            /* If we had already tried opening w/o fork+setuid and
> -             * failed, no sense trying again. Just set return the
> -             * original errno that we got at that time (by
> -             * definition, always either EACCES or EPERM - EACCES
> -             * is close enough).
> -             */
> -            return -EACCES;
> -        }
> -        if ((fd = open(path, openflags, mode)) < 0)
> -            return -errno;
> -        ret = virFileOpenForceOwnerMode(path, fd, mode, uid, gid, flags);
> -        if (ret < 0) {
> -            VIR_FORCE_CLOSE(fd);
> -            return ret;
> -        }
> +        VIR_FREE(msg);
> +        /* Use child exit status if possible; otherwise,
> +         * just use -EACCES, since by our original failure in
> +         * the non fork+setuid path would have been EACCES or
> +         * EPERM by definition, but EACCES is close enough.
> +         * Besides -EPERM is like returning fd == -1.
> +         */
> +        if (WIFEXITED(status))
> +            ret = -WEXITSTATUS(status);
> +        else
> +            ret = -EACCES;
> +        return ret;
>      }
> +
> +    /* if waitpid succeeded, but recvfd failed, report recvfd_errno */
> +    if (recvfd_errno != 0) {
> +        virReportSystemError(recvfd_errno,
> +                             _("failed recvfd for child creating '%s'"),
> +                             path);
> +        return -recvfd_errno;
> +    }
> +
> +    /* otherwise, waitpid and recvfd succeeded, return the fd */
>      return fd;
>  }
>  
> 

ACK with that fixed.

Michal