Re: [Qemu-devel] [PATCH 03/10] block/dmg: extract processing of resource forks

[John Snow]

On 12/27/2014 10:01 AM, Peter Wu wrote:
> Besides the offset, also read the resource length. This length is now
> used in the extracted function to verify the end of the resource fork
> against "count" from the resource fork.
>
> Signed-off-by: Peter Wu <address@hidden>
> ---
>   block/dmg.c | 90 ++++++++++++++++++++++++++++++++++++++++---------------------
>   1 file changed, 59 insertions(+), 31 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 6dc6dbb..7f49388 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -278,38 +278,13 @@ fail:
>       return ret;
>   }
>
> -static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> -                    Error **errp)
> +static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
> +                                  uint64_t info_begin, uint64_t info_length)
>   {
> -    BDRVDMGState *s = bs->opaque;
> -    DmgHeaderState ds;
> -    uint64_t info_begin, info_end;
> -    uint32_t count, tmp;
> -    int64_t offset;
>       int ret;
> -
> -    bs->read_only = 1;
> -    s->n_chunks = 0;
> -    s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> -    ds.last_in_offset = 0;
> -    ds.last_out_offset = 0;
> -    ds.max_compressed_size = 1;
> -    ds.max_sectors_per_chunk = 1;
> -
> -    /* locate the UDIF trailer */
> -    offset = dmg_find_koly_offset(bs->file);
> -    if (offset < 0) {
> -        ret = offset;
> -        goto fail;
> -    }
> -
> -    ret = read_uint64(bs, offset + 0x28, &info_begin);
> -    if (ret < 0) {
> -        goto fail;
> -    } else if (info_begin == 0) {
> -        ret = -EINVAL;
> -        goto fail;
> -    }
> +    uint32_t count, tmp;
> +    uint64_t info_end;
> +    uint64_t offset;
>
>       ret = read_uint32(bs, info_begin, &tmp);
>       if (ret < 0) {
> @@ -326,6 +301,10 @@ static int dmg_open(BlockDriverState *bs, QDict *options, 
> int flags,
>           ret = -EINVAL;
>           goto fail;
>       }
> +    if (count > info_length) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
>       info_end = info_begin + count;
>
>       /* begin of mish block */
> @@ -342,12 +321,61 @@ static int dmg_open(BlockDriverState *bs, QDict *options, 
> int flags,
>           }
>           offset += 4;
>
> -        ret = dmg_read_mish_block(bs, &ds, offset, count);
> +        ret = dmg_read_mish_block(bs, ds, offset, count);
>           if (ret < 0) {
>               goto fail;
>           }
>           offset += count;
>       }
> +    return 0;
> +
> +fail:
> +    return ret;
> +}
> +
> +static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> +                    Error **errp)
> +{
> +    BDRVDMGState *s = bs->opaque;
> +    DmgHeaderState ds;
> +    uint64_t rsrc_fork_offset, rsrc_fork_length;
> +    int64_t offset;
> +    int ret;
> +
> +    bs->read_only = 1;
> +    s->n_chunks = 0;
> +    s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> +    ds.last_in_offset = 0;
> +    ds.last_out_offset = 0;
> +    ds.max_compressed_size = 1;
> +    ds.max_sectors_per_chunk = 1;
> +
> +    /* locate the UDIF trailer */
> +    offset = dmg_find_koly_offset(bs->file);
> +    if (offset < 0) {
> +        ret = offset;
> +        goto fail;
> +    }
> +
> +    /* offset of resource fork (RsrcForkOffset) */
> +    ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    ret = read_uint64(bs, offset + 0x30, &rsrc_fork_length);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    if (rsrc_fork_offset != 0 && rsrc_fork_length != 0) {
> +        ret = dmg_read_resource_fork(bs, &ds,
> +                                     rsrc_fork_offset, rsrc_fork_length);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +    } else {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
>
>       /* initialize zlib engine */
>       s->compressed_chunk = qemu_try_blockalign(bs->file,

I know your refactor hardly touches the code here, but this is a good 
place to mention it:

From what I can tell from the Resource Fork format
https://developer.apple.com/legacy/library/documentation/mac/pdf/MoreMacintoshToolbox.pdf#page=151

there are four fields of interest in the header of the resourcefork 
before we start looping trying to find mish data:

uint32_t data_offset;
uint32_t map_offset;
uint32_t data_length;
uint32_t map_length;

We are treating the map which comes right after the data as the "data 
length" which is not too far from the truth, but may include extra bytes.

e.g., in the vlc dmg you linked:

The koly header advertises a Resource Fork length of 93276 bytes.
the resource fork itself has a header of:

data_offset := 256
map_offset := 92860     // not length!
data_length := 92604
map_length := 416


Since we are naming this function explicitly after the Resource Fork, we 
should probably tidy this up just a little bit to be clearer at the very 
least.

The space reserved for system use, here asserted by QEMU to always be 
0x100, is technically variable and perhaps could be adjusted as well 
with minimal changes.

A comment pointing to this documentation for the format here would also 
be helpful, as your other source details koly and mish, but Resource 
Fork was missing.