Re: [Qemu-devel] [PATCH v2 2/5] 9p: disallow the NUL character in all strings

[Greg Kurz]

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

> On 08/26/2016 10:07 AM, Greg Kurz wrote:
> > According to the 9P spec at http://man.cat-v.org/plan_9/5/intro :
> > 
> > Data items of larger or variable lengths are represented by a
> > two-byte field specifying a count, n, followed by n bytes of
> > data.  Text strings are represented this way, with the text
> > itself stored as a UTF-8 encoded sequence of Unicode charac-
> > ters (see utf(6)). Text strings in 9P messages are not NUL-
> > terminated: n counts the bytes of UTF-8 data, which include
> > no final zero byte.  The NUL character is illegal in all
> > text strings in 9P, and is therefore excluded from file
> > names, user names, and so on.
> > 
> > With this patch, if a 9P client sends a text string containing a NUL
> > character, the request will fail and the client is returned EINVAL.
> > 
> > The checking is done in v9fs_iov_vunmarshal() because it is a convenient
> > place to check all client originated strings.
> > 
> > Suggested-by: Peter Maydell <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >  fsdev/9p-iov-marshal.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> > index 663cad542900..9bcdc370231d 100644
> > --- a/fsdev/9p-iov-marshal.c
> > +++ b/fsdev/9p-iov-marshal.c
> > @@ -127,7 +127,12 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int 
> > out_num, size_t offset,
> >                                       str->size);
> >                  if (copied > 0) {
> >                      str->data[str->size] = 0;
> > -                } else {
> > +                    /* 9P forbids NUL characters in all text strings */
> > +                    if (strlen(str->data) != str->size) {  
> 
> If this were glibc, we could micro-optimize and do:
> 
> if (rawmemchr(str->data, 0) != str->data + str->size)
> 
> so that strlen() doesn't have to visit the tail end of the string if a
> NUL is present early.  But your code is just fine as-is, and doesn't

Hmmm... if a NUL is present early, why would strlen() visit the tail end of
the string ?

Looking at the glibc sources (string/strlen.c and string/rawmemchr.c), both 
calls
share the same implementation: handle initial characters 1 by 1 and then test a
longword at a time... and FWIW, strlen() knows at compile time it looks for 0 
instead of a runtime character for rawmemchr(). I have the feeling that strlen()
is the more optimized actually.

> have to worry about rawmemchr() being present.
> 
> Reviewed-by: Eric Blake <address@hidden>
>

pgpBoUERMwrUR.pgp
Description: OpenPGP digital signature