Re: [Qemu-devel] [PATCH v2 08/12] block/dmg: fix sector data offset calculation

[John Snow <jsnow@redhat.com>]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> This patch addresses two issues:
>
>   - The data fork offset was not taken into account, resulting in failure
>     to read an InstallESD.dmg file (5164763151 bytes) which had a
>     non-zero DataForkOffset field.
>   - The offset of the previous block ("partition") was unconditionally
>     added to the current block because older files would start the input
>     offset of a new block at zero. Newer files (including vlc-2.1.5.dmg,
>     tuxpaint-0.9.15-macosx.dmg and OS X Yosemite [MAS].dmg) failed in
>     reads because these files have chunk offsets, relative to the begin
>     of a data fork.
>
> Now the data offset of the mish is taken into account. While we could
> check that the data_offset is within the data fork, let's not do that
> here as it would only result in parse failures on invalid files (rather
> than gracefully handling such bad files). dmg_read will error out if
> the offset is incorrect.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: use sector and data offset as provided by the BLKX header. This
>       allows us to drop last_in_offset. The previous heuristics to detect
>       relative offsets is not needed anymore. Squashed the data fork
>       offset length check into this patch.
> ---
>   block/dmg.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 57feb1b..130efac 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -188,7 +188,7 @@ static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
>   typedef struct DmgHeaderState {
>       /* used internally by dmg_read_mish_block to remember offsets of blocks
>        * across calls */
> -    uint64_t last_in_offset;
> +    uint64_t data_fork_offset;
>       uint64_t last_out_offset;
>       /* exported for dmg_open */
>       uint32_t max_compressed_size;
> @@ -203,6 +203,8 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>       size_t new_size;
>       uint32_t chunk_count;
>       int64_t offset = 0;
> +    uint64_t data_offset;
> +    uint64_t in_offset = ds->data_fork_offset;
>
>       type = buff_read_uint32(buffer, offset);
>       /* skip data that is not a valid MISH block (invalid magic or too small) */
> @@ -211,8 +213,12 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           return 0;
>       }
>
> -    offset += 4;
> -    offset += 200;
> +    /* location in data fork for (compressed) blob (in bytes) */
> +    data_offset = buff_read_uint64(buffer, offset + 0x18);
> +    in_offset += data_offset;
> +
> +    /* move to begin of chunk entries */
> +    offset += 204;
>
>       chunk_count = (count - 204) / 40;
>       new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
> @@ -228,7 +234,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
>               s->types[i] != 2) {
>               if (s->types[i] == 0xffffffff && i > 0) {
> -                ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];

Great!

>                   ds->last_out_offset = s->sectors[i - 1] +
>                                         s->sectorcounts[i - 1];
>               }
> @@ -255,7 +260,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           }
>
>           s->offsets[i] = buff_read_uint64(buffer, offset);
> -        s->offsets[i] += ds->last_in_offset;
> +        s->offsets[i] += in_offset;
>           offset += 8;
>
>           s->lengths[i] = buff_read_uint64(buffer, offset);
> @@ -413,7 +418,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>       s->n_chunks = 0;
>       s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
>       /* used by dmg_read_mish_block to keep track of the current I/O position */
> -    ds.last_in_offset = 0;
> +    ds.data_fork_offset = 0;
>       ds.last_out_offset = 0;
>       ds.max_compressed_size = 1;
>       ds.max_sectors_per_chunk = 1;
> @@ -425,6 +430,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>           goto fail;
>       }
>
> +    /* offset of data fork (DataForkOffset) */
> +    ret = read_uint64(bs, offset + 0x18, &ds.data_fork_offset);
> +    if (ret < 0) {
> +        goto fail;
> +    } else if (ds.data_fork_offset > offset) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>       /* offset of resource fork (RsrcForkOffset) */
>       ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
>       if (ret < 0) {
>

Reviewed-by: John Snow <jsnow@redhat.com>