Re: [Qemu-devel] [PULL 07/31] 9pfs: introduce relative_openat_nofollow() helper

[Eric Blake]

On 02/27/2017 04:59 PM, Greg Kurz wrote:
> When using the passthrough security mode, symbolic links created by the
> guest are actual symbolic links on the host file system.
> 

Hmm, I just barely started reviewing the series, and see a pull request.
At this point, anything I point out can probably be done as followup
patches rather than forcing a respin of the pull (and soft freeze is
appropriate for that).

> Suggested-by: Jann Horn <address@hidden>
> Signed-off-by: Greg Kurz <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> (renamed openat_nofollow() to relative_openat_nofollow(),
>  assert path is relative, Greg Kurz)
> Signed-off-by: Greg Kurz <address@hidden>
> ---

> +int relative_openat_nofollow(int dirfd, const char *path, int flags,
> +                             mode_t mode)
> +{
> +    int fd;
> +
> +    assert(path[0] != '/');

If you move this assert...

> +
> +    fd = dup(dirfd);
> +    if (fd == -1) {
> +        return -1;
> +    }
> +
> +    while (*path) {
> +        const char *c;
> +        int next_fd;
> +        char *head;

...here, you can make sure there are no 'a//b' issues to worry about.

> +
> +        head = g_strdup(path);
> +        c = strchr(path, '/');
> +        if (c) {
> +            head[c - path] = 0;
> +            next_fd = openat_dir(fd, head);
> +        } else {
> +            next_fd = openat_file(fd, head, flags, mode);
> +        }
> +        g_free(head);
> +        if (next_fd == -1) {
> +            close_preserve_errno(fd);
> +            return -1;
> +        }
> +        close(fd);
> +        fd = next_fd;
> +
> +        if (!c) {
> +            break;
> +        }
> +        path = c + 1;

or else add an assert here.


> +static inline int openat_file(int dirfd, const char *name, int flags,
> +                              mode_t mode)
> +{
> +    int fd, serrno;
> +
> +    fd = openat(dirfd, name, flags | O_NOFOLLOW | O_NOCTTY | O_NONBLOCK,
> +                mode);
> +    if (fd == -1) {
> +        return -1;
> +    }
> +
> +    serrno = errno;
> +    /* O_NONBLOCK was only needed to open the file. Let's drop it. */
> +    assert(!fcntl(fd, F_SETFL, flags));

Ewww. Side effect inside an assert().  :(

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature