[Qemu-devel] [PATCH v2 00/12] block/dmg: (compatibility) fixes and bzip2 support

[Qemu-devel] [PATCH v2 00/12] block/dmg: (compatibility) fixes and bzip2 support

[Peter Wu]

Hi,

This is the second revision of improvements to DMG image file support.
See [1] for an overview of the previous patchset.

Thanks to John Snow for his efforts in reviewing patches and providing
suggestions. The errp suggestion from Stefan Hajnoczi is also
incorporated.

An overview of changes since v1 (also mentioned in each patch):

  block/dmg: properly detect the UDIF trailer [+R-b, set errp)
  block/dmg: extract mish block decoding functionality [+R-b, added
        comments, expanded commit message, renamed var]
  block/dmg: extract processing of resource forks [see patch]
  block/dmg: process a buffer instead of reading ints [+R-b, no changes]
  block/dmg: validate chunk size to avoid overflow [added offset check]
  block/dmg: process XML plists [added offset check]
  block/dmg: set virtual size to a non-zero value [fix commit message]
  block/dmg: fix sector data offset calculation [use data provided by file]
  block/dmg: use SectorNumber from BLKX header [new patch]
  block/dmg: factor out block type check [extracted from bzip patch]
  block/dmg: support bzip2 block entry types [set/use BZIP2_LIBS]
  block/dmg: improve zeroes handling [no changes]

(the other length checking patch was squashed into the others)

Note: in the previous patches I mentioned that dmg2img would result in a
shorter file than qemu-img convert. That turns out to be a bug in
dmg2img for which a patch is available[2].

A quick test runner is available at
https://lekensteyn.nl/files/dmg-tests/. This script depends on the fixed
dmg2img program and can then be run as follows:

    QEMU_IMG=/tmp/qout/qemu-img ./run-dmg-tests.sh dmg-images/*.dmg

You will first need some DMG files, I have collected four different
public examples with different properties[1]. These can be found in
urls.txt at https://lekensteyn.nl/files/dmg-tests/dmg-images/.

Kind regards,
Peter

 [1]: https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03606.html
 [2]: https://github.com/Lekensteyn/dmg2img/commit/a1d4861b4b0f2ac5090938233a1156bb130cb3ef

Peter Wu (12):
  block/dmg: properly detect the UDIF trailer
  block/dmg: extract mish block decoding functionality
  block/dmg: extract processing of resource forks
  block/dmg: process a buffer instead of reading ints
  block/dmg: validate chunk size to avoid overflow
  block/dmg: process XML plists
  block/dmg: set virtual size to a non-zero value
  block/dmg: fix sector data offset calculation
  block/dmg: use SectorNumber from BLKX header
  block/dmg: factor out block type check
  block/dmg: support bzip2 block entry types
  block/dmg: improve zeroes handling

 block/Makefile.objs |   1 +
 block/dmg.c         | 503 ++++++++++++++++++++++++++++++++++++++++------------
 configure           |  31 ++++
 3 files changed, 418 insertions(+), 117 deletions(-)

-- 
2.2.1

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

[Peter Wu]

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 <peter@lekensteyn.nl>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 v2: added R-b, set errp as suggested by Stefan Hajnoczi
---
 block/dmg.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index e455886..9183459 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -131,6 +131,48 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
     }
 }
 
+static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
+{
+    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 < 0) {
+        error_setg_errno(errp, -length,
+            "Failed to get file size while reading UDIF trailer");
+        return length;
+    } else if (length < 512) {
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+            "dmg file must be at least 512 bytes long");
+        return -EINVAL;
+    }
+    if (length > 511 + 512) {
+        offset = length - 511 - 512;
+    }
+    length = length < 515 ? length : 515;
+    ret = bdrv_pread(file_bs, offset, buffer, length);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed while reading UDIF trailer");
+        return ret;
+    }
+    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;
+        }
+    }
+    error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+        "Could not locate UDIF trailer in dmg file");
+    return -EINVAL;
+}
+
 static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
@@ -145,15 +187,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, errp);
     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) {
-- 
2.2.1

[Qemu-devel] [PATCH v2 02/12] block/dmg: extract mish block decoding functionality

[Peter Wu]

Extract the mish block decoder such that this can be used for other
formats in the future. A new DmgHeaderState struct is introduced to
share state while decoding.

The code is kept unchanged as much as possible, a "fail" label is added
for example where a simple return would probably do. In dmg_open, the
variable "tmp" is renamed to "rsrc_data_offset" for clarity and comments
have been added explaining various data.

Note that this patch has one subtle difference with the previous
version which should not affect functionality. In the previous code,
the end of a resource was inferred from the mish block (the offsets
would be increased by the fields). In this patch, the resource length
is used instead to avoid the need to rely on the previous offsets.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 v2: added R-b, added comments, renamed "tmp" var to "rsrc_data_offset".
--
 block/dmg.c | 228 +++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 133 insertions(+), 95 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 9183459..e01559f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -173,19 +173,138 @@ static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
     return -EINVAL;
 }
 
+/* used when building the sector table */
+typedef struct DmgHeaderState {
+    /* used internally by dmg_read_mish_block to remember offsets of blocks
+     * across calls */
+    uint64_t last_in_offset;
+    uint64_t last_out_offset;
+    /* exported for dmg_open */
+    uint32_t max_compressed_size;
+    uint32_t max_sectors_per_chunk;
+} DmgHeaderState;
+
+static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
+                               int64_t offset, uint32_t count)
+{
+    BDRVDMGState *s = bs->opaque;
+    uint32_t type, i;
+    int ret;
+    size_t new_size;
+    uint32_t chunk_count;
+
+    ret = read_uint32(bs, offset, &type);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* skip data that is not a valid MISH block (invalid magic or too small) */
+    if (type != 0x6d697368 || count < 244) {
+        /* assume success for now */
+        return 0;
+    }
+
+    offset += 4;
+    offset += 200;
+
+    chunk_count = (count - 204) / 40;
+    new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
+    s->types = g_realloc(s->types, new_size / 2);
+    s->offsets = g_realloc(s->offsets, new_size);
+    s->lengths = g_realloc(s->lengths, new_size);
+    s->sectors = g_realloc(s->sectors, new_size);
+    s->sectorcounts = g_realloc(s->sectorcounts, new_size);
+
+    for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
+        ret = read_uint32(bs, offset, &s->types[i]);
+        if (ret < 0) {
+            goto fail;
+        }
+        offset += 4;
+        if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
+            s->types[i] != 2) {
+            if (s->types[i] == 0xffffffff && i > 0) {
+                ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
+                ds->last_out_offset = s->sectors[i - 1] +
+                                      s->sectorcounts[i - 1];
+            }
+            chunk_count--;
+            i--;
+            offset += 36;
+            continue;
+        }
+        offset += 4;
+
+        ret = read_uint64(bs, offset, &s->sectors[i]);
+        if (ret < 0) {
+            goto fail;
+        }
+        s->sectors[i] += ds->last_out_offset;
+        offset += 8;
+
+        ret = read_uint64(bs, offset, &s->sectorcounts[i]);
+        if (ret < 0) {
+            goto fail;
+        }
+        offset += 8;
+
+        if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+            error_report("sector count %" PRIu64 " for chunk %" PRIu32
+                         " is larger than max (%u)",
+                         s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        ret = read_uint64(bs, offset, &s->offsets[i]);
+        if (ret < 0) {
+            goto fail;
+        }
+        s->offsets[i] += ds->last_in_offset;
+        offset += 8;
+
+        ret = read_uint64(bs, offset, &s->lengths[i]);
+        if (ret < 0) {
+            goto fail;
+        }
+        offset += 8;
+
+        if (s->lengths[i] > DMG_LENGTHS_MAX) {
+            error_report("length %" PRIu64 " for chunk %" PRIu32
+                         " is larger than max (%u)",
+                         s->lengths[i], i, DMG_LENGTHS_MAX);
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        update_max_chunk_size(s, i, &ds->max_compressed_size,
+                              &ds->max_sectors_per_chunk);
+    }
+    s->n_chunks += chunk_count;
+    return 0;
+
+fail:
+    return ret;
+}
+
 static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
     BDRVDMGState *s = bs->opaque;
-    uint64_t info_begin, info_end, last_in_offset, last_out_offset;
-    uint32_t count, tmp;
-    uint32_t max_compressed_size = 1, max_sectors_per_chunk = 1, i;
+    DmgHeaderState ds;
+    uint64_t info_begin, info_end;
+    uint32_t count, rsrc_data_offset;
     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);
@@ -202,10 +321,10 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = read_uint32(bs, info_begin, &tmp);
+    ret = read_uint32(bs, info_begin, &rsrc_data_offset);
     if (ret < 0) {
         goto fail;
-    } else if (tmp != 0x100) {
+    } else if (rsrc_data_offset != 0x100) {
         ret = -EINVAL;
         goto fail;
     }
