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

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2864 bytes --]

On Mon, 27 Feb 2017 17:37:56 -0600
Eric Blake <eblake@redhat.com> wrote:

> 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).
> 

Yes but I now realize I have another nit... Patch 2/31 should have

From: Pradeep <pradeepkiruvale@gmail.com>

but for unknown reasons, it got dropped at some point, and cannot be
fixed in a followup patch.

For simplicity, I guess I'd rather fix all the issues and respin a new
pull tomorrow.

> > Suggested-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > (renamed openat_nofollow() to relative_openat_nofollow(),
> >  assert path is relative, Greg Kurz)
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---  
> 
> > +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().  :(
> 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]