| Message ID | 147222402890.18925.12890875990211775724.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
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 <peter.maydell@linaro.org> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > 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 have to worry about rawmemchr() being present. Reviewed-by: Eric Blake <eblake@redhat.com>
On Fri, 26 Aug 2016 13:41:23 -0500 Eric Blake <eblake@redhat.com> 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 <peter.maydell@linaro.org> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > 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 <eblake@redhat.com> >
On Fri, 26 Aug 2016 17:07:08 +0200 Greg Kurz <groug@kaod.org> 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 <peter.maydell@linaro.org> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > 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) { > + copied = -EINVAL; > + } Ugh ! QEMU sends terminating NULs :-\ which cause virtfs-proxy-helper to fail here during init. > + } > + if (copied <= 0) { > v9fs_string_free(str); > } > } > >
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) { + copied = -EINVAL; + } + } + if (copied <= 0) { v9fs_string_free(str); } }
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 <peter.maydell@linaro.org> Signed-off-by: Greg Kurz <groug@kaod.org> --- fsdev/9p-iov-marshal.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)