@@ -217,15 +336,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         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;
 
-    /* read offsets */
-    last_in_offset = last_out_offset = 0;
+    /* read offsets (mish blocks) from one or more resources in resource data */
     while (offset < info_end) {
-        uint32_t type;
-
+        /* size of following resource */
         ret = read_uint32(bs, offset, &count);
         if (ret < 0) {
             goto fail;
@@ -235,100 +354,19 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         }
         offset += 4;
 
-        ret = read_uint32(bs, offset, &type);
+        ret = dmg_read_mish_block(bs, &ds, offset, count);
         if (ret < 0) {
             goto fail;
         }
-
-        if (type == 0x6d697368 && count >= 244) {
-            size_t new_size;
-            uint32_t chunk_count;
-
-            offset += 4;
-            offset += 200;
-
-            chunk_count = (count - 204) / 40;
-            new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
-            s->types = g_realloc(s->types, new_size / 2);
-            s->offsets = g_realloc(s->offsets, new_size);
-            s->lengths = g_realloc(s->lengths, new_size);
-            s->sectors = g_realloc(s->sectors, new_size);
-            s->sectorcounts = g_realloc(s->sectorcounts, new_size);
-
-            for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
-                ret = read_uint32(bs, offset, &s->types[i]);
-                if (ret < 0) {
-                    goto fail;
-                }
-                offset += 4;
-                if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
-                    s->types[i] != 2) {
-                    if (s->types[i] == 0xffffffff && i > 0) {
-                        last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
-                        last_out_offset = s->sectors[i - 1] +
-                                          s->sectorcounts[i - 1];
-                    }
-                    chunk_count--;
-                    i--;
-                    offset += 36;
-                    continue;
-                }
-                offset += 4;
-
-                ret = read_uint64(bs, offset, &s->sectors[i]);
-                if (ret < 0) {
-                    goto fail;
-                }
-                s->sectors[i] += last_out_offset;
-                offset += 8;
-
-                ret = read_uint64(bs, offset, &s->sectorcounts[i]);
-                if (ret < 0) {
-                    goto fail;
-                }
-                offset += 8;
-
-                if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
-                    error_report("sector count %" PRIu64 " for chunk %" PRIu32
-                                 " is larger than max (%u)",
-                                 s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
-                    ret = -EINVAL;
-                    goto fail;
-                }
-
-                ret = read_uint64(bs, offset, &s->offsets[i]);
-                if (ret < 0) {
-                    goto fail;
-                }
-                s->offsets[i] += last_in_offset;
-                offset += 8;
-
-                ret = read_uint64(bs, offset, &s->lengths[i]);
-                if (ret < 0) {
-                    goto fail;
-                }
-                offset += 8;
-
-                if (s->lengths[i] > DMG_LENGTHS_MAX) {
-                    error_report("length %" PRIu64 " for chunk %" PRIu32
-                                 " is larger than max (%u)",
-                                 s->lengths[i], i, DMG_LENGTHS_MAX);
-                    ret = -EINVAL;
-                    goto fail;
-                }
-
-                update_max_chunk_size(s, i, &max_compressed_size,
-                                      &max_sectors_per_chunk);
-            }
-            s->n_chunks += chunk_count;
-        }
+        /* advance offset by size of resource */
+        offset += count;
     }
 
     /* initialize zlib engine */
     s->compressed_chunk = qemu_try_blockalign(bs->file,
-                                              max_compressed_size + 1);
+                                              ds.max_compressed_size + 1);
     s->uncompressed_chunk = qemu_try_blockalign(bs->file,
-                                                512 * max_sectors_per_chunk);
+                                                512 * ds.max_sectors_per_chunk);
     if (s->compressed_chunk == NULL || s->uncompressed_chunk == NULL) {
         ret = -ENOMEM;
         goto fail;
-- 
2.2.1

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

[Peter Wu]

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 <peter@lekensteyn.nl>
---
 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,
-- 
2.2.1

[Qemu-devel] [PATCH v2 04/12] block/dmg: process a buffer instead of reading ints

[Peter Wu]

As the decoded plist XML is not a pointer in the file,
dmg_read_mish_block must be able to process a buffer instead of a file
pointer. Since the full buffer must be processed, let's change the
return value again to just a success flag.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 v2: added R-b
---
 block/dmg.c | 60 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index ed99cf5..4913249 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -100,6 +100,16 @@ static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
     return 0;
 }
 
+static inline uint64_t buff_read_uint64(const uint8_t *buffer, int64_t offset)
+{
+    return be64_to_cpu(*(uint64_t *)&buffer[offset]);
+}
+
+static inline uint32_t buff_read_uint32(const uint8_t *buffer, int64_t offset)
+{
+    return be32_to_cpu(*(uint32_t *)&buffer[offset]);
+}
+
 /* Increase max chunk sizes, if necessary.  This function is used to calculate
  * the buffer sizes needed for compressed/uncompressed chunk I/O.
  */
@@ -184,20 +194,16 @@ typedef struct DmgHeaderState {
     uint32_t max_sectors_per_chunk;
 } DmgHeaderState;
 
-static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
-                               int64_t offset, uint32_t count)
+static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
+                               uint8_t *buffer, uint32_t count)
 {
-    BDRVDMGState *s = bs->opaque;
     uint32_t type, i;
     int ret;
     size_t new_size;
     uint32_t chunk_count;
+    int64_t offset = 0;
 
-    ret = read_uint32(bs, offset, &type);
-    if (ret < 0) {
-        goto fail;
-    }
-
+    type = buff_read_uint32(buffer, offset);
     /* skip data that is not a valid MISH block (invalid magic or too small) */
     if (type != 0x6d697368 || count < 244) {
         /* assume success for now */
@@ -216,10 +222,7 @@ static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
     s->sectorcounts = g_realloc(s->sectorcounts, new_size);
 
     for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
-        ret = read_uint32(bs, offset, &s->types[i]);
-        if (ret < 0) {
-            goto fail;
-        }
+        s->types[i] = buff_read_uint32(buffer, offset);
         offset += 4;
         if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
             s->types[i] != 2) {
@@ -235,17 +238,11 @@ static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
         }
         offset += 4;
 
-        ret = read_uint64(bs, offset, &s->sectors[i]);
-        if (ret < 0) {
-            goto fail;
-        }
+        s->sectors[i] = buff_read_uint64(buffer, offset);
         s->sectors[i] += ds->last_out_offset;
         offset += 8;
 
-        ret = read_uint64(bs, offset, &s->sectorcounts[i]);
-        if (ret < 0) {
-            goto fail;
-        }
+        s->sectorcounts[i] = buff_read_uint64(buffer, offset);
         offset += 8;
 
         if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
@@ -256,17 +253,11 @@ static int dmg_read_mish_block(BlockDriverState *bs, DmgHeaderState *ds,
             goto fail;
         }
 
-        ret = read_uint64(bs, offset, &s->offsets[i]);
-        if (ret < 0) {
-            goto fail;
-        }
+        s->offsets[i] = buff_read_uint64(buffer, offset);
         s->offsets[i] += ds->last_in_offset;
         offset += 8;
 
-        ret = read_uint64(bs, offset, &s->lengths[i]);
-        if (ret < 0) {
-            goto fail;
-        }
+        s->lengths[i] = buff_read_uint64(buffer, offset);
         offset += 8;
 
         if (s->lengths[i] > DMG_LENGTHS_MAX) {
@@ -290,8 +281,10 @@ fail:
 static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
                                   uint64_t info_begin, uint64_t info_length)
 {
+    BDRVDMGState *s = bs->opaque;
     int ret;
     uint32_t count, rsrc_data_offset;
+    uint8_t *buffer = NULL;
     uint64_t info_end;
     uint64_t offset;
 
@@ -332,16 +325,23 @@ static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
         }
         offset += 4;
 
-        ret = dmg_read_mish_block(bs, ds, offset, count);
+        buffer = g_realloc(buffer, count);
+        ret = bdrv_pread(bs->file, offset, buffer, count);
+        if (ret < 0) {
+            goto fail;
+        }
+
+        ret = dmg_read_mish_block(s, ds, buffer, count);
         if (ret < 0) {
             goto fail;
         }
         /* advance offset by size of resource */
         offset += count;
     }
-    return 0;
+    ret = 0;
 
 fail:
+    g_free(buffer);
     return ret;
 }
 
-- 
2.2.1

[Qemu-devel] [PATCH v2 05/12] block/dmg: validate chunk size to avoid overflow

[Peter Wu]

Previously the chunk size was not checked, allowing for a large memory
allocation. This patch checks whether the chunks size is within the
resource fork length, and whether the resource fork is below the
trailer of the dmg file.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v2: added resource fork offset check
---
 block/dmg.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/dmg.c b/block/dmg.c
