Re: [Qemu-devel] [PATCH v2 1/5] 9p: forbid illegal path names

[Greg Kurz]

On Fri, 26 Aug 2016 13:33:28 -0500
Eric Blake <address@hidden> wrote:

> On 08/26/2016 10:07 AM, Greg Kurz wrote:
> > Empty path components don't make sense and may cause undefined behavior,
> > depending on the backend.
> > 
> > Also, the walk request described in the 9P spec [1] clearly shows that
> > the client is supposed to send individual path components: the official
> > linux client never sends portions of path containing the / character for
> > example.
> > 
> > Moreover, the 9P spec [2] also states that a system can decide to restrict
> > the set of supported characters used in path components, with an explicit
> > mention "to remove slashes from name components".
> > 
> > This patch introduces a new name_is_illegal() helper that checks the
> > names sent by the client are not empty and don't contain unwanted chars.
> > Since 9pfs is only supported on linux hosts, only the / character is
> > checked at the moment. When support for other hosts (AKA. win32) is added,
> > other chars may need to be blacklisted as well.
> > 
> > If a client sends an illegal path component, the request will fail and
> > EINVAL is returned to the client.
> > 
> > [1] http://man.cat-v.org/plan_9/5/walk
> > [2] http://man.cat-v.org/plan_9/5/intro
> > 
> > Suggested-by: Peter Maydell <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  hw/9pfs/9p.c |   56 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index b6b02b46a9da..dba11773699b 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1256,6 +1256,11 @@ static int v9fs_walk_marshal(V9fsPDU *pdu, uint16_t 
> > nwnames, V9fsQID *qids)
> >      return offset;
> >  }
> >  
> > +static bool name_is_illegal(const char *name)
> > +{
> > +    return name == NULL || strchr(name, '/') != NULL;  
> 
> Is anyone actually passing NULL?  And the commit message says you are
> blacklisting the empty string "", but that is not what you did here.
> Depending on whether callers can even pass NULL, you may be able to just
> s/name == NULL/!*name/
> 

Oops you're right, v9fs_iov_vunmarshal() always allocates and sets str->data, 
even
for empty strings... so I guess this cannot be call with NULL and your 
suggestion
is the way to go.

pgpedwu5wK7S9.pgp
Description: OpenPGP digital signature