Re: [Qemu-devel] [PATCH 4/6] dmg: Fix bdrv_open() error handling

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Return -errno instead of -1 on errors. While touching the
> code, fix even some more memory leaks than in the other drivers...

LOL

> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/dmg.c |  136 ++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 103 insertions(+), 33 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index ac397dc..d47d2d8 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -57,29 +57,53 @@ static int dmg_probe(const uint8_t *buf, int buf_size, 
> const char *filename)
>      return 0;
>  }
>  
> -static off_t read_off(BlockDriverState *bs, int64_t offset)
> +static int read_uint64(BlockDriverState *bs, int64_t offset, uint64_t 
> *result)
>  {
> -     uint64_t buffer;
> -     if (bdrv_pread(bs->file, offset, &buffer, 8) < 8)
> -             return 0;
> -     return be64_to_cpu(buffer);
> +    uint64_t buffer;
> +    int ret;
> +
> +    ret = bdrv_pread(bs->file, offset, &buffer, 8);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    *result = be64_to_cpu(buffer);
> +    return 0;
>  }
>  
> -static off_t read_uint32(BlockDriverState *bs, int64_t offset)
> +static int read_off(BlockDriverState *bs, int64_t offset, off_t *result)
> +{
> +    uint64_t buffer;
> +    int ret;
> +
> +    ret = read_uint64(bs, offset, &buffer);
> +    *result = buffer;
> +
> +    return ret;
> +}
> +
> +static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t 
> *result)
>  {
>       uint32_t buffer;

Indentation of the line above is now off.

> -     if (bdrv_pread(bs->file, offset, &buffer, 4) < 4)
> -             return 0;
> -     return be32_to_cpu(buffer);
> +    int ret;
> +
> +    ret = bdrv_pread(bs->file, offset, &buffer, 4);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    *result = be32_to_cpu(buffer);
> +    return 0;
>  }

You're fixing bugs here.  Please mention them in the commit message.

Code before:

* read_off() silently truncates when off_t is narrower than 64 bits, and
  can't distinguish between error and successful read of zero.

* Three out of four callers of read_off() don't bother to check for
  failure.

* read_uint32() can't distinguish between error and successful read of
  zero.  Truncation isn't a problem, because off_t is surely at least 32
  bits wide.

* Two out of four callers of read_uint32() don't bother to check for
  failure.

After:

* All callers check for errors.

* read_uint64() and read_uint32() are fine.

* read_off() silently truncates when off_t is narrower than 64 bits.
  Why isn't that a problem?

  Hmm, the only remaining caller reads into off_t info_begin, which is
  then passed to int64_t parameters, either directly, or via off_t
  offset.  What about replacing the remaining off_t by int64_t?

