* [Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg @ 2018-12-23 2:59 yuchenlin 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search yuchenlin ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: yuchenlin @ 2018-12-23 2:59 UTC (permalink / raw) To: qemu-devel; +Cc: stefanha, qemu-block, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search 2018-12-23 2:59 [Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg yuchenlin @ 2018-12-23 2:59 ` yuchenlin 2018-12-24 15:27 ` Julio Faracco 2019-01-02 11:49 ` Stefan Hajnoczi 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number yuchenlin ` (2 subsequent siblings) 3 siblings, 2 replies; 14+ messages in thread From: yuchenlin @ 2018-12-23 2:59 UTC (permalink / raw) To: qemu-devel; +Cc: stefanha, qemu-block, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search yuchenlin @ 2018-12-24 15:27 ` Julio Faracco 2019-01-02 11:49 ` Stefan Hajnoczi 1 sibling, 0 replies; 14+ messages in thread From: Julio Faracco @ 2018-12-24 15:27 UTC (permalink / raw) To: yuchenlin; +Cc: QEMU Developers, qemu-block, Stefan Hajnoczi 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 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search yuchenlin 2018-12-24 15:27 ` Julio Faracco @ 2019-01-02 11:49 ` Stefan Hajnoczi 2019-01-02 12:20 ` 林育辰 1 sibling, 1 reply; 14+ messages in thread From: Stefan Hajnoczi @ 2019-01-02 11:49 UTC (permalink / raw) To: yuchenlin; +Cc: qemu-devel, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search 2019-01-02 11:49 ` Stefan Hajnoczi @ 2019-01-02 12:20 ` 林育辰 2019-01-03 10:09 ` Stefan Hajnoczi 0 siblings, 1 reply; 14+ messages in thread From: 林育辰 @ 2019-01-02 12:20 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel, qemu-block 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search 2019-01-02 12:20 ` 林育辰 @ 2019-01-03 10:09 ` Stefan Hajnoczi 2019-01-03 11:40 ` Yu-Chen Lin 0 siblings, 1 reply; 14+ messages in thread From: Stefan Hajnoczi @ 2019-01-03 10:09 UTC (permalink / raw) To: 林育辰; +Cc: qemu-devel, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search 2019-01-03 10:09 ` Stefan Hajnoczi @ 2019-01-03 11:40 ` Yu-Chen Lin 0 siblings, 0 replies; 14+ messages in thread From: Yu-Chen Lin @ 2019-01-03 11:40 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel, qemu-block 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number 2018-12-23 2:59 [Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg yuchenlin 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search yuchenlin @ 2018-12-23 2:59 ` yuchenlin 2018-12-24 15:28 ` Julio Faracco 2019-01-02 11:49 ` Stefan Hajnoczi 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk yuchenlin 2018-12-24 15:26 ` [Qemu-devel] [Qemu-block] [PATCH v2 0/3] dmg: fixing reading in dmg Julio Faracco 3 siblings, 2 replies; 14+ messages in thread From: yuchenlin @ 2018-12-23 2:59 UTC (permalink / raw) To: qemu-devel; +Cc: stefanha, qemu-block, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number yuchenlin @ 2018-12-24 15:28 ` Julio Faracco 2019-01-02 11:49 ` Stefan Hajnoczi 1 sibling, 0 replies; 14+ messages in thread From: Julio Faracco @ 2018-12-24 15:28 UTC (permalink / raw) To: yuchenlin; +Cc: QEMU Developers, qemu-block, Stefan Hajnoczi 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 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number yuchenlin 2018-12-24 15:28 ` Julio Faracco @ 2019-01-02 11:49 ` Stefan Hajnoczi 1 sibling, 0 replies; 14+ messages in thread From: Stefan Hajnoczi @ 2019-01-02 11:49 UTC (permalink / raw) To: yuchenlin; +Cc: qemu-devel, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk 2018-12-23 2:59 [Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg yuchenlin 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search yuchenlin 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number yuchenlin @ 2018-12-23 2:59 ` yuchenlin 2018-12-24 15:28 ` Julio Faracco 2019-01-02 12:40 ` Stefan Hajnoczi 2018-12-24 15:26 ` [Qemu-devel] [Qemu-block] [PATCH v2 0/3] dmg: fixing reading in dmg Julio Faracco 3 siblings, 2 replies; 14+ messages in thread From: yuchenlin @ 2018-12-23 2:59 UTC (permalink / raw) To: qemu-devel; +Cc: stefanha, qemu-block, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk yuchenlin @ 2018-12-24 15:28 ` Julio Faracco 2019-01-02 12:40 ` Stefan Hajnoczi 1 sibling, 0 replies; 14+ messages in thread From: Julio Faracco @ 2018-12-24 15:28 UTC (permalink / raw) To: yuchenlin; +Cc: QEMU Developers, qemu-block, Stefan Hajnoczi 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 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk yuchenlin 2018-12-24 15:28 ` Julio Faracco @ 2019-01-02 12:40 ` Stefan Hajnoczi 1 sibling, 0 replies; 14+ messages in thread From: Stefan Hajnoczi @ 2019-01-02 12:40 UTC (permalink / raw) To: yuchenlin; +Cc: qemu-devel, qemu-block [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/3] dmg: fixing reading in dmg 2018-12-23 2:59 [Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg yuchenlin ` (2 preceding siblings ...) 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk yuchenlin @ 2018-12-24 15:26 ` Julio Faracco 3 siblings, 0 replies; 14+ messages in thread From: Julio Faracco @ 2018-12-24 15:26 UTC (permalink / raw) To: yuchenlin; +Cc: QEMU Developers, qemu-block, Stefan Hajnoczi 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 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-01-03 11:40 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-23 2:59 [Qemu-devel] [PATCH v2 0/3] dmg: fixing reading in dmg yuchenlin 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 1/3] dmg: fix binary search yuchenlin 2018-12-24 15:27 ` Julio Faracco 2019-01-02 11:49 ` Stefan Hajnoczi 2019-01-02 12:20 ` 林育辰 2019-01-03 10:09 ` Stefan Hajnoczi 2019-01-03 11:40 ` Yu-Chen Lin 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 2/3] dmg: use enumeration type instead of hard coding number yuchenlin 2018-12-24 15:28 ` Julio Faracco 2019-01-02 11:49 ` Stefan Hajnoczi 2018-12-23 2:59 ` [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk yuchenlin 2018-12-24 15:28 ` Julio Faracco 2019-01-02 12:40 ` Stefan Hajnoczi 2018-12-24 15:26 ` [Qemu-devel] [Qemu-block] [PATCH v2 0/3] dmg: fixing reading in dmg Julio Faracco
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.