Re: [Qemu-devel] [PATCH 01/10] block/dmg: properly detect the UDIF trailer

[Peter Wu]

On Friday 02 January 2015 18:58:00 John Snow wrote:
> 
> On 12/27/2014 10:01 AM, Peter Wu wrote:
> > DMG files have a variable length with a UDIF trailer at the end of a
> > file. This UDIF trailer is essential as it describes the contents of
> > the image. At the moment however, the start of this trailer is almost
> > always incorrect as bdrv_getlength() returns a multiple of the block
> > size (rounded up). This results in a failure to recognize DMG files,
> > resulting in Invalid argument (EINVAL) errors.
> >
> > As there is no API to retrieve the real file size, look for the magic
> > header in the last two sectors to find the start of this 512-byte UDIF
> > trailer (the "koly" block).
> >
> > The resource fork offset ("info_begin") has its offset adjusted as the
> > initial value of offset does not mean "end of file" anymore, but "begin
> > of UDIF trailer".
> >
> > Signed-off-by: Peter Wu <address@hidden>
> > ---
> >   block/dmg.c | 40 ++++++++++++++++++++++++++++++++++++----
> >   1 file changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index e455886..df274f9 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -131,6 +131,39 @@ static void update_max_chunk_size(BDRVDMGState *s, 
> > uint32_t chunk,
> >       }
> >   }
> >
> > +static int64_t dmg_find_koly_offset(BlockDriverState *file_bs)
> > +{
> > +    int64_t length;
> > +    int64_t offset = 0;
> > +    uint8_t buffer[515];
> > +    int i, ret;
> > +
> > +    /* bdrv_getlength returns a multiple of block size (512), rounded up. 
> > Since
> > +     * dmg images can have odd sizes, try to look for the "koly" magic 
> > which
> > +     * marks the begin of the UDIF trailer (512 bytes). This magic can be 
> > found
> > +     * in the last 511 bytes of the second-last sector or the first 4 
> > bytes of
> > +     * the last sector (search space: 515 bytes) */
> > +    length = bdrv_getlength(file_bs);
> > +    if (length < 512) {
> > +        return length < 0 ? length : -EINVAL;
> > +    }
> > +    if (length > 511 + 512) {
> > +        offset = length - 511 - 512;
> > +    }
> > +    length = length < 515 ? length : 515;
> > +    ret = bdrv_pread(file_bs, offset, buffer, length);
> > +    if (ret < 4) {
> > +        return ret < 0 ? ret : -EINVAL;
> > +    }
> > +    for (i = 0; i < length - 3; i++) {
> > +        if (buffer[i] == 'k' && buffer[i+1] == 'o' &&
> > +            buffer[i+2] == 'l' && buffer[i+3] == 'y') {
> > +            return offset + i;
> > +        }
> > +    }
> > +    return -EINVAL;
> > +}
> > +
> >   static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> >                       Error **errp)
> >   {
> > @@ -145,15 +178,14 @@ static int dmg_open(BlockDriverState *bs, QDict 
> > *options, int flags,
> >       s->n_chunks = 0;
> >       s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> >
> > -    /* read offset of info blocks */
> > -    offset = bdrv_getlength(bs->file);
> > +    /* locate the UDIF trailer */
> > +    offset = dmg_find_koly_offset(bs->file);
> >       if (offset < 0) {
> >           ret = offset;
> >           goto fail;
> >       }
> > -    offset -= 0x1d8;
> >
> > -    ret = read_uint64(bs, offset, &info_begin);
> > +    ret = read_uint64(bs, offset + 0x28, &info_begin);
> >       if (ret < 0) {
> >           goto fail;
> >       } else if (info_begin == 0) {
> >
> 
> If there really is no convenient way to retrieve the real length ...
> (Stefan: Would that be difficult to add?)
> 
> Reviewed-by: John Snow <address@hidden>

The real length can be stored, but it takes more effort to do so. See
the stalled work on this bdrv-getlength-conversion branch[1] and in
particular "block: do not directly set total_sectors"[2].
-- 
Kind regards,
Peter
https://lekensteyn.nl

 [1]: https://git.lekensteyn.nl/peter/qemu/log/?h=bdrv-getlength-conversion
 [2]: 
https://git.lekensteyn.nl/peter/qemu/commit/?h=bdrv-getlength-conversion&id=e5164735e5b674a10134894589a060a0f5f32ccc