From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8uz8-0002Gv-Gy for qemu-devel@nongnu.org; Wed, 07 Jan 2015 13:05:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y8uz3-0005fk-GZ for qemu-devel@nongnu.org; Wed, 07 Jan 2015 13:05:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57092) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y8uz3-0005fc-9K for qemu-devel@nongnu.org; Wed, 07 Jan 2015 13:05:29 -0500 Message-ID: <54AD755D.2000108@redhat.com> Date: Wed, 07 Jan 2015 13:05:17 -0500 From: John Snow MIME-Version: 1.0 References: <1420566495-13284-1-git-send-email-peter@lekensteyn.nl> <1420566495-13284-4-git-send-email-peter@lekensteyn.nl> In-Reply-To: <1420566495-13284-4-git-send-email-peter@lekensteyn.nl> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 03/12] block/dmg: extract processing of resource forks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Wu , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 01/06/2015 12:48 PM, 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. > > Instead of relying on the value of offset to conclude whether the > resource fork is available or not (info_begin==0), check the > rsrc_fork_length instead. This would allow a dmg file to begin with a > resource fork. This seemingly unnecessary restriction was found while > trying to craft a DMG file by hand. > > Other changes: > > - Do not require resource data offset to be 0x100 (but check that it > is within bounds though). > - Further improve boundary checking (resource data must be within > the resource fork). > - Use correct value for resource data length (spotted by John Snow) > - Consider the resource data offset when determining info_end. > This fixes an EINVAL on the tuxpaint dmg example. > > The resource fork format is documented at > https://developer.apple.com/legacy/library/documentation/mac/pdf/MoreMacintoshToolbox.pdf#page=151 > > Signed-off-by: Peter Wu > --- > v2: expanded commit message (incl. documentation link). Do not require > a fixed resource data offset of 0x100, improve boundary checking, > fix resource data len (8 vs 4), append offset when checking the end > of the resource data. > -- > block/dmg.c | 104 ++++++++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 66 insertions(+), 38 deletions(-) > > diff --git a/block/dmg.c b/block/dmg.c > index e01559f..ed99cf5 100644 > --- a/block/dmg.c > +++ b/block/dmg.c > @@ -287,60 +287,38 @@ 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, rsrc_data_offset; > - int64_t offset; > int ret; > + uint32_t count, rsrc_data_offset; > + uint64_t info_end; > + uint64_t offset; > > - bs->read_only = 1; > - 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.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, errp); > - 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; > - } > - > + /* read offset from begin of resource fork (info_begin) to resource data */ > ret = read_uint32(bs, info_begin, &rsrc_data_offset); > if (ret < 0) { > goto fail; > - } else if (rsrc_data_offset != 0x100) { > + } else if (rsrc_data_offset > info_length) { > ret = -EINVAL; > goto fail; > } > > - ret = read_uint32(bs, info_begin + 4, &count); > + /* read length of resource data */ > + ret = read_uint32(bs, info_begin + 8, &count); > if (ret < 0) { > goto fail; > - } else if (count == 0) { > + } else if (count == 0 || rsrc_data_offset + count > info_length) { > ret = -EINVAL; > goto fail; > } > - /* end of resource data, ignoring the following resource map */ > - info_end = info_begin + count; > > /* begin of resource data (consisting of one or more resources) */ > - offset = info_begin + 0x100; > + offset = info_begin + rsrc_data_offset; > + > + /* end of resource data (there is possibly a following resource map > + * which will be ignored). */ > + info_end = offset + count; > > /* read offsets (mish blocks) from one or more resources in resource data */ > while (offset < info_end) { > @@ -354,13 +332,63 @@ 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; > } > /* advance offset by size of resource */ > 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; > + /* used by dmg_read_mish_block to keep track of the current I/O position */ > + 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, errp); > + 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_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, > Reviewed-by: John Snow