Re: [Qemu-devel] [PATCH 2/5] 9pfs: local: resolve special directories in paths

[Greg Kurz]

On Fri, 5 May 2017 11:59:15 -0500
Eric Blake <address@hidden> wrote:

> On 05/05/2017 09:37 AM, Greg Kurz wrote:
> > When using the mapped-file security mode, the creds of a path /foo/bar
> > are stored in the /foo/.virtfs_metadata/bar file. This is okay for all
> > paths unless they end with '.' or '..', because we cannot create the
> > corresponding file in the metadata directory.
> > 
> > This patch ensures that '.' and '..' are resolved in all paths.
> > 
> > The core code only passes path elements (no '/') to the backend, with
> > the notable exception of the '/' path, which refers to the virtfs root.
> > This patch preserve the current behavior of converting it to '.' so  
> 
> s/preserve/preserves/
> 

I'll fix this.

> > that it can be passed to "*at()" syscalls ('/' would mean the host root).
> > 
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  hw/9pfs/9p-local.c |   30 +++++++++++++++++++++++-------
> >  1 file changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index f3ebca4f7a56..92262f3c3e37 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -1097,14 +1097,30 @@ static int local_name_to_path(FsContext *ctx, 
> > V9fsPath *dir_path,
> >                                const char *name, V9fsPath *target)
> >  {
> >      if (dir_path) {
> > -        v9fs_path_sprintf(target, "%s/%s", dir_path->data, name);
> > -    } else if (strcmp(name, "/")) {
> > -        v9fs_path_sprintf(target, "%s", name);
> > +        if (!strcmp(name, ".")) {
> > +            /* "." relative to "foo/bar" is "foo/bar" */
> > +            v9fs_path_copy(target, dir_path);
> > +        } else if (!strcmp(name, "..")) {
> > +            if (!strcmp(dir_path->data, ".")) {
> > +                /* ".." relative to the root is "." */
> > +                v9fs_path_sprintf(target, ".");
> > +            } else {
> > +                char *tmp = g_path_get_dirname(dir_path->data);
> > +                /* ".." relative to "foo/bar" is equivalent to "foo" */  
> 
> True only if bar is not a symlink to some other directory.  What
> guarantees do you have that you are not going to be inadvertently
> skipping a traversal through symlinks and thereby picking the wrong
> location for '..'?
> 

My understanding is that symlinks are supposed to be resolved by the client,
and we shouldn't follow them in the server.

> > +                v9fs_path_sprintf(target, "%s", tmp);
> > +                g_free(tmp);
> > +            }
> > +        } else {
> > +            assert(!strchr(name, '/'));
> > +            v9fs_path_sprintf(target, "%s/%s", dir_path->data, name);
> > +        }
> > +    } else if (!strcmp(name, "/") || !strcmp(name, ".") ||
> > +               !strcmp(name, "..")) {
> > +            /* This is the root fid */
> > +        v9fs_path_sprintf(target, ".");
> >      } else {
> > -        /* We want the path of the export root to be relative, otherwise
> > -         * "*at()" syscalls would treat it as "/" in the host.
> > -         */
> > -        v9fs_path_sprintf(target, "%s", ".");
> > +        assert(!strchr(name, '/'));
> > +        v9fs_path_sprintf(target, "./%s", name);
> >      }
> >      return 0;
> >  }
> > 
> >   
>

pgpvauVnmFsEO.pgp
Description: OpenPGP digital signature