Re: [Qemu-devel] [PATCH] 9pfs: disallow / in path components

[Greg Kurz]

On Wed, 24 Aug 2016 20:23:22 +0100
Peter Maydell <address@hidden> wrote:

> On 24 August 2016 at 17:40, Greg Kurz <address@hidden> wrote:
> > On Wed, 24 Aug 2016 16:00:24 +0100
> > Peter Maydell <address@hidden> wrote:  
> >> Do we also need ".." and "." to be illegal names (for at least most
> >> operations)?  
> 
> > I understand how ".." could be an issue, but I don't for "."... can you
> > please elaborate ?  
> 
> If you try to create, open, etc a file named "." then it won't create a
> file named "."; it'll do something wrong instead. So we shouldn't
> permit attempts to treat "." as an ordinary filename.

FWIW, on UNIX filesystems, "." cannot be created, nor deleted, nor symlinked
to:

mkdir(".", 0777)                        = -1 EEXIST (File exists)
open(".", O_WRONLY|O_CREAT|O_TRUNC, 0666) = -1 EISDIR (Is a directory)
rmdir(".")                              = -1 EINVAL (Invalid argument)
symlink("foo", ".")                      = -1 EEXIST (File exists)

I get the very same behaviour with the 9pfs local backend in QEMU (using
the linux client and turning strings into "." with GDB). So I still don't
get what "something wrong" refers to... but I'm not opposed to handling
"." and ".." differently (see below).

> 
> (Basically the rationale is that for Linux the constraints on
> file and pathnames are only
>  * no NULs

I have a separate patch for this as the spec forbids NUL for all strings,
not only file and pathnames, as said in http://man.cat-v.org/plan_9/5/intro.

>  * no /
>  * "." is special
>  * ".." is special

Indeed, that's what http://man.cat-v.org/plan_9/5/open says about the
create request:

The names . and .. are special; it is illegal to create files with these
names.

I hence agree these should be checked in the middle layer for create and
lcreate. And even if they are not described in the spec, it probably also
makes sense for all other operations except walk.

I'll make it a separate patch since it is conceptually different than
blacklisting illegal characters like /, which applies to all requests,
including walk.

>  * can't be the empty string

I could not find it in the spec but it makes sense indeed. Maybe this
should be generalized as I could not find a valid use of the empty
string in the 9P spec.

> We should reflect that in our error checking.)
> 
> > The spec says:
> >
> >           Directories are created by create with DMDIR set in the per-
> >           missions argument (see stat(5)). The members of a directory
> >           can be found with read(5). All directories must support
> >           walks to the directory .. (dot-dot) meaning parent direc-
> >           tory, although by convention directories contain no explicit
> >           entry for .. or . (dot).  The parent of the root directory
> >           of a server's tree is itself.  
> 
> Yes, walk is the one special case that needs to handle "." and ".."
> (because for this operation they have a defined meaning that doesn't
> mean "just pass them through to the libc functions").
> 
> > So I don't think we should boldly make ".." an illegal name, but
> > rather ignore it. Pretty much like doing chdir("..") when the current
> > directory is /.
> >
> > All operations except walk take an existing fid and a single path component.
> > A possible fix would be to convert ".." to "" when the fid points to the 
> > root
> > of the export path. Makes sense ?  
> 
> What did you want the empty string to mean? (We should probably
> also define the empty string as an illegal name).
> 

You can ignore this remark since ".." and "." will be specifically ignored for
all operations except walk.

> thanks
> -- PMM

Thanks for your valuable help, Peter !

I'll try to come up with a new series ASAP. But since I'm still on holidays 
until
the end of the week, don't feel forced to hold 2.7 for this if you don't hear 
from
me.

Cheers.

--
Greg