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

[Peter Wu]

On Friday 02 January 2015 19:01:06 John Snow wrote:
> 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.

The rsrc_fork_offset and rsrc_fork_length values above come from the
trailer, not the resource fork... but now that I read the new function I
see what you mean here. The offset '4' should be '8'. I'll fix this in
v2.

> 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.

This artificial restriction looks unnecessary even when there are no
other files in practice. I have replaced it by a saner check for v2.

> 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.

I'll mention this in the commit message, thanks!
-- 
Kind regards,
Peter
https://lekensteyn.nl