index 4913249..5f6976b 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -319,7 +319,7 @@ static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
         ret = read_uint32(bs, offset, &count);
         if (ret < 0) {
             goto fail;
-        } else if (count == 0) {
+        } else if (count == 0 || count > info_end - offset) {
             ret = -EINVAL;
             goto fail;
         }
@@ -379,6 +379,11 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     if (ret < 0) {
         goto fail;
     }
+    if (rsrc_fork_offset >= offset ||
+        rsrc_fork_length > offset - rsrc_fork_offset) {
+        ret = -EINVAL;
+        goto fail;
+    }
     if (rsrc_fork_length != 0) {
         ret = dmg_read_resource_fork(bs, &ds,
                                      rsrc_fork_offset, rsrc_fork_length);
-- 
2.2.1

[Qemu-devel] [PATCH v2 06/12] block/dmg: process XML plists

[Peter Wu]

The format is simple enough to avoid using a full-blown XML parser. It
assumes that all BLKX items begin with the "mish" magic word, therefore
it is not a problem if other values get matched which are not a BLKX
block.

The offsets are based on the description at
http://newosxbook.com/DMG.html

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v2: added offset check, allow resource fork to be located at beginning
     of file (removed `rsrc_fork_offset != 0` condition)

It got a `Reviewed-by: John Snow <jsnow@redhat.com>` for v1. I have not
copied the R-b as I wanted to be sure that John agrees with the removal
of the offset != 0 condition.
---
 block/dmg.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/block/dmg.c b/block/dmg.c
index 5f6976b..1a0fa0e 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -26,6 +26,7 @@
 #include "qemu/bswap.h"
 #include "qemu/module.h"
 #include <zlib.h>
+#include <glib.h>
 
 enum {
     /* Limit chunk sizes to prevent unreasonable amounts of memory being used
@@ -345,12 +346,66 @@ fail:
     return ret;
 }
 
+static int dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
+                              uint64_t info_begin, uint64_t info_length)
+{
+    BDRVDMGState *s = bs->opaque;
+    int ret;
+    uint8_t *buffer = NULL;
+    char *data_begin, *data_end;
+
+    /* Have at least some length to avoid NULL for g_malloc. Attempt to set a
+     * safe upper cap on the data length. A test sample had a XML length of
+     * about 1 MiB. */
+    if (info_length == 0 || info_length > 16 * 1024 * 1024) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    buffer = g_malloc(info_length + 1);
+    buffer[info_length] = '\0';
+    ret = bdrv_pread(bs->file, info_begin, buffer, info_length);
+    if (ret != info_length) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* look for <data>...</data>. The data is 284 (0x11c) bytes after base64
+     * decode. The actual data element has 431 (0x1af) bytes which includes tabs
+     * and line feeds. */
+    data_end = (char *)buffer;
+    while ((data_begin = strstr(data_end, "<data>")) != NULL) {
+        gsize out_len = 0;
+
+        data_begin += 6;
+        data_end = strstr(data_begin, "</data>");
+        /* malformed XML? */
+        if (data_end == NULL) {
+            ret = -EINVAL;
+            goto fail;
+        }
+        *data_end++ = '\0';
+        g_base64_decode_inplace(data_begin, &out_len);
+        ret = dmg_read_mish_block(s, ds, (uint8_t *)data_begin,
+                                  (uint32_t)out_len);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
+    ret = 0;
+
+fail:
+    g_free(buffer);
+    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;
+    uint64_t plist_xml_offset, plist_xml_length;
     int64_t offset;
     int ret;
 
@@ -384,12 +439,31 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EINVAL;
         goto fail;
     }
+    /* offset of property list (XMLOffset) */
+    ret = read_uint64(bs, offset + 0xd8, &plist_xml_offset);
+    if (ret < 0) {
+        goto fail;
+    }
+    ret = read_uint64(bs, offset + 0xe0, &plist_xml_length);
+    if (ret < 0) {
+        goto fail;
+    }
+    if (plist_xml_offset >= offset ||
+        plist_xml_length > offset - plist_xml_offset) {
+        ret = -EINVAL;
+        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 if (plist_xml_length != 0) {
+        ret = dmg_read_plist_xml(bs, &ds, plist_xml_offset, plist_xml_length);
+        if (ret < 0) {
+            goto fail;
+        }
     } else {
         ret = -EINVAL;
         goto fail;
-- 
2.2.1

[Qemu-devel] [PATCH v2 07/12] block/dmg: set virtual size to a non-zero value

[Peter Wu]

Right now the virtual size is always reported as zero which makes it
impossible to convert between formats.

After this patch, the number of sectors will be read from the trailer
("koly" block).

To verify the behavior, the output of `dmg2img foo.dmg foo.img` was
compared against `qemu-img convert -f dmg -O raw foo.dmg foo.raw`. The
tests showed that the file contents are exactly the same, except that
QEMU creates a slightly larger file (it matches the total sectors
count).

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v2: fixed typo in commit message (s/mish/koly/)
---
 block/dmg.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/dmg.c b/block/dmg.c
index 1a0fa0e..57feb1b 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -453,6 +453,14 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EINVAL;
         goto fail;
     }
+    ret = read_uint64(bs, offset + 0x1ec, (uint64_t *)&bs->total_sectors);
+    if (ret < 0) {
+        goto fail;
+    }
+    if (bs->total_sectors < 0) {
+        ret = -EINVAL;
+        goto fail;
+    }
     if (rsrc_fork_length != 0) {
         ret = dmg_read_resource_fork(bs, &ds,
                                      rsrc_fork_offset, rsrc_fork_length);
-- 
2.2.1

[Qemu-devel] [PATCH v2 08/12] block/dmg: fix sector data offset calculation

[Peter Wu]

This patch addresses two issues:

 - The data fork offset was not taken into account, resulting in failure
   to read an InstallESD.dmg file (5164763151 bytes) which had a
   non-zero DataForkOffset field.
 - The offset of the previous block ("partition") was unconditionally
   added to the current block because older files would start the input
   offset of a new block at zero. Newer files (including vlc-2.1.5.dmg,
   tuxpaint-0.9.15-macosx.dmg and OS X Yosemite [MAS].dmg) failed in
   reads because these files have chunk offsets, relative to the begin
   of a data fork.

Now the data offset of the mish is taken into account. While we could
check that the data_offset is within the data fork, let's not do that
here as it would only result in parse failures on invalid files (rather
than gracefully handling such bad files). dmg_read will error out if
the offset is incorrect.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v2: use sector and data offset as provided by the BLKX header. This
     allows us to drop last_in_offset. The previous heuristics to detect
     relative offsets is not needed anymore. Squashed the data fork
     offset length check into this patch.
---
 block/dmg.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 57feb1b..130efac 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -188,7 +188,7 @@ static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
 typedef struct DmgHeaderState {
     /* used internally by dmg_read_mish_block to remember offsets of blocks
      * across calls */
-    uint64_t last_in_offset;
+    uint64_t data_fork_offset;
     uint64_t last_out_offset;
     /* exported for dmg_open */
     uint32_t max_compressed_size;
@@ -203,6 +203,8 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
     size_t new_size;
     uint32_t chunk_count;
     int64_t offset = 0;
+    uint64_t data_offset;
+    uint64_t in_offset = ds->data_fork_offset;
 
     type = buff_read_uint32(buffer, offset);
     /* skip data that is not a valid MISH block (invalid magic or too small) */
@@ -211,8 +213,12 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         return 0;
     }
 
-    offset += 4;
-    offset += 200;
+    /* location in data fork for (compressed) blob (in bytes) */
+    data_offset = buff_read_uint64(buffer, offset + 0x18);
+    in_offset += data_offset;
+
+    /* move to begin of chunk entries */
+    offset += 204;
 
     chunk_count = (count - 204) / 40;
     new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
@@ -228,7 +234,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
             s->types[i] != 2) {
             if (s->types[i] == 0xffffffff && i > 0) {
-                ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];
                 ds->last_out_offset = s->sectors[i - 1] +
                                       s->sectorcounts[i - 1];
             }
@@ -255,7 +260,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         }
 
         s->offsets[i] = buff_read_uint64(buffer, offset);
-        s->offsets[i] += ds->last_in_offset;
+        s->offsets[i] += in_offset;
         offset += 8;
 
         s->lengths[i] = buff_read_uint64(buffer, offset);
@@ -413,7 +418,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     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.data_fork_offset = 0;
     ds.last_out_offset = 0;
     ds.max_compressed_size = 1;
     ds.max_sectors_per_chunk = 1;
@@ -425,6 +430,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    /* offset of data fork (DataForkOffset) */
+    ret = read_uint64(bs, offset + 0x18, &ds.data_fork_offset);
+    if (ret < 0) {
+        goto fail;
+    } else if (ds.data_fork_offset > offset) {
+        ret = -EINVAL;
+        goto fail;
+    }
+
     /* offset of resource fork (RsrcForkOffset) */
     ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
     if (ret < 0) {
-- 
2.2.1

[Qemu-devel] [PATCH v2 09/12] block/dmg: use SectorNumber from BLKX header

[Peter Wu]

Previously the sector table parsing relied on the previous offset of
the DMG file. Now it uses the sector number from the BLKX header
(see http://newosxbook.com/DMG.html).

The implementation of dmg2img (from vu1tur) does not base the output
sector on the location of the terminator (0xffffffff) either so it
should be safe to drop this dependency on the previous state.

(It makes somehow makes sense, a terminator should halt further
processing of a block and is perhaps used to preallocate some space.)

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v2: initial patch after suggestions from John Snow to read and use
     these fields.
---
 block/dmg.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 130efac..57922c5 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -189,7 +189,6 @@ typedef struct DmgHeaderState {
     /* used internally by dmg_read_mish_block to remember offsets of blocks
      * across calls */
     uint64_t data_fork_offset;
-    uint64_t last_out_offset;
     /* exported for dmg_open */
     uint32_t max_compressed_size;
     uint32_t max_sectors_per_chunk;
@@ -205,6 +204,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
     int64_t offset = 0;
     uint64_t data_offset;
     uint64_t in_offset = ds->data_fork_offset;
+    uint64_t out_offset;
 
     type = buff_read_uint32(buffer, offset);
     /* skip data that is not a valid MISH block (invalid magic or too small) */
@@ -213,6 +213,9 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         return 0;
     }
 
+    /* chunk offsets are relative to this sector number */
+    out_offset = buff_read_uint64(buffer, offset + 8);
+
     /* location in data fork for (compressed) blob (in bytes) */
     data_offset = buff_read_uint64(buffer, offset + 0x18);
     in_offset += data_offset;
@@ -233,10 +236,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         offset += 4;
         if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
             s->types[i] != 2) {
-            if (s->types[i] == 0xffffffff && i > 0) {
-                ds->last_out_offset = s->sectors[i - 1] +
-                                      s->sectorcounts[i - 1];
-            }
             chunk_count--;
             i--;
             offset += 36;
@@ -245,7 +244,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         offset += 4;
 
         s->sectors[i] = buff_read_uint64(buffer, offset);
-        s->sectors[i] += ds->last_out_offset;
+        s->sectors[i] += out_offset;
         offset += 8;
 
         s->sectorcounts[i] = buff_read_uint64(buffer, offset);
@@ -419,7 +418,6 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     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.data_fork_offset = 0;
-    ds.last_out_offset = 0;
     ds.max_compressed_size = 1;
     ds.max_sectors_per_chunk = 1;
 
-- 
2.2.1

[Qemu-devel] [PATCH v2 10/12] block/dmg: factor out block type check

[Peter Wu]

In preparation for adding bzip2 support, split the type check into a
separate function. Make all offsets relative to the begin of a chunk
such that it is easier to recognize the position without having to
add up all offsets. Some comments are added to describe the fields.

There is no functional change.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v2: new patch, split off bzip2 patch. Besides the
     dmg_is_known_block_type function, the offsets are now also made
     relative.
---
 block/dmg.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 57922c5..b1d0930 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -194,6 +194,18 @@ typedef struct DmgHeaderState {
     uint32_t max_sectors_per_chunk;
 } DmgHeaderState;
 
+static bool dmg_is_known_block_type(uint32_t entry_type)
+{
+    switch (entry_type) {
+    case 0x00000001:    /* uncompressed */
+    case 0x00000002:    /* zeroes */
+    case 0x80000005:    /* zlib */
+        return true;
+    default:
+        return false;
+    }
+}
+
 static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
                                uint8_t *buffer, uint32_t count)
 {
@@ -233,22 +245,19 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
 
     for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
         s->types[i] = buff_read_uint32(buffer, offset);
-        offset += 4;
-        if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
-            s->types[i] != 2) {
+        if (!dmg_is_known_block_type(s->types[i])) {
             chunk_count--;
             i--;
-            offset += 36;
+            offset += 40;
             continue;
         }
-        offset += 4;
 
-        s->sectors[i] = buff_read_uint64(buffer, offset);
+        /* sector number */
+        s->sectors[i] = buff_read_uint64(buffer, offset + 8);
         s->sectors[i] += out_offset;
-        offset += 8;
 
-        s->sectorcounts[i] = buff_read_uint64(buffer, offset);
-        offset += 8;
+        /* sector count */
+        s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
 
         if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
             error_report("sector count %" PRIu64 " for chunk %" PRIu32
@@ -258,12 +267,12 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
             goto fail;
         }
 
-        s->offsets[i] = buff_read_uint64(buffer, offset);
+        /* offset in (compressed) data fork */
+        s->offsets[i] = buff_read_uint64(buffer, offset + 0x18);
         s->offsets[i] += in_offset;
-        offset += 8;
 
-        s->lengths[i] = buff_read_uint64(buffer, offset);
-        offset += 8;
+        /* length in (compressed) data fork */
+        s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
 
         if (s->lengths[i] > DMG_LENGTHS_MAX) {
             error_report("length %" PRIu64 " for chunk %" PRIu32
@@ -275,6 +284,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
 
         update_max_chunk_size(s, i, &ds->max_compressed_size,
                               &ds->max_sectors_per_chunk);
+        offset += 40;
     }
     s->n_chunks += chunk_count;
     return 0;
-- 
2.2.1

[Qemu-devel] [PATCH v2 11/12] block/dmg: support bzip2 block entry types

[Peter Wu]

This patch adds support for bzip2-compressed block entries as introduced
with OS X 10.4 (source: https://en.wikipedia.org/wiki/Apple_Disk_Image).

It was tested against a 5.2G "OS X Yosemite" installation image which
stores the BLXX block in the XML property list (instead of resource
forks) and has over 5k chunks.

New configure entries are added (--enable-bzip2 / --disable-bzip2) to
control inclusion of bzip2 functionality (which requires linking against
libbz2). The help message suggests that this option is needed for DMG
files, but the tests are generic enough that other parts of QEMU can use
bzip2 if needed.

The identifiers are based on http://newosxbook.com/DMG.html.

The decompression routines are based on the zlib case, but as there is
no way to reset the decompression state (unlike zlib), memory is
allocated and deallocated for every decompression. This should not be
problematic as the decompression takes most of the time and as blocks
are typically about/over 1 MiB in size, only one allocation is done
every 2000 sectors.

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v2: split block type check into a different patch ("[PATCH v2 10/12]
     block/dmg: factor out block type check").
     Add BZIP2_LIBS instead of polluting libs_softmmu with -lbz2 (which
     would also end up in fsdev/virtfs-proxy-helper).
     Fix unused variable warning with --disable-bzip2.
---
 block/Makefile.objs |  1 +
 block/dmg.c         | 43 ++++++++++++++++++++++++++++++++++++++++++-
 configure           | 31 +++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 04b0e43..e7ea07c 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -36,5 +36,6 @@ gluster.o-libs     := $(GLUSTERFS_LIBS)
 ssh.o-cflags       := $(LIBSSH2_CFLAGS)
 ssh.o-libs         := $(LIBSSH2_LIBS)
 archipelago.o-libs := $(ARCHIPELAGO_LIBS)
+dmg.o-libs         := $(BZIP2_LIBS)
 qcow.o-libs        := -lz
 linux-aio.o-libs   := -laio
diff --git a/block/dmg.c b/block/dmg.c
index b1d0930..8239221 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -26,6 +26,9 @@
 #include "qemu/bswap.h"
 #include "qemu/module.h"
 #include <zlib.h>
+#ifdef CONFIG_BZIP2
+#include <bzlib.h>
+#endif
 #include <glib.h>
 
 enum {
@@ -56,6 +59,9 @@ typedef struct BDRVDMGState {
     uint8_t *compressed_chunk;
     uint8_t *uncompressed_chunk;
     z_stream zstream;
+#ifdef CONFIG_BZIP2
+    bz_stream bzstream;
+#endif
 } BDRVDMGState;
 
 static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
@@ -123,6 +129,7 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
 
     switch (s->types[chunk]) {
     case 0x80000005: /* zlib compressed */
+    case 0x80000006: /* bzip2 compressed */
         compressed_size = s->lengths[chunk];
         uncompressed_sectors = s->sectorcounts[chunk];
         break;
@@ -200,6 +207,9 @@ static bool dmg_is_known_block_type(uint32_t entry_type)
     case 0x00000001:    /* uncompressed */
     case 0x00000002:    /* zeroes */
     case 0x80000005:    /* zlib */
+#ifdef CONFIG_BZIP2
+    case 0x80000006:    /* bzip2 */
+#endif
         return true;
     default:
         return false;
@@ -565,13 +575,16 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
     if (!is_sector_in_chunk(s, s->current_chunk, sector_num)) {
         int ret;
         uint32_t chunk = search_chunk(s, sector_num);
+#ifdef CONFIG_BZIP2
+        uint64_t total_out;
+#endif
 
         if (chunk >= s->n_chunks) {
             return -1;
         }
 
         s->current_chunk = s->n_chunks;
-        switch (s->types[chunk]) {
+        switch (s->types[chunk]) { /* block entry type */
         case 0x80000005: { /* zlib compressed */
             /* we need to buffer, because only the chunk as whole can be
              * inflated. */
@@ -595,6 +608,34 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
                 return -1;
             }
             break; }
+#ifdef CONFIG_BZIP2
+        case 0x80000006: /* bzip2 compressed */
+            /* we need to buffer, because only the chunk as whole can be
+             * inflated. */
+            ret = bdrv_pread(bs->file, s->offsets[chunk],
+                             s->compressed_chunk, s->lengths[chunk]);
+            if (ret != s->lengths[chunk]) {
+                return -1;
+            }
+
+            ret = BZ2_bzDecompressInit(&s->bzstream, 0, 0);
+            if (ret != BZ_OK) {
+                return -1;
+            }
+            s->bzstream.next_in = (char *)s->compressed_chunk;
+            s->bzstream.avail_in = (unsigned int) s->lengths[chunk];
+            s->bzstream.next_out = (char *)s->uncompressed_chunk;
+            s->bzstream.avail_out = (unsigned int) 512 * s->sectorcounts[chunk];
+            ret = BZ2_bzDecompress(&s->bzstream);
+            total_out = ((uint64_t)s->bzstream.total_out_hi32 << 32) +
+                        s->bzstream.total_out_lo32;
+            BZ2_bzDecompressEnd(&s->bzstream);
+            if (ret != BZ_STREAM_END ||
+                total_out != 512 * s->sectorcounts[chunk]) {
+                return -1;
+            }
+            break;
+#endif /* CONFIG_BZIP2 */
         case 1: /* copy */
             ret = bdrv_pread(bs->file, s->offsets[chunk],
                              s->uncompressed_chunk, s->lengths[chunk]);
diff --git a/configure b/configure
index cae588c..650bb13 100755
--- a/configure
+++ b/configure
@@ -313,6 +313,7 @@ glx=""
 zlib="yes"
 lzo=""
 snappy=""
+bzip2=""
 guest_agent=""
 guest_agent_with_vss="no"
 vss_win32_sdk=""
@@ -1060,6 +1061,10 @@ for opt do
   ;;
   --enable-snappy) snappy="yes"
   ;;
+  --disable-bzip2) bzip2="no"
+  ;;
+  --enable-bzip2) bzip2="yes"
+  ;;
   --enable-guest-agent) guest_agent="yes"
   ;;
   --disable-guest-agent) guest_agent="no"
@@ -1374,6 +1379,8 @@ Advanced options (experts only):
   --enable-usb-redir       enable usb network redirection support
   --enable-lzo             enable the support of lzo compression library
   --enable-snappy          enable the support of snappy compression library
+  --enable-bzip2           enable the support of bzip2 compression library (for
+                           reading bzip2-compressed dmg images)
   --disable-guest-agent    disable building of the QEMU Guest Agent
   --enable-guest-agent     enable building of the QEMU Guest Agent
   --with-vss-sdk=SDK-path  enable Windows VSS support in QEMU Guest Agent
@@ -1820,6 +1827,24 @@ EOF
 fi
 
 ##########################################
+# bzip2 check
+
+if test "$bzip2" != "no" ; then
+    cat > $TMPC << EOF
+#include <bzlib.h>
+int main(void) { BZ2_bzlibVersion(); return 0; }
+EOF
+    if compile_prog "" "-lbz2" ; then
+        bzip2="yes"
+    else
+        if test "$bzip2" = "yes"; then
+            feature_not_found "libbzip2" "Install libbzip2 devel"
+        fi
+        bzip2="no"
+    fi
+fi
+
+##########################################
 # libseccomp check
 
 if test "$seccomp" != "no" ; then
@@ -4340,6 +4365,7 @@ echo "vhdx              $vhdx"
 echo "Quorum            $quorum"
 echo "lzo support       $lzo"
 echo "snappy support    $snappy"
+echo "bzip2 support     $bzip2"
 echo "NUMA host support $numa"
 
 if test "$sdl_too_old" = "yes"; then
@@ -4695,6 +4721,11 @@ if test "$snappy" = "yes" ; then
   echo "CONFIG_SNAPPY=y" >> $config_host_mak
 fi
 
+if test "$bzip2" = "yes" ; then
+  echo "CONFIG_BZIP2=y" >> $config_host_mak
+  echo "BZIP2_LIBS=-lbz2" >> $config_host_mak
+fi
+
 if test "$libiscsi" = "yes" ; then
   echo "CONFIG_LIBISCSI=m" >> $config_host_mak
   echo "LIBISCSI_CFLAGS=$libiscsi_cflags" >> $config_host_mak
-- 
2.2.1

[Qemu-devel] [PATCH v2 12/12] block/dmg: improve zeroes handling

[Peter Wu]

Disk images may contain large all-zeroes gaps (1.66k sectors or 812 MiB
is seen in the real world). These blocks (type 2) do not need to be
extracted into a temporary buffer, there is no need to allocate memory
for these blocks nor to check its length.

(For the test image, the maximum uncompressed size is 1054371 bytes,
probably for a bzip2-compressed block.)

Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
 v2: no changes (did not receive comments last time)
---
 block/dmg.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 8239221..4e24076 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -137,7 +137,9 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
         uncompressed_sectors = (s->lengths[chunk] + 511) / 512;
         break;
     case 2: /* zero */
-        uncompressed_sectors = s->sectorcounts[chunk];
+        /* as the all-zeroes block may be large, it is treated specially: the
+         * sector is not copied from a large buffer, a simple memset is used
+         * instead. Therefore uncompressed_sectors does not need to be set. */
         break;
     }
 
@@ -269,7 +271,9 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         /* sector count */
         s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
 
-        if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+        /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
+         * therefore be unbounded. */
+        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
             error_report("sector count %" PRIu64 " for chunk %" PRIu32
                          " is larger than max (%u)",
                          s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
@@ -644,7 +648,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
             }
             break;
         case 2: /* zero */
-            memset(s->uncompressed_chunk, 0, 512 * s->sectorcounts[chunk]);
+            /* see dmg_read, it is treated specially. No buffer needs to be
+             * pre-filled, the zeroes can be set directly. */
             break;
         }
         s->current_chunk = chunk;
@@ -663,6 +668,13 @@ static int dmg_read(BlockDriverState *bs, int64_t sector_num,
         if (dmg_read_chunk(bs, sector_num + i) != 0) {
             return -1;
         }
+        /* Special case: current chunk is all zeroes. Do not perform a memcpy as
+         * s->uncompressed_chunk may be too small to cover the large all-zeroes
+         * section. dmg_read_chunk is called to find s->current_chunk */
+        if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
+            memset(buf + i * 512, 0, 512);
+            continue;
+        }
         sector_offset_in_chunk = sector_num + i - s->sectors[s->current_chunk];
         memcpy(buf + i * 512,
                s->uncompressed_chunk + sector_offset_in_chunk * 512, 512);
-- 
2.2.1

Re: [Qemu-devel] [PATCH v2 11/12] block/dmg: support bzip2 block entry types

[Paolo Bonzini]

On 06/01/2015 18:48, Peter Wu wrote:
>  v2: split block type check into a different patch ("[PATCH v2 10/12]
>      block/dmg: factor out block type check").
>      Add BZIP2_LIBS instead of polluting libs_softmmu with -lbz2 (which
>      would also end up in fsdev/virtfs-proxy-helper).
>      Fix unused variable warning with --disable-bzip2.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2493 bytes --]

On Tue, Jan 06, 2015 at 06:48:04PM +0100, 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 <peter@lekensteyn.nl>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  v2: added R-b, set errp as suggested by Stefan Hajnoczi
> ---
>  block/dmg.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/block/dmg.c b/block/dmg.c
> index e455886..9183459 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -131,6 +131,48 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
>      }
>  }
>  
> +static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
> +{
> +    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 < 0) {
> +        error_setg_errno(errp, -length,
> +            "Failed to get file size while reading UDIF trailer");
> +        return length;
> +    } else if (length < 512) {
> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +            "dmg file must be at least 512 bytes long");
> +        return -EINVAL;

Not worth respinning but error_setg() is a shortcut for error_set(errp,
ERROR_CLASS_GENERIC_ERROR, fmt, ...).

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Peter Wu]

On Wednesday 07 January 2015 13:19:34 Stefan Hajnoczi wrote:
> On Tue, Jan 06, 2015 at 06:48:04PM +0100, 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 <peter@lekensteyn.nl>
> > Reviewed-by: John Snow <jsnow@redhat.com>
> > ---
> >  v2: added R-b, set errp as suggested by Stefan Hajnoczi
> > ---
> >  block/dmg.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 45 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/dmg.c b/block/dmg.c
> > index e455886..9183459 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -131,6 +131,48 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
> >      }
> >  }
> >  
> > +static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
> > +{
> > +    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 < 0) {
> > +        error_setg_errno(errp, -length,
> > +            "Failed to get file size while reading UDIF trailer");
> > +        return length;
> > +    } else if (length < 512) {
> > +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> > +            "dmg file must be at least 512 bytes long");
> > +        return -EINVAL;
> 
> Not worth respinning but error_setg() is a shortcut for error_set(errp,
> ERROR_CLASS_GENERIC_ERROR, fmt, ...).
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Good to know, I got a compile error when ERROR_CLASS_GENERIC_ERROR was
omitted, didn't think of using error_setg instead.

