[libvirt] [PATCH v5 2/5] util: Create virFileAccessibleAs function

[Eric Blake]

On 10/21/2011 07:12 AM, Michal Privoznik wrote:
> This function checks if a given path is accessible under
> given uid and gid.
> ---
>   src/util/util.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   src/util/util.h |    3 ++
>   2 files changed, 86 insertions(+), 0 deletions(-)
>
> diff --git a/src/util/util.c b/src/util/util.c
> index af27344..7343485 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -671,6 +671,77 @@ virFileIsExecutable(const char *file)
>   }
>
>   #ifndef WIN32
> +/* Check that a file is accessible under certain
> + * user&  gid.
> + * @mode can be F_OK, or a bitwise combination of R_OK, W_OK, and X_OK.
> + * see 'man access' for more details.
> + * Returns 0 on success, -errno on fail,
> + *>0 on signaled child.

That's awkward.  How about just stating:

Returns 0 on success, -1 on fail with errno set.

> + */
> +int
> +virFileAccessibleAs(const char *path, int mode,
> +                    uid_t uid, gid_t gid)
> +{
> +    pid_t pid = 0;
> +    int waitret, status, ret = 0;
> +    int forkRet = 0;
> +    bool do_fork = (uid != getuid() || gid != getgid());
> +
> +    if (do_fork)
> +        forkRet = virFork(&pid);

The control flow is a bit hard to follow.  While it looks correct, I 
think it would be easier to read as follows (it also matches my proposal 
to return -1 with errno set, when encountering error):

if (uid == getuid() && gid == getgid())
     return access(path, mode);

forkRet = virFork(&pid);

... code without reference to do_fork ...

> +    if (pid) { /* parent */
> +        do {
> +            if ((waitret = waitpid(pid,&status, 0))<  0) {
> +                ret = -errno;
> +                virKillProcess(pid, SIGKILL);
> +                return ret;
> +            }
> +        } while (!WIFEXITED(status)&&  !WIFSIGNALED(status));

Use virPidWait() (from command.h), not this hand-rolled do/while loop. 
In particular, your loop will only ever execute exactly once, since 
status will always satisfy either WIFEXITED or WIFSIGNALED if waitpid() 
was successful; and you fail to handle EINTR as a reason to retry waitpid.

Returning a positive value for a signaled child is awkward; I would just 
treat that as failure, by setting errno to EIO and returning -1.  The 
likelihood of the child dying by signal is super-slim, not to mention 
that I don't see how the client can behave any better by knowing that 
the child process died than if it just reacted as if access were denied 
in the first place.

> @@ -993,6 +1064,18 @@ childerror:
>
>   #else /* WIN32 */
>
> +int
> +virFileAccessibleAs(const char *path ATTRIBUTE_UNUSED,
> +                    int mode ATTRIBUTE_UNUSED,
> +                    uid_t uid ATTRIBUTE_UNUSED,
> +                    git_t gid ATTRIBUTE_UNUSED)
> +{
> +    virUtilError(VIR_ERR_INTERNAL_ERROR,
> +                 "%s", _("virFileAccessibleAs is not implemented for WIN32"));
> +
> +    return -ENOSYS;

Rather than failing with -ENOSYS, can we at least try access(path, 
mode), ignoring uid and gid?  It may have false negatives (fail in cases 
where a different uid would succeed), but is more likely to be useful 
than just blindly declaring failure.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org