Re: [Qemu-devel] [PATCH 3/3] hw/9pfs: use g_strdup_printf() instead of PATH_MAX limitation

[Markus Armbruster]

Chen Gang <address@hidden> writes:

> When path is truncated by PATH_MAX limitation, it causes QEMU to access
> incorrect file. So use original full path instead of PATH_MAX within
> 9pfs (need check/process ENOMEM for related memory allocation).
>
> The related test:
[...]
> Signed-off-by: Chen Gang <address@hidden>
> ---
>  hw/9pfs/cofs.c                 |  15 ++-
>  hw/9pfs/virtio-9p-handle.c     |   9 +-
>  hw/9pfs/virtio-9p-local.c      | 285 
> +++++++++++++++++++++++++++--------------
>  hw/9pfs/virtio-9p-posix-acl.c  |  52 ++++++--
>  hw/9pfs/virtio-9p-xattr-user.c |  27 +++-
>  hw/9pfs/virtio-9p-xattr.c      |   9 +-
>  hw/9pfs/virtio-9p-xattr.h      |  27 +++-
>  hw/9pfs/virtio-9p.h            |   6 +-
>  8 files changed, 292 insertions(+), 138 deletions(-)
>
> diff --git a/hw/9pfs/cofs.c b/hw/9pfs/cofs.c
> index 3891050..739bad0 100644
> --- a/hw/9pfs/cofs.c
> +++ b/hw/9pfs/cofs.c
> @@ -20,18 +20,24 @@
>  int v9fs_co_readlink(V9fsPDU *pdu, V9fsPath *path, V9fsString *buf)
>  {
>      int err;
> -    ssize_t len;
> +    ssize_t len, maxlen = PATH_MAX;
>      V9fsState *s = pdu->s;
>  
>      if (v9fs_request_cancelled(pdu)) {
>          return -EINTR;
>      }
> -    buf->data = g_malloc(PATH_MAX);
> +    buf->data = g_malloc(maxlen);
>      v9fs_path_read_lock(s);
>      v9fs_co_run_in_worker(
> -        {
> +        while (1) {
>              len = s->ops->readlink(&s->ctx, path,
> -                                   buf->data, PATH_MAX - 1);
> +                                   buf->data, maxlen - 1);
> +            if (len == maxlen - 1) {
> +                g_free(buf->data);
> +                maxlen *= 2;
> +                buf->data = g_malloc(maxlen);
> +                continue;
> +            }
>              if (len > -1) {
>                  buf->size = len;
>                  buf->data[len] = 0;
                   err = 0;
>              } else {
>                  err = -errno;
>              }
> +            break;
>          });
>      v9fs_path_unlock(s);
>      if (err) {

Harmless off-by-one: you double the buffer even when the link contents
plus terminating null fits the buffer exactly (len == maxlen - 1).

I prefer to have the exceptional stuff handled in conditionals, and not
the normal stuff, like this:

        for (;;) {
            len = s->ops->readlink(&s->ctx, path, buf->data, maxlen);
            if (len < 0) {
                err = -errno;
                break;
            }
            if (len == maxlen) {
                g_free(buf->data);
                maxlen *= 2;
                buf->data = g_malloc(maxlen);
                continue;
            }
            buf->size = len;
            buf->data[len] = 0;
            err = 0;
            break;
        }

Matter of taste.

[...]

I skimmed a few more hunks, and they look good to me.  Leaving full
review to Aneesh.