When merging, please replace the above ERROR_CLASS_GENERIC_ERROR by:

        error_setg(errp, "dmg file must be at least 512 bytes long");

(and likewise for the last error_set in the function)
-- 
Kind regards,
Peter
https://lekensteyn.nl

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

[John Snow]

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 <peter@lekensteyn.nl>
> ---
>   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 <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 05/12] block/dmg: validate chunk size to avoid overflow

[John Snow]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> Previously the chunk size was not checked, allowing for a large memory
> allocation. This patch checks whether the chunks size is within the
> resource fork length, and whether the resource fork is below the
> trailer of the dmg file.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: added resource fork offset check
> ---
>   block/dmg.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 4913249..5f6976b 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -319,7 +319,7 @@ static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
>           ret = read_uint32(bs, offset, &count);
>           if (ret < 0) {
>               goto fail;
> -        } else if (count == 0) {
> +        } else if (count == 0 || count > info_end - offset) {
>               ret = -EINVAL;
>               goto fail;
>           }
> @@ -379,6 +379,11 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>       if (ret < 0) {
>           goto fail;
>       }
> +    if (rsrc_fork_offset >= offset ||
> +        rsrc_fork_length > offset - rsrc_fork_offset) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
>       if (rsrc_fork_length != 0) {
>           ret = dmg_read_resource_fork(bs, &ds,
>                                        rsrc_fork_offset, rsrc_fork_length);
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 06/12] block/dmg: process XML plists

