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

[Aneesh Kumar K.V]

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:
>
>  - Environments (for qemu-devel):
>
>    - Host is under fedora17 desktop with ext4fs:
>
>      qemu-system-x86_64 -hda test.img -m 1024 \
>        -net nic,vlan=4,model=virtio,macaddr=00:16:35:AF:94:04 \
>        -net tap,vlan=4,ifname=tap4,script=no,downscript=no \
>        -device virtio-9p-pci,id=fs0,fsdev=fsdev0,mount_tag=hostshare \
>        -fsdev local,security_model=passthrough,id=fsdev0,\
>          path=/upstream/vm/data/share/1234567890abcdefghijklmnopqrstuvwxyz\
>            ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890acdefghijklmnopqrstuvwxyz\
>            ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890/111111111111111111111111111\
>            1111111111111111111111111111111111111111111111111111222222222222\
>            2222222222222222222222222222222222222222222222222222222222222222\
>            2222222222222222222222222222222222233333333333333333333333333333\
>            3333333333333333333333333333333333
>
>     - Guest is ubuntu12 server with 9pfs.
>
>       mount -t 9p -o trans=virtio,version=9p2000.L hostshare /share
>
>     - Limitations:
>
>       full path limitation is PATH_MAX (4096B include nul) under Linux.
>       file/dir node name maximized length is 256 (include nul) under ext4.
>
>  - Special test:
>
>     Under host, modify the file: "/upstream/vm/data/share/1234567890abcdefg\
>       hijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890acdefghijklmno\
>       pqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890/111111111111111111111\
>       111111111111111111111111111111111111111111111111111111111122222222222\
>       222222222222222222222222222222222222222222222222222222222222222222222\
>       222222222222222222222222222222233333333333333333333333333333333333333\
>       3333333333333333333333333/4444444444444444444444444444444444444444444\
>       444444444444444444444444444444444444444444444444444444444444444444444\
>       444444444444444444444444444444444444444444444444444444444444444444444\
>       444444444444444444444444444444444444444/55555555555555555555555555555\
>       555555555555555555555555555555555555555555555555555555555555555555555\
>       555555555555555555555555555555555555555555555555555555555555555555555\
>       555555555555555555555555555555555555555555555555555555555555555555555\
>       55555555/666666666666666666666666666666666666666666666666666666666666\
>       666666666666666666666666666666666666666666666666666666666666666666666\
>       666666666666666666666666666666666666666666666666666666666666666666666\
>       666666666666666666666/77777777777777777777777777777777777777777777777\
>       777777777777777777777777777777777777777777777777777777777777777777777\
>       777777777777777777777777777777777777777777777777777777777777777777777\
>       77777777777777777777777777777777777777777777777777777777777/888888888\
>       888888888888888888888888888888888888888888888888888888888888888888888\
>       888888888888888888888888888888888888888888888888888888888888888888888\
>       888888888888888888888888888888888888888888888888888888888888888888888\
>       888888888/99999999999999999999999999999999999999999999999999999999999\
>       999999999999999999999999999999999999999999999999999999999999999999999\
>       999999999999999999999999999999999999999999999999999999999999999999999\
>       99999999999999999999999999999999999999999/000000000000000000000000000\
>       000000000000000000000000000000000000000000000000000000000000000000000\
>       000000000000000000000000000000000000000000000000000000000000000000000\
>       000000000000000000000000000000000000000000000000/aaaaaaaaaaaaaaaaaaaa\
>       aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
>       aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\
>       aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/bbbbbbbbbbbbbbbbbbbbbbb\
>       bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\
>       bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\
>       bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb/ccccccccc\
>       ccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc\
>       ccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc\
>       ccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc\
>       cccccccccc/dddddddddddddddddddddddddddddddddddddddddddddddddddddddddd\
>       ddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd\
>       ddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd\
>       dddddddddddddddddddddd/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\
>       eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\
>       eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee\
>       eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee/fffffffffffffff\
>       fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff\
>       fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff\
>       ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/gggggggggg\
>       ggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg\
>       ggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg\
>       ggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg\
>       ggggggggggggggggggggggg/iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii\
>       iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii\
>       iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii\
>       iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii/jjjjjjjjjjjjj\
>       jjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjj\
>       jjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjj/ppppppppppppppppppppp\
>       ppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppppp\
>       ppppppppppppppppppppppppppppppppppppppp/test1234567890file.log"
>         (need enter dir firstly, then modify file, or can not open it).
>
>    Under guest, still allow modify "test1234567890file.log" (will generate
>    "test123456" file with contents).
>
>    After apply this patch, can not open "test1234567890file.log" under guest
>    (permission denied).
>
>
>  - Common test:
>
>    All are still OK after apply this path.
>
>      "mkdir -p", "create/open file/dir", "modify file/dir", "rm file/dir".
>      change various mount point paths under host and/or guest.
>
>
> 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) {

Can we keep this as 
      v9fs_co_run_in_worker(
        {
                      
               buf->data = __readlink(&s->ctx, path);

        }

I can do that change for you if you want. I will also have to go through
the rest of the code to make sure we do free the memory in all error
path. So the rest of the review is going to take time. Hope that is ok

-aneesh