>  
>  static int dmg_open(BlockDriverState *bs, int flags)
>  {
>      BDRVDMGState *s = bs->opaque;
>      off_t info_begin,info_end,last_in_offset,last_out_offset;
> -    uint32_t count;
> +    uint32_t count, tmp;
>      uint32_t max_compressed_size=1,max_sectors_per_chunk=1,i;
>      int64_t offset;
> +    int ret;
>  
>      bs->read_only = 1;
>      s->n_chunks = 0;
> @@ -88,21 +112,32 @@ static int dmg_open(BlockDriverState *bs, int flags)
>      /* read offset of info blocks */
>      offset = bdrv_getlength(bs->file);
>      if (offset < 0) {
> +        ret = offset;
>          goto fail;
>      }
>      offset -= 0x1d8;
>  
> -    info_begin = read_off(bs, offset);
> -    if (info_begin == 0) {
> -     goto fail;
> +    ret = read_off(bs, offset, &info_begin);
> +    if (ret < 0) {
> +        goto fail;
> +    } else if (info_begin == 0) {
> +        ret = -EINVAL;
> +        goto fail;
>      }
>  
> -    if (read_uint32(bs, info_begin) != 0x100) {
> +    ret = read_uint32(bs, info_begin, &tmp);
> +    if (ret < 0) {
> +        goto fail;
> +    } else if (tmp != 0x100) {
> +        ret = -EINVAL;
>          goto fail;
>      }
>  
> -    count = read_uint32(bs, info_begin + 4);
> -    if (count == 0) {
> +    ret = read_uint32(bs, info_begin + 4, &count);
> +    if (ret < 0) {
> +        goto fail;
> +    } else if (count == 0) {
> +        ret = -EINVAL;
>          goto fail;
>      }
>      info_end = info_begin + count;
> @@ -114,12 +149,20 @@ static int dmg_open(BlockDriverState *bs, int flags)
>      while (offset < info_end) {
>          uint32_t type;
>  
> -     count = read_uint32(bs, offset);
> -     if(count==0)
> -         goto fail;
> +        ret = read_uint32(bs, offset, &count);
> +        if (ret < 0) {
> +            goto fail;
> +        } else if (count == 0) {
> +            ret = -EINVAL;
> +            goto fail;
> +        }
>          offset += 4;
>  
> -     type = read_uint32(bs, offset);
> +        ret = read_uint32(bs, offset, &type);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +
>       if (type == 0x6d697368 && count >= 244) {
>           int new_size, chunk_count;
>  
> @@ -134,8 +177,11 @@ static int dmg_open(BlockDriverState *bs, int flags)
>           s->sectors = g_realloc(s->sectors, new_size);
>           s->sectorcounts = g_realloc(s->sectorcounts, new_size);
>  
> -         for(i=s->n_chunks;i<s->n_chunks+chunk_count;i++) {
> -             s->types[i] = read_uint32(bs, offset);
> +            for(i=s->n_chunks;i<s->n_chunks+chunk_count;i++) {

Since you're touching this line anyway, you could treat the poor thing
to a few spaces.

> +                ret = read_uint32(bs, offset, &s->types[i]);
> +                if (ret < 0) {
> +                    goto fail;
> +                }
>               offset += 4;
>               if(s->types[i]!=0x80000005 && s->types[i]!=1 && s->types[i]!=2) 
> {
>                   if(s->types[i]==0xffffffff) {
> @@ -149,17 +195,31 @@ static int dmg_open(BlockDriverState *bs, int flags)
>               }
>               offset += 4;
>  
> -             s->sectors[i] = last_out_offset+read_off(bs, offset);
> -             offset += 8;
> -
> -             s->sectorcounts[i] = read_off(bs, offset);
> -             offset += 8;
> -
> -             s->offsets[i] = last_in_offset+read_off(bs, offset);
> -             offset += 8;
> -
> -             s->lengths[i] = read_off(bs, offset);
> -             offset += 8;
> +                ret = read_uint64(bs, offset, &s->sectors[i]);
> +                if (ret < 0) {
> +                    goto fail;
> +                }
> +                s->sectors[i] += last_out_offset;
> +                offset += 8;
> +
> +                ret = read_uint64(bs, offset, &s->sectorcounts[i]);
> +                if (ret < 0) {
> +                    goto fail;
> +                }
> +                offset += 8;
> +
> +                ret = read_uint64(bs, offset, &s->offsets[i]);
> +                if (ret < 0) {
> +                    goto fail;
> +                }
> +                s->offsets[i] += last_in_offset;
> +                offset += 8;
> +
> +                ret = read_uint64(bs, offset, &s->lengths[i]);
> +                if (ret < 0) {
> +                    goto fail;
> +                }
> +                offset += 8;
>  
>               if(s->lengths[i]>max_compressed_size)
>                   max_compressed_size = s->lengths[i];
> @@ -173,15 +233,25 @@ static int dmg_open(BlockDriverState *bs, int flags)
>      /* initialize zlib engine */
>      s->compressed_chunk = g_malloc(max_compressed_size+1);
>      s->uncompressed_chunk = g_malloc(512*max_sectors_per_chunk);
> -    if(inflateInit(&s->zstream) != Z_OK)
> -     goto fail;
> +    if(inflateInit(&s->zstream) != Z_OK) {
> +        ret = -EINVAL;

As in PATCH 2/4: good enough.

> +        goto fail;
> +    }
>  
>      s->current_chunk = s->n_chunks;
>  
>      qemu_co_mutex_init(&s->lock);
>      return 0;
> +
>  fail:
> -    return -1;
> +    g_free(s->types);
> +    g_free(s->offsets);
> +    g_free(s->lengths);
> +    g_free(s->sectors);
> +    g_free(s->sectorcounts);
> +    g_free(s->compressed_chunk);
> +    g_free(s->uncompressed_chunk);
> +    return ret;
>  }
>  
>  static inline int is_sector_in_chunk(BDRVDMGState* s,