[Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg

[Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg

[yuchenlin]

There are two bugs in dmg reading.

First, it may hang in binary search. this problem is solved by patch 1.
Second, because of lacking zero chunk table, reading zero sector will
return EIO. thie problem is solved by patch 2 and 3.

Thanks

v1 - >v2:
* fix typos in patch 1
* add patch 2 and patch 3

yuchenlin (3):
  dmg: fix binary search
  dmg: use enumeration type instead of hard coding number
  dmg: don't skip zero chunk

 block/dmg.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/3] dmg: fix binary search

[yuchenlin]

There is a possible hang in original binary search implementation. That is
if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.

The chunk1 will be still 4, and so on.

Signed-off-by: yuchenlin <npes87184@gmail.com>
---
 block/dmg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 50e91aef6d..0e05702f5d 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -572,14 +572,14 @@ static inline uint32_t search_chunk(BDRVDMGState *s, uint64_t sector_num)
 {
     /* binary search */
     uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
-    while (chunk1 != chunk2) {
+    while (chunk1 <= chunk2) {
         chunk3 = (chunk1 + chunk2) / 2;
         if (s->sectors[chunk3] > sector_num) {
-            chunk2 = chunk3;
+            chunk2 = chunk3 - 1;
         } else if (s->sectors[chunk3] + s->sectorcounts[chunk3] > sector_num) {
             return chunk3;
         } else {
-            chunk1 = chunk3;
+            chunk1 = chunk3 + 1;
         }
     }
     return s->n_chunks; /* error */
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number

[yuchenlin]

Signed-off-by: yuchenlin <npes87184@gmail.com>
---
 block/dmg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 0e05702f5d..6b0a057bf8 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -267,7 +267,7 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
 
         /* 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) {
+        if (s->types[i] != UDIG && 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);
@@ -706,7 +706,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         /* 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 */
+        if (s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
             qemu_iovec_memset(qiov, i * 512, 0, 512);
             continue;
         }
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk

[yuchenlin]

The dmg file has many tables which describe: "start from sector XXX to
sector XXX, the compression method is XXX and where the compressed data
resides on".

Each sector in the expanded file should be covered by a table. The table
will describe the offset of compressed data (or raw depends on the type)
in the dmg.

For example:

[-----------The expanded file------------]
[---bzip table ---]/* zeros */[---zlib---]
    ^
    | if we want to read this sector.

we will find bzip table which contains this sector, and get the
compressed data offset, read it from dmg, uncompress it, finally write to
expanded file.

If we skip zero chunk (table), some sector cannot find the table which
will cause search_chunk() return s->n_chunks, dmg_read_chunk() return -1
and finally causing dmg_co_preadv() return EIO.

See:

[-----------The expanded file------------]
[---bzip table ---]/* zeros */[---zlib---]
                    ^
                    | if we want to read this sector.

Oops, we cannot find the table contains it...

In the original implementation, we don't have zero table. When we try to
read sector inside the zero chunk. We will get EIO, and skip reading.

After this patch, we treat zero chunk the same as ignore chunk, it will
directly write zero and avoid some sector may not find the table.

After this patch:

[-----------The expanded file------------]
[---bzip table ---][--zeros--][---zlib---]

Signed-off-by: yuchenlin <npes87184@gmail.com>
---
 block/dmg.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/block/dmg.c b/block/dmg.c
index 6b0a057bf8..137fe9c1ff 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -130,7 +130,8 @@ static void update_max_chunk_size(BDRVDMGState *s, uint32_t chunk,
     case UDRW: /* copy */
         uncompressed_sectors = DIV_ROUND_UP(s->lengths[chunk], 512);
         break;
-    case UDIG: /* zero */
+    case UDZE: /* zero */
+    case UDIG: /* ignore */
         /* 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. */
@@ -199,8 +200,9 @@ typedef struct DmgHeaderState {
 static bool dmg_is_known_block_type(uint32_t entry_type)
 {
     switch (entry_type) {
+    case UDZE:    /* zeros */
     case UDRW:    /* uncompressed */
-    case UDIG:    /* zeroes */
+    case UDIG:    /* ignore */
     case UDZO:    /* zlib */
         return true;
     case UDBZ:    /* bzip2 */
@@ -265,9 +267,10 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
         /* sector count */
         s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
 
-        /* all-zeroes sector (type 2) does not need to be "uncompressed" and can
-         * therefore be unbounded. */
-        if (s->types[i] != UDIG && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
+        /* all-zeroes sector (type UDZE and UDIG) does not need to be
+         * "uncompressed" and can therefore be unbounded. */
+        if (s->types[i] != UDZE && s->types[i] != UDIG
+            && 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);
@@ -671,7 +674,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs, uint64_t sector_num)
                 return -1;
             }
             break;
-        case UDIG: /* zero */
+        case UDZE: /* zeros */
+        case UDIG: /* ignore */
             /* see dmg_read, it is treated specially. No buffer needs to be
              * pre-filled, the zeroes can be set directly. */
             break;
@@ -706,7 +710,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         /* 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] == UDIG) { /* all zeroes block entry */
+        if (s->types[s->current_chunk] == UDZE
+            || s->types[s->current_chunk] == UDIG) { /* all zeroes block entry */
             qemu_iovec_memset(qiov, i * 512, 0, 512);
             continue;
         }
-- 
2.17.1

Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] dmg: fixing reading in dmg

[Julio Faracco]

The series looks good to me.

I tested with existing DMGs and DMGs that I created by myself.
Both are working fine now.

Reviewed-by: Julio Faracco <jcfaracco@gmail.com>

Em dom, 23 de dez de 2018 às 01:01, yuchenlin <npes87184@gmail.com>
escreveu:

> There are two bugs in dmg reading.
>
> First, it may hang in binary search. this problem is solved by patch 1.
> Second, because of lacking zero chunk table, reading zero sector will
> return EIO. thie problem is solved by patch 2 and 3.
>
> Thanks
>
> v1 - >v2:
> * fix typos in patch 1
> * add patch 2 and patch 3
>
> yuchenlin (3):
>   dmg: fix binary search
>   dmg: use enumeration type instead of hard coding number
>   dmg: don't skip zero chunk
>
>  block/dmg.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> --
> 2.17.1
>
>
>

Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search

[Julio Faracco]

Looks good to me.

Reviewed-by: Julio Faracco <jcfaracco@gmail.com>

Em dom, 23 de dez de 2018 às 01:04, yuchenlin <npes87184@gmail.com>
escreveu:

> There is a possible hang in original binary search implementation. That is
> if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.
>
> The chunk1 will be still 4, and so on.
>
> Signed-off-by: yuchenlin <npes87184@gmail.com>
> ---
>  block/dmg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 50e91aef6d..0e05702f5d 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -572,14 +572,14 @@ static inline uint32_t search_chunk(BDRVDMGState *s,
> uint64_t sector_num)
>  {
>      /* binary search */
>      uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
> -    while (chunk1 != chunk2) {
> +    while (chunk1 <= chunk2) {
>          chunk3 = (chunk1 + chunk2) / 2;
>          if (s->sectors[chunk3] > sector_num) {
> -            chunk2 = chunk3;
> +            chunk2 = chunk3 - 1;
>          } else if (s->sectors[chunk3] + s->sectorcounts[chunk3] >
> sector_num) {
>              return chunk3;
>          } else {
> -            chunk1 = chunk3;
> +            chunk1 = chunk3 + 1;
>          }
>      }
>      return s->n_chunks; /* error */
> --
> 2.17.1
>
>
>

Re: [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number

[Julio Faracco]

Looks good to me.

Reviewed-by: Julio Faracco <jcfaracco@gmail.com>

Em dom, 23 de dez de 2018 às 01:03, yuchenlin <npes87184@gmail.com>
escreveu:

> Signed-off-by: yuchenlin <npes87184@gmail.com>
> ---
>  block/dmg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 0e05702f5d..6b0a057bf8 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -267,7 +267,7 @@ static int dmg_read_mish_block(BDRVDMGState *s,
> DmgHeaderState *ds,
>
>          /* 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) {
> +        if (s->types[i] != UDIG && 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);
> @@ -706,7 +706,7 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes,
>          /* 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
> */
> +        if (s->types[s->current_chunk] == UDIG) { /* all zeroes block
> entry */
>              qemu_iovec_memset(qiov, i * 512, 0, 512);
>              continue;
>          }
> --
> 2.17.1
>
>
>

Re: [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk

[Julio Faracco]

Looks good to me.

Reviewed-by: Julio Faracco <jcfaracco@gmail.com>

Em dom, 23 de dez de 2018 às 01:03, yuchenlin <npes87184@gmail.com>
escreveu:

> The dmg file has many tables which describe: "start from sector XXX to
> sector XXX, the compression method is XXX and where the compressed data
> resides on".
>
> Each sector in the expanded file should be covered by a table. The table
> will describe the offset of compressed data (or raw depends on the type)
> in the dmg.
>
> For example:
>
> [-----------The expanded file------------]
> [---bzip table ---]/* zeros */[---zlib---]
>     ^
>     | if we want to read this sector.
>
> we will find bzip table which contains this sector, and get the
> compressed data offset, read it from dmg, uncompress it, finally write to
> expanded file.
>
> If we skip zero chunk (table), some sector cannot find the table which
> will cause search_chunk() return s->n_chunks, dmg_read_chunk() return -1
> and finally causing dmg_co_preadv() return EIO.
>
> See:
>
> [-----------The expanded file------------]
> [---bzip table ---]/* zeros */[---zlib---]
>                     ^
>                     | if we want to read this sector.
>
> Oops, we cannot find the table contains it...
>
> In the original implementation, we don't have zero table. When we try to
> read sector inside the zero chunk. We will get EIO, and skip reading.
>
> After this patch, we treat zero chunk the same as ignore chunk, it will
> directly write zero and avoid some sector may not find the table.
>
> After this patch:
>
> [-----------The expanded file------------]
> [---bzip table ---][--zeros--][---zlib---]
>
> Signed-off-by: yuchenlin <npes87184@gmail.com>
> ---
>  block/dmg.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 6b0a057bf8..137fe9c1ff 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -130,7 +130,8 @@ static void update_max_chunk_size(BDRVDMGState *s,
> uint32_t chunk,
>      case UDRW: /* copy */
>          uncompressed_sectors = DIV_ROUND_UP(s->lengths[chunk], 512);
>          break;
> -    case UDIG: /* zero */
> +    case UDZE: /* zero */
> +    case UDIG: /* ignore */
>          /* 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. */
> @@ -199,8 +200,9 @@ typedef struct DmgHeaderState {
>  static bool dmg_is_known_block_type(uint32_t entry_type)
>  {
>      switch (entry_type) {
> +    case UDZE:    /* zeros */
>      case UDRW:    /* uncompressed */
> -    case UDIG:    /* zeroes */
> +    case UDIG:    /* ignore */
>      case UDZO:    /* zlib */
>          return true;
>      case UDBZ:    /* bzip2 */
> @@ -265,9 +267,10 @@ static int dmg_read_mish_block(BDRVDMGState *s,
> DmgHeaderState *ds,
>          /* sector count */
>          s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
>
> -        /* all-zeroes sector (type 2) does not need to be "uncompressed"
> and can
> -         * therefore be unbounded. */
> -        if (s->types[i] != UDIG && s->sectorcounts[i] >
> DMG_SECTORCOUNTS_MAX) {
> +        /* all-zeroes sector (type UDZE and UDIG) does not need to be
> +         * "uncompressed" and can therefore be unbounded. */
> +        if (s->types[i] != UDZE && s->types[i] != UDIG
> +            && 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);
> @@ -671,7 +674,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
> uint64_t sector_num)
>                  return -1;
>              }
>              break;
> -        case UDIG: /* zero */
> +        case UDZE: /* zeros */
> +        case UDIG: /* ignore */
>              /* see dmg_read, it is treated specially. No buffer needs to
> be
>               * pre-filled, the zeroes can be set directly. */
>              break;
> @@ -706,7 +710,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes,
>          /* 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] == UDIG) { /* all zeroes block
> entry */
> +        if (s->types[s->current_chunk] == UDZE
> +            || s->types[s->current_chunk] == UDIG) { /* all zeroes block
> entry */
>              qemu_iovec_memset(qiov, i * 512, 0, 512);
>              continue;
>          }
> --
> 2.17.1
>
>
>

Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search

[Stefan Hajnoczi]

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

On Sun, Dec 23, 2018 at 10:59:37AM +0800, yuchenlin wrote:
> There is a possible hang in original binary search implementation. That is
> if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.
> 
> The chunk1 will be still 4, and so on.
> 
> Signed-off-by: yuchenlin <npes87184@gmail.com>
> ---
>  block/dmg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/dmg.c b/block/dmg.c
> index 50e91aef6d..0e05702f5d 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -572,14 +572,14 @@ static inline uint32_t search_chunk(BDRVDMGState *s, uint64_t sector_num)
>  {
>      /* binary search */
>      uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
> -    while (chunk1 != chunk2) {
> +    while (chunk1 <= chunk2) {
>          chunk3 = (chunk1 + chunk2) / 2;
>          if (s->sectors[chunk3] > sector_num) {
> -            chunk2 = chunk3;
> +            chunk2 = chunk3 - 1;

Question from the previous email you sent:

What happens when chunk1 = 0, chunk2 = 1, and chunk3 = 0?  This would
cause out-of-bounds sectors[] accesses.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number

[Stefan Hajnoczi]

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

On Sun, Dec 23, 2018 at 10:59:38AM +0800, yuchenlin wrote:
> Signed-off-by: yuchenlin <npes87184@gmail.com>
> ---
>  block/dmg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search

[林育辰]

Hi, Stefan

Thank you for your reviewing.

This series is focus on fixing bug #1809304 (see:
https://bugs.launchpad.net/qemu/+bug/1809304).
There is an example dmg file in #1809304 which will trigger this bug.

About your case, I think we can simply check whether chunk3 is zero before
we decrease it.
if s->sectors[chunk3] > sector_num and chunk3 is zero (i.e. s->sectors[0] >
sector_num), it means we cannot find the table contains sector_num.
We can return s->n_chunks (error) directly.

What do you think?

Thanks,
Yu-Chen


Stefan Hajnoczi <stefanha@redhat.com> 於 2019年1月2日 週三 下午7:49寫道：

> On Sun, Dec 23, 2018 at 10:59:37AM +0800, yuchenlin wrote:
> > There is a possible hang in original binary search implementation. That
> is
> > if chunk1 = 4, chunk2 = 5, chunk3 = 4, and we go else case.
> >
> > The chunk1 will be still 4, and so on.
> >
> > Signed-off-by: yuchenlin <npes87184@gmail.com>
> > ---
> >  block/dmg.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/dmg.c b/block/dmg.c
> > index 50e91aef6d..0e05702f5d 100644
> > --- a/block/dmg.c
> > +++ b/block/dmg.c
> > @@ -572,14 +572,14 @@ static inline uint32_t search_chunk(BDRVDMGState
> *s, uint64_t sector_num)
> >  {
> >      /* binary search */
> >      uint32_t chunk1 = 0, chunk2 = s->n_chunks, chunk3;
> > -    while (chunk1 != chunk2) {
> > +    while (chunk1 <= chunk2) {
> >          chunk3 = (chunk1 + chunk2) / 2;
> >          if (s->sectors[chunk3] > sector_num) {
> > -            chunk2 = chunk3;
> > +            chunk2 = chunk3 - 1;
>
> Question from the previous email you sent:
>
> What happens when chunk1 = 0, chunk2 = 1, and chunk3 = 0?  This would
> cause out-of-bounds sectors[] accesses.
>
> Stefan
>

Re: [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk

[Stefan Hajnoczi]

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

On Sun, Dec 23, 2018 at 10:59:39AM +0800, yuchenlin wrote:
> The dmg file has many tables which describe: "start from sector XXX to
> sector XXX, the compression method is XXX and where the compressed data
> resides on".
> 
> Each sector in the expanded file should be covered by a table. The table
> will describe the offset of compressed data (or raw depends on the type)
> in the dmg.
> 
> For example:
> 
> [-----------The expanded file------------]
> [---bzip table ---]/* zeros */[---zlib---]
>     ^
>     | if we want to read this sector.
> 
> we will find bzip table which contains this sector, and get the
> compressed data offset, read it from dmg, uncompress it, finally write to
> expanded file.
> 
> If we skip zero chunk (table), some sector cannot find the table which
> will cause search_chunk() return s->n_chunks, dmg_read_chunk() return -1
> and finally causing dmg_co_preadv() return EIO.
> 
> See:
> 
> [-----------The expanded file------------]
> [---bzip table ---]/* zeros */[---zlib---]
>                     ^
>                     | if we want to read this sector.
> 
> Oops, we cannot find the table contains it...
> 
> In the original implementation, we don't have zero table. When we try to
> read sector inside the zero chunk. We will get EIO, and skip reading.
> 
> After this patch, we treat zero chunk the same as ignore chunk, it will
> directly write zero and avoid some sector may not find the table.
> 
> After this patch:
> 
> [-----------The expanded file------------]
> [---bzip table ---][--zeros--][---zlib---]
> 
> Signed-off-by: yuchenlin <npes87184@gmail.com>
> ---
>  block/dmg.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search

[Stefan Hajnoczi]

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

On Wed, Jan 02, 2019 at 08:20:54PM +0800, 林育辰 wrote:
> This series is focus on fixing bug #1809304 (see:
> https://bugs.launchpad.net/qemu/+bug/1809304).
> There is an example dmg file in #1809304 which will trigger this bug.

Thanks.  It would be great to include a tiny dmg file in
tests/qemu-iotests/sample_images/ and add a test case for it.

The file should be small (kilobytes, not megabytes) and must be
redistributable (no proprietary content or even GPL software, which
requires distributing source code).

Do you have time to do that?

> About your case, I think we can simply check whether chunk3 is zero before
> we decrease it.
> if s->sectors[chunk3] > sector_num and chunk3 is zero (i.e. s->sectors[0] >
> sector_num), it means we cannot find the table contains sector_num.
> We can return s->n_chunks (error) directly.
> 
> What do you think?

Sounds good.  We have to assume that the file contents are invalid and
handle all cases.

I'll review the next revision of your patch.  Thanks!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search

[Yu-Chen Lin]

Stefan Hajnoczi <stefanha@redhat.com> 於 2019年1月3日 週四 下午6:09寫道：

> On Wed, Jan 02, 2019 at 08:20:54PM +0800, 林育辰 wrote:
> > This series is focus on fixing bug #1809304 (see:
> > https://bugs.launchpad.net/qemu/+bug/1809304).
> > There is an example dmg file in #1809304 which will trigger this bug.
>
> Thanks.  It would be great to include a tiny dmg file in
> tests/qemu-iotests/sample_images/ and add a test case for it.
>
> The file should be small (kilobytes, not megabytes) and must be
> redistributable (no proprietary content or even GPL software, which
> requires distributing source code).
>
> Do you have time to do that?
>

Sure!


>
> > About your case, I think we can simply check whether chunk3 is zero
> before
> > we decrease it.
> > if s->sectors[chunk3] > sector_num and chunk3 is zero (i.e.
> s->sectors[0] >
> > sector_num), it means we cannot find the table contains sector_num.
> > We can return s->n_chunks (error) directly.
> >
> > What do you think?
>
> Sounds good.  We have to assume that the file contents are invalid and
> handle all cases.
>
> I'll review the next revision of your patch.  Thanks!
>

Thanks!

Yu-Chen Lin


> Stefan
>