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

[John Snow <jsnow@redhat.com>]

On 12/27/2014 10:01 AM, 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 a continuous input offset.
>

What does "continuous input offset" mean? This change is not as clear to 
me, see below.

> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   block/dmg.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 984997f..93b597f 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -179,6 +179,7 @@ static int64_t dmg_find_koly_offset(BlockDriverState *file_bs)
>   typedef struct DmgHeaderState {
>       /* used internally by dmg_read_mish_block to remember offsets of blocks
>        * across calls */
> +    uint64_t data_fork_offset;
>       uint64_t last_in_offset;
>       uint64_t last_out_offset;
>       /* exported for dmg_open */
> @@ -194,6 +195,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>       size_t new_size;
>       uint32_t chunk_count;
>       int64_t offset = 0;
> +    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) */
> @@ -246,7 +248,12 @@ 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;
> +        /* If this offset is below the previous chunk end, then assume that all
> +         * following offsets are after the previous chunks. */
> +        if (s->offsets[i] + in_offset < ds->last_in_offset) {
> +            in_offset = ds->last_in_offset;
> +        }
> +        s->offsets[i] += in_offset;

I take it that all of the offsets referenced in the mish structures are 
relative to the start of the data fork block, which is why we are taking 
a value from the koly block and applying it to mish block values.

correct?

>           offset += 8;
>
>           s->lengths[i] = buff_read_uint64(buffer, offset);
> @@ -400,6 +407,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>       bs->read_only = 1;
>       s->n_chunks = 0;
>       s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> +    ds.data_fork_offset = 0;
>       ds.last_in_offset = 0;
>       ds.last_out_offset = 0;
>       ds.max_compressed_size = 1;
> @@ -412,6 +420,12 @@ 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;
> +    }
> +
>       /* offset of resource fork (RsrcForkOffset) */
>       ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
>       if (ret < 0) {
>

A general question here:

Are we ever reading the preamble of the mish block? I see we are reading 
the 'n' items of 40-byte chunk data, but is there a reason we skip the 
first 200 bytes of mish data, or have I misread the documents on DMG 
that exist?

It looks like there are some good fields here: SectorNumber, 
SectorCount, DataOffset, and BlockDescriptors -- can these not be used 
to provide a more explicit error-checking of offsets, allowing us to 
make less assumptions about where these blocks begin and end?

Is there some reason they are unreliable?