[John Snow]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> The format is simple enough to avoid using a full-blown XML parser. It
> assumes that all BLKX items begin with the "mish" magic word, therefore
> it is not a problem if other values get matched which are not a BLKX
> block.
>
> The offsets are based on the description at
> http://newosxbook.com/DMG.html
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: added offset check, allow resource fork to be located at beginning
>       of file (removed `rsrc_fork_offset != 0` condition)
>
> It got a `Reviewed-by: John Snow <jsnow@redhat.com>` for v1. I have not
> copied the R-b as I wanted to be sure that John agrees with the removal
> of the offset != 0 condition.

It makes sense to me, but as I don't really have an authoritative 
standard to follow, this really relies on "This didn't break anything 
obvious."

I think you have more images than I do, and as long as you tested them 
again under v2:

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>   block/dmg.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 74 insertions(+)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 5f6976b..1a0fa0e 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -26,6 +26,7 @@
>   #include "qemu/bswap.h"
>   #include "qemu/module.h"
>   #include <zlib.h>
> +#include <glib.h>
>
>   enum {
>       /* Limit chunk sizes to prevent unreasonable amounts of memory being used
> @@ -345,12 +346,66 @@ fail:
>       return ret;
>   }
>
> +static int dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
> +                              uint64_t info_begin, uint64_t info_length)
> +{
> +    BDRVDMGState *s = bs->opaque;
> +    int ret;
> +    uint8_t *buffer = NULL;
> +    char *data_begin, *data_end;
> +
> +    /* Have at least some length to avoid NULL for g_malloc. Attempt to set a
> +     * safe upper cap on the data length. A test sample had a XML length of
> +     * about 1 MiB. */
> +    if (info_length == 0 || info_length > 16 * 1024 * 1024) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    buffer = g_malloc(info_length + 1);
> +    buffer[info_length] = '\0';
> +    ret = bdrv_pread(bs->file, info_begin, buffer, info_length);
> +    if (ret != info_length) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    /* look for <data>...</data>. The data is 284 (0x11c) bytes after base64
> +     * decode. The actual data element has 431 (0x1af) bytes which includes tabs
> +     * and line feeds. */
> +    data_end = (char *)buffer;
> +    while ((data_begin = strstr(data_end, "<data>")) != NULL) {
> +        gsize out_len = 0;
> +
> +        data_begin += 6;
> +        data_end = strstr(data_begin, "</data>");
> +        /* malformed XML? */
> +        if (data_end == NULL) {
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +        *data_end++ = '\0';
> +        g_base64_decode_inplace(data_begin, &out_len);
> +        ret = dmg_read_mish_block(s, ds, (uint8_t *)data_begin,
> +                                  (uint32_t)out_len);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +    }
> +    ret = 0;
> +
> +fail:
> +    g_free(buffer);
> +    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;
> +    uint64_t plist_xml_offset, plist_xml_length;
>       int64_t offset;
>       int ret;
>
> @@ -384,12 +439,31 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>           ret = -EINVAL;
>           goto fail;
>       }
> +    /* offset of property list (XMLOffset) */
> +    ret = read_uint64(bs, offset + 0xd8, &plist_xml_offset);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    ret = read_uint64(bs, offset + 0xe0, &plist_xml_length);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    if (plist_xml_offset >= offset ||
> +        plist_xml_length > offset - plist_xml_offset) {
> +        ret = -EINVAL;
> +        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 if (plist_xml_length != 0) {
> +        ret = dmg_read_plist_xml(bs, &ds, plist_xml_offset, plist_xml_length);
> +        if (ret < 0) {
> +            goto fail;
> +        }
>       } else {
>           ret = -EINVAL;
>           goto fail;
>

Re: [Qemu-devel] [PATCH v2 07/12] block/dmg: set virtual size to a non-zero value

[John Snow]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> Right now the virtual size is always reported as zero which makes it
> impossible to convert between formats.
>
> After this patch, the number of sectors will be read from the trailer
> ("koly" block).
>
> To verify the behavior, the output of `dmg2img foo.dmg foo.img` was
> compared against `qemu-img convert -f dmg -O raw foo.dmg foo.raw`. The
> tests showed that the file contents are exactly the same, except that
> QEMU creates a slightly larger file (it matches the total sectors
> count).
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: fixed typo in commit message (s/mish/koly/)
> ---
>   block/dmg.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 1a0fa0e..57feb1b 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -453,6 +453,14 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>           ret = -EINVAL;
>           goto fail;
>       }
> +    ret = read_uint64(bs, offset + 0x1ec, (uint64_t *)&bs->total_sectors);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    if (bs->total_sectors < 0) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
>       if (rsrc_fork_length != 0) {
>           ret = dmg_read_resource_fork(bs, &ds,
>                                        rsrc_fork_offset, rsrc_fork_length);
>

You may want to adjust the commit message here to reflect the 
information in your new cover letter regarding the bug in dmg2img, but 
that's not worth re-spinning for.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 08/12] block/dmg: fix sector data offset calculation

[John Snow]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> This patch addresses two issues:
>
>   - The data fork offset was not taken into account, resulting in failure
>     to read an InstallESD.dmg file (5164763151 bytes) which had a
>     non-zero DataForkOffset field.
>   - The offset of the previous block ("partition") was unconditionally
>     added to the current block because older files would start the input
>     offset of a new block at zero. Newer files (including vlc-2.1.5.dmg,
>     tuxpaint-0.9.15-macosx.dmg and OS X Yosemite [MAS].dmg) failed in
>     reads because these files have chunk offsets, relative to the begin
>     of a data fork.
>
> Now the data offset of the mish is taken into account. While we could
> check that the data_offset is within the data fork, let's not do that
> here as it would only result in parse failures on invalid files (rather
> than gracefully handling such bad files). dmg_read will error out if
> the offset is incorrect.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: use sector and data offset as provided by the BLKX header. This
>       allows us to drop last_in_offset. The previous heuristics to detect
>       relative offsets is not needed anymore. Squashed the data fork
>       offset length check into this patch.
> ---
>   block/dmg.c | 26 ++++++++++++++++++++------
>   1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 57feb1b..130efac 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -188,7 +188,7 @@ static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
>   typedef struct DmgHeaderState {
>       /* used internally by dmg_read_mish_block to remember offsets of blocks
>        * across calls */
> -    uint64_t last_in_offset;
> +    uint64_t data_fork_offset;
>       uint64_t last_out_offset;
>       /* exported for dmg_open */
>       uint32_t max_compressed_size;
> @@ -203,6 +203,8 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>       size_t new_size;
>       uint32_t chunk_count;
>       int64_t offset = 0;
> +    uint64_t data_offset;
> +    uint64_t in_offset = ds->data_fork_offset;
>
>       type = buff_read_uint32(buffer, offset);
>       /* skip data that is not a valid MISH block (invalid magic or too small) */
> @@ -211,8 +213,12 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           return 0;
>       }
>
> -    offset += 4;
> -    offset += 200;
> +    /* location in data fork for (compressed) blob (in bytes) */
> +    data_offset = buff_read_uint64(buffer, offset + 0x18);
> +    in_offset += data_offset;
> +
> +    /* move to begin of chunk entries */
> +    offset += 204;
>
>       chunk_count = (count - 204) / 40;
>       new_size = sizeof(uint64_t) * (s->n_chunks + chunk_count);
> @@ -228,7 +234,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
>               s->types[i] != 2) {
>               if (s->types[i] == 0xffffffff && i > 0) {
> -                ds->last_in_offset = s->offsets[i - 1] + s->lengths[i - 1];

Great!

>                   ds->last_out_offset = s->sectors[i - 1] +
>                                         s->sectorcounts[i - 1];
>               }
> @@ -255,7 +260,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           }
>
>           s->offsets[i] = buff_read_uint64(buffer, offset);
> -        s->offsets[i] += ds->last_in_offset;
> +        s->offsets[i] += in_offset;
>           offset += 8;
>
>           s->lengths[i] = buff_read_uint64(buffer, offset);
> @@ -413,7 +418,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>       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.data_fork_offset = 0;
>       ds.last_out_offset = 0;
>       ds.max_compressed_size = 1;
>       ds.max_sectors_per_chunk = 1;
> @@ -425,6 +430,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>           goto fail;
>       }
>
> +    /* offset of data fork (DataForkOffset) */
> +    ret = read_uint64(bs, offset + 0x18, &ds.data_fork_offset);
> +    if (ret < 0) {
> +        goto fail;
> +    } else if (ds.data_fork_offset > offset) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>       /* offset of resource fork (RsrcForkOffset) */
>       ret = read_uint64(bs, offset + 0x28, &rsrc_fork_offset);
>       if (ret < 0) {
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 09/12] block/dmg: use SectorNumber from BLKX header

[John Snow]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> Previously the sector table parsing relied on the previous offset of
> the DMG file. Now it uses the sector number from the BLKX header
> (see http://newosxbook.com/DMG.html).
>
> The implementation of dmg2img (from vu1tur) does not base the output
> sector on the location of the terminator (0xffffffff) either so it
> should be safe to drop this dependency on the previous state.
>
> (It makes somehow makes sense, a terminator should halt further
> processing of a block and is perhaps used to preallocate some space.)
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: initial patch after suggestions from John Snow to read and use
>       these fields.
> ---
>   block/dmg.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 130efac..57922c5 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -189,7 +189,6 @@ typedef struct DmgHeaderState {
>       /* used internally by dmg_read_mish_block to remember offsets of blocks
>        * across calls */
>       uint64_t data_fork_offset;
> -    uint64_t last_out_offset;
>       /* exported for dmg_open */
>       uint32_t max_compressed_size;
>       uint32_t max_sectors_per_chunk;
> @@ -205,6 +204,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>       int64_t offset = 0;
>       uint64_t data_offset;
>       uint64_t in_offset = ds->data_fork_offset;
> +    uint64_t out_offset;
>
>       type = buff_read_uint32(buffer, offset);
>       /* skip data that is not a valid MISH block (invalid magic or too small) */
> @@ -213,6 +213,9 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           return 0;
>       }
>
> +    /* chunk offsets are relative to this sector number */
> +    out_offset = buff_read_uint64(buffer, offset + 8);
> +
>       /* location in data fork for (compressed) blob (in bytes) */
>       data_offset = buff_read_uint64(buffer, offset + 0x18);
>       in_offset += data_offset;
> @@ -233,10 +236,6 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           offset += 4;
>           if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
>               s->types[i] != 2) {
> -            if (s->types[i] == 0xffffffff && i > 0) {
> -                ds->last_out_offset = s->sectors[i - 1] +
> -                                      s->sectorcounts[i - 1];
> -            }

This is exactly what I was hoping to see could be eliminated by reading 
the mish headers. Thanks for this.

>               chunk_count--;
>               i--;
>               offset += 36;
> @@ -245,7 +244,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           offset += 4;
>
>           s->sectors[i] = buff_read_uint64(buffer, offset);
> -        s->sectors[i] += ds->last_out_offset;
> +        s->sectors[i] += out_offset;
>           offset += 8;
>
>           s->sectorcounts[i] = buff_read_uint64(buffer, offset);
> @@ -419,7 +418,6 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>       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.data_fork_offset = 0;
> -    ds.last_out_offset = 0;
>       ds.max_compressed_size = 1;
>       ds.max_sectors_per_chunk = 1;
>
>

Much nicer.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 10/12] block/dmg: factor out block type check

[John Snow]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> In preparation for adding bzip2 support, split the type check into a
> separate function. Make all offsets relative to the begin of a chunk
> such that it is easier to recognize the position without having to
> add up all offsets. Some comments are added to describe the fields.
>
> There is no functional change.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: new patch, split off bzip2 patch. Besides the
>       dmg_is_known_block_type function, the offsets are now also made
>       relative.
> ---
>   block/dmg.c | 36 +++++++++++++++++++++++-------------
>   1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 57922c5..b1d0930 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -194,6 +194,18 @@ typedef struct DmgHeaderState {
>       uint32_t max_sectors_per_chunk;
>   } DmgHeaderState;
>
> +static bool dmg_is_known_block_type(uint32_t entry_type)
> +{
> +    switch (entry_type) {
> +    case 0x00000001:    /* uncompressed */
> +    case 0x00000002:    /* zeroes */
> +    case 0x80000005:    /* zlib */
> +        return true;
> +    default:
> +        return false;
> +    }
> +}
> +
>   static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>                                  uint8_t *buffer, uint32_t count)
>   {
> @@ -233,22 +245,19 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>
>       for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
>           s->types[i] = buff_read_uint32(buffer, offset);
> -        offset += 4;
> -        if (s->types[i] != 0x80000005 && s->types[i] != 1 &&
> -            s->types[i] != 2) {
> +        if (!dmg_is_known_block_type(s->types[i])) {
>               chunk_count--;
>               i--;
> -            offset += 36;
> +            offset += 40;
>               continue;
>           }
> -        offset += 4;
>
> -        s->sectors[i] = buff_read_uint64(buffer, offset);
> +        /* sector number */
> +        s->sectors[i] = buff_read_uint64(buffer, offset + 8);
>           s->sectors[i] += out_offset;
> -        offset += 8;
>
> -        s->sectorcounts[i] = buff_read_uint64(buffer, offset);
> -        offset += 8;
> +        /* sector count */
> +        s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
>
>           if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
>               error_report("sector count %" PRIu64 " for chunk %" PRIu32
> @@ -258,12 +267,12 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>               goto fail;
>           }
>
> -        s->offsets[i] = buff_read_uint64(buffer, offset);
> +        /* offset in (compressed) data fork */
> +        s->offsets[i] = buff_read_uint64(buffer, offset + 0x18);
>           s->offsets[i] += in_offset;
> -        offset += 8;
>
> -        s->lengths[i] = buff_read_uint64(buffer, offset);
> -        offset += 8;
> +        /* length in (compressed) data fork */
> +        s->lengths[i] = buff_read_uint64(buffer, offset + 0x20);
>
>           if (s->lengths[i] > DMG_LENGTHS_MAX) {
>               error_report("length %" PRIu64 " for chunk %" PRIu32
> @@ -275,6 +284,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>
>           update_max_chunk_size(s, i, &ds->max_compressed_size,
>                                 &ds->max_sectors_per_chunk);
> +        offset += 40;
>       }
>       s->n_chunks += chunk_count;
>       return 0;
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 11/12] block/dmg: support bzip2 block entry types

[John Snow]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> This patch adds support for bzip2-compressed block entries as introduced
> with OS X 10.4 (source: https://en.wikipedia.org/wiki/Apple_Disk_Image).
>
> It was tested against a 5.2G "OS X Yosemite" installation image which
> stores the BLXX block in the XML property list (instead of resource
> forks) and has over 5k chunks.
>
> New configure entries are added (--enable-bzip2 / --disable-bzip2) to
> control inclusion of bzip2 functionality (which requires linking against
> libbz2). The help message suggests that this option is needed for DMG
> files, but the tests are generic enough that other parts of QEMU can use
> bzip2 if needed.
>
> The identifiers are based on http://newosxbook.com/DMG.html.
>
> The decompression routines are based on the zlib case, but as there is
> no way to reset the decompression state (unlike zlib), memory is
> allocated and deallocated for every decompression. This should not be
> problematic as the decompression takes most of the time and as blocks
> are typically about/over 1 MiB in size, only one allocation is done
> every 2000 sectors.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: split block type check into a different patch ("[PATCH v2 10/12]
>       block/dmg: factor out block type check").
>       Add BZIP2_LIBS instead of polluting libs_softmmu with -lbz2 (which
>       would also end up in fsdev/virtfs-proxy-helper).
>       Fix unused variable warning with --disable-bzip2.
> ---
>   block/Makefile.objs |  1 +
>   block/dmg.c         | 43 ++++++++++++++++++++++++++++++++++++++++++-
>   configure           | 31 +++++++++++++++++++++++++++++++
>   3 files changed, 74 insertions(+), 1 deletion(-)
>

[snip]

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 12/12] block/dmg: improve zeroes handling

[John Snow]

On 01/06/2015 12:48 PM, Peter Wu wrote:
> Disk images may contain large all-zeroes gaps (1.66k sectors or 812 MiB
> is seen in the real world). These blocks (type 2) do not need to be
> extracted into a temporary buffer, there is no need to allocate memory
> for these blocks nor to check its length.
>
> (For the test image, the maximum uncompressed size is 1054371 bytes,
> probably for a bzip2-compressed block.)
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
>   v2: no changes (did not receive comments last time)
> ---
>   block/dmg.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 8239221..4e24076 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -137,7 +137,9 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
>           uncompressed_sectors = (s->lengths[chunk] + 511) / 512;
>           break;
>       case 2: /* zero */
> -        uncompressed_sectors = s->sectorcounts[chunk];
> +        /* as the all-zeroes block may be large, it is treated specially: the
> +         * sector is not copied from a large buffer, a simple memset is used
> +         * instead. Therefore uncompressed_sectors does not need to be set. */
>           break;
>       }
>
> @@ -269,7 +271,9 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
>           /* sector count */
>           s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
>
> -        if (s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
> +        /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
> +         * therefore be unbounded. */
> +        if (s->types[i] != 2 && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
>               error_report("sector count %" PRIu64 " for chunk %" PRIu32
>                            " is larger than max (%u)",
>                            s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
> @@ -644,7 +648,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
>               }
>               break;
>           case 2: /* zero */
> -            memset(s->uncompressed_chunk, 0, 512 * s->sectorcounts[chunk]);
> +            /* see dmg_read, it is treated specially. No buffer needs to be
> +             * pre-filled, the zeroes can be set directly. */
>               break;
>           }
>           s->current_chunk = chunk;
> @@ -663,6 +668,13 @@ static int dmg_read(BlockDriverState *bs, int64_t sector_num,
>           if (dmg_read_chunk(bs, sector_num + i) != 0) {
>               return -1;
>           }
> +        /* Special case: current chunk is all zeroes. Do not perform a memcpy as
> +         * s->uncompressed_chunk may be too small to cover the large all-zeroes
> +         * section. dmg_read_chunk is called to find s->current_chunk */
> +        if (s->types[s->current_chunk] == 2) { /* all zeroes block entry */
> +            memset(buf + i * 512, 0, 512);
> +            continue;
> +        }
>           sector_offset_in_chunk = sector_num + i - s->sectors[s->current_chunk];
>           memcpy(buf + i * 512,
>                  s->uncompressed_chunk + sector_offset_in_chunk * 512, 512);
>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 00/12] block/dmg: (compatibility) fixes and bzip2 support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3285 bytes --]

On Tue, Jan 06, 2015 at 06:48:03PM +0100, Peter Wu wrote:
> Hi,
> 
> This is the second revision of improvements to DMG image file support.
> See [1] for an overview of the previous patchset.
> 
> Thanks to John Snow for his efforts in reviewing patches and providing
> suggestions. The errp suggestion from Stefan Hajnoczi is also
> incorporated.
> 
> An overview of changes since v1 (also mentioned in each patch):
> 
>   block/dmg: properly detect the UDIF trailer [+R-b, set errp)
>   block/dmg: extract mish block decoding functionality [+R-b, added
>         comments, expanded commit message, renamed var]
>   block/dmg: extract processing of resource forks [see patch]
>   block/dmg: process a buffer instead of reading ints [+R-b, no changes]
>   block/dmg: validate chunk size to avoid overflow [added offset check]
>   block/dmg: process XML plists [added offset check]
>   block/dmg: set virtual size to a non-zero value [fix commit message]
>   block/dmg: fix sector data offset calculation [use data provided by file]
>   block/dmg: use SectorNumber from BLKX header [new patch]
>   block/dmg: factor out block type check [extracted from bzip patch]
>   block/dmg: support bzip2 block entry types [set/use BZIP2_LIBS]
>   block/dmg: improve zeroes handling [no changes]
> 
> (the other length checking patch was squashed into the others)
> 
> Note: in the previous patches I mentioned that dmg2img would result in a
> shorter file than qemu-img convert. That turns out to be a bug in
> dmg2img for which a patch is available[2].
> 
> A quick test runner is available at
> https://lekensteyn.nl/files/dmg-tests/. This script depends on the fixed
> dmg2img program and can then be run as follows:
> 
>     QEMU_IMG=/tmp/qout/qemu-img ./run-dmg-tests.sh dmg-images/*.dmg
> 
> You will first need some DMG files, I have collected four different
> public examples with different properties[1]. These can be found in
> urls.txt at https://lekensteyn.nl/files/dmg-tests/dmg-images/.
> 
> Kind regards,
> Peter
> 
>  [1]: https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03606.html
>  [2]: https://github.com/Lekensteyn/dmg2img/commit/a1d4861b4b0f2ac5090938233a1156bb130cb3ef
> 
> Peter Wu (12):
>   block/dmg: properly detect the UDIF trailer
>   block/dmg: extract mish block decoding functionality
>   block/dmg: extract processing of resource forks
>   block/dmg: process a buffer instead of reading ints
>   block/dmg: validate chunk size to avoid overflow
>   block/dmg: process XML plists
>   block/dmg: set virtual size to a non-zero value
>   block/dmg: fix sector data offset calculation
>   block/dmg: use SectorNumber from BLKX header
>   block/dmg: factor out block type check
>   block/dmg: support bzip2 block entry types
>   block/dmg: improve zeroes handling
> 
>  block/Makefile.objs |   1 +
>  block/dmg.c         | 503 ++++++++++++++++++++++++++++++++++++++++------------
>  configure           |  31 ++++
>  3 files changed, 418 insertions(+), 117 deletions(-)

Hard to verify this does not introduce regressions since qemu-iotests
does not support dmg.  The code looks good though.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]

On Wed, Jan 07, 2015 at 03:19:13PM +0100, Peter Wu wrote:
> On Wednesday 07 January 2015 13:19:34 Stefan Hajnoczi wrote:
> > On Tue, Jan 06, 2015 at 06:48:04PM +0100, 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 <peter@lekensteyn.nl>
> > > Reviewed-by: John Snow <jsnow@redhat.com>
> > > ---
> > >  v2: added R-b, set errp as suggested by Stefan Hajnoczi
> > > ---
> > >  block/dmg.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 45 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/block/dmg.c b/block/dmg.c
> > > index e455886..9183459 100644
> > > --- a/block/dmg.c
> > > +++ b/block/dmg.c
> > > @@ -131,6 +131,48 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
> > >      }
> > >  }
> > >  
> > > +static int64_t dmg_find_koly_offset(BlockDriverState *file_bs, Error **errp)
> > > +{
> > > +    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 < 0) {
> > > +        error_setg_errno(errp, -length,
> > > +            "Failed to get file size while reading UDIF trailer");
> > > +        return length;
> > > +    } else if (length < 512) {
> > > +        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
> > > +            "dmg file must be at least 512 bytes long");
> > > +        return -EINVAL;
> > 
> > Not worth respinning but error_setg() is a shortcut for error_set(errp,
> > ERROR_CLASS_GENERIC_ERROR, fmt, ...).
> > 
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Good to know, I got a compile error when ERROR_CLASS_GENERIC_ERROR was
> omitted, didn't think of using error_setg instead.
> 
> When merging, please replace the above ERROR_CLASS_GENERIC_ERROR by:
> 
>         error_setg(errp, "dmg file must be at least 512 bytes long");
> 
> (and likewise for the last error_set in the function)

Done!

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]