Re: [Qemu-devel] [PATCH] 9pfs: make unmarshal V9fsString more robust

[Greg Kurz]

On Tue, 27 Sep 2016 04:44:11 -0700
Li Qiang <address@hidden> wrote:

> From: Li Qiang <address@hidden>
> 
> In 9pfs function v9fs_iov_vunmarshal, it will not allocate space
> for empty string. This will cause several NULL pointer dereference
> issues. this patch fix this issue.
> 
> Signed-off-by: Li Qiang <address@hidden>
> ---

Talking about robustness was appropriate for your previous patches, but
it does not really apply here since v9fs_iov_vunmarshal() does not have
any issue with empty strings actually.

I've changed the title to:

9pfs: allocate space for guest originated empty strings

And while here, I've updated the changelog to provide a more detailed
justification:

    If a guest sends an empty string paramater to any 9P operation, the current
    code unmarshals it into a V9fsString equal to { .size = 0, .data = NULL }.
    
    This is unfortunate because it can cause NULL pointer dereference to happen
    at various locations in the 9pfs code. And we don't want to check str->data
    everywhere we pass it to strcmp() or any other function which expects a
    dereferenceable pointer.
    
    This patch enforces the allocation of genuine C empty strings instead, so
    callers don't have to bother.

Thanks.

--
Greg

>  fsdev/9p-iov-marshal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fsdev/9p-iov-marshal.c b/fsdev/9p-iov-marshal.c
> index 663cad5..1d16f8d 100644
> --- a/fsdev/9p-iov-marshal.c
> +++ b/fsdev/9p-iov-marshal.c
> @@ -125,7 +125,7 @@ ssize_t v9fs_iov_vunmarshal(struct iovec *out_sg, int 
> out_num, size_t offset,
>                  str->data = g_malloc(str->size + 1);
>                  copied = v9fs_unpack(str->data, out_sg, out_num, offset,
>                                       str->size);
> -                if (copied > 0) {
> +                if (copied >= 0) {
>                      str->data[str->size] = 0;
>                  } else {
>                      v9fs_string_free(str);