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

[Chen Gang]

On 03/03/2014 04:34 PM, Markus Armbruster wrote:
> 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.
> 

That sounds good to me, after this patch pass checking, I will/should
send patch v2 for it.

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

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed