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 <pe...@lekensteyn.nl>
> > ---
> > 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
