Bug#686502: pxz produces archives broken for busybox's unxz
Hi, so is the latest patch by Abou acceptable? If the logic's ok I guess the committer could also fix up the last bunch of coding style issues. Kind regards Philipp Kern -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20130223160702.ga1...@spike.0x539.de
Bug#686502: pxz produces archives broken for busybox's unxz
On Sun, 2013-01-06 at 23:32 +0100, Bastian Blank wrote:
On Thu, Dec 27, 2012 at 10:08:07PM +0100, Abou Al Montacir wrote:
+ if (r == XZ_STREAM_END) {
+ /* Eat padding. Stream never starts with zeros, and
padding is 32 aligned */
+ while ((iobuf.in_pos iobuf.in_size)
(iobuf.in[iobuf.in_pos] == 0)) {
+ iobuf.in_pos += 1;
+ }
+ /* Reached end of buffer. Fill it again from stream */
+ if (iobuf.in_pos == iobuf.in_size) {
+ continue;
+ }
+ if(iobuf.in_pos % 4){
Are you sure this is correct? in_pos is the position in tht buffer, not
the file. Also look out for coding style.
Provided the buffer size is multiple of 4 bytes, which is the case for
BUFSIZ. Of course one can decide to use a mis aligned buffer, but this
is not common and I consider it bad coding practice.
+ if (r == XZ_STREAM_END) {
Again the same check?
Not really, r could have been changed since the above check (r = XZ_DATA_ERROR;
when %4 check fails)
if (r == XZ_STREAM_END) {
- break;
+ xz_dec_end(state);
+ /* Look for any other streams */
+ continue;
Why do you have three XZ_STREAM_END checks in this state machine?
I use XZ_STREAM_END to check end of stream and eat padding, to check
there is still valid data (no paddding error) before initializing
decoder, and finally to free the decoder at en of current stream.
Cheers,
signature.asc
Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
Hi, On Thu, Dec 27, 2012 at 10:08:07PM +0100, Abou Al Montacir wrote: I've fixed my patch and think that know it should really be conformant. I also attached some short samples to be tested. One of them only should fail to decode. could somebody please review that patch and if suitable upload it? Thanks :-) Philipp Kern signature.asc Description: Digital signature
Bug#686502: pxz produces archives broken for busybox's unxz
On Sat, Dec 22, 2012 at 12:03:31AM +0100, Abou Al Montacir wrote:
--- busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch
1970-01-01 01:00:00.0 +0100
+++ busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch
2012-12-21 19:23:12.0 +0100
@@ -0,0 +1,25 @@
+Author: Abou Al Montacir abou.almonta...@sfr.fr
+Purpose: Fix decompression of multi stream XZ compressed files
+ (Closes: bug#686502)
+
+--- busybox-1.20.0~/archival/libarchive/decompress_unxz.c2012-12-20
21:51:04.0 +0100
busybox-1.20.0/archival/libarchive/decompress_unxz.c 2012-12-20
21:49:11.0 +0100
+@@ -87,7 +87,17 @@ unpack_xz_stream(transformer_aux_data_t *aux, int src_fd,
int dst_fd)
+ iobuf.out_pos = 0;
+ }
+ if (r == XZ_STREAM_END) {
+-break;
++if (iobuf.in_pos != iobuf.in_size) {
++// Initialize decoder for new stream
++xz_dec_end(state);
++state = xz_dec_init(XZ_DYNALLOC, 64*1024*1024);
Why can't you use the existing call somewhere at the beginning? If I
remember correctly, you need 128*1024*1024 to decompress all valid
files.
++// Eat padding
++while (iobuf.in[iobuf.in_pos] == 0){
++iobuf.in_pos += 1;
++}
Padding is a multiple of _four_ bytes. Did you read the spec?
++}
++// Look for other streams
++continue;
Does it bail out if there is no new stream?
Bastian
--
Men will always be men -- no matter where they are.
-- Harry Mudd, Mudd's Women, stardate 1329.8
--
To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/20130106203959.ga12...@waldi.eu.org
Bug#686502: pxz produces archives broken for busybox's unxz
On Sun, Jan 06, 2013 at 09:40:00PM +0100, Bastian Blank wrote: This was the wrong mail. Bastian -- Oblivion together does not frighten me, beloved. -- Thalassa (in Anne Mulhall's body), Return to Tomorrow, stardate 4770.3. -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20130106222618.ga14...@waldi.eu.org
Bug#686502: pxz produces archives broken for busybox's unxz
On Thu, Dec 27, 2012 at 10:08:07PM +0100, Abou Al Montacir wrote:
+ if (r == XZ_STREAM_END) {
+ /* Eat padding. Stream never starts with zeros, and
padding is 32 aligned */
+ while ((iobuf.in_pos iobuf.in_size)
(iobuf.in[iobuf.in_pos] == 0)) {
+ iobuf.in_pos += 1;
+ }
+ /* Reached end of buffer. Fill it again from stream */
+ if (iobuf.in_pos == iobuf.in_size) {
+ continue;
+ }
+ if(iobuf.in_pos % 4){
Are you sure this is correct? in_pos is the position in tht buffer, not
the file. Also look out for coding style.
+ if (r == XZ_STREAM_END) {
Again the same check?
if (r == XZ_STREAM_END) {
- break;
+ xz_dec_end(state);
+ /* Look for any other streams */
+ continue;
Why do you have three XZ_STREAM_END checks in this state machine?
Bastian
--
There are always alternatives.
-- Spock, The Galileo Seven, stardate 2822.3
--
To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/20130106223213.gb14...@waldi.eu.org
Bug#686502: pxz produces archives broken for busybox's unxz
On Mon, 2012-12-24 at 17:11 -0800, Jonathan Nieder wrote: Abou Al Montacir wrote: On Sat, 2012-12-22 at 10:21 -0800, Jonathan Nieder wrote: What happens if a stream ends at a buffer boundary, followed by padding? Or if padding doesn't fit in the buffer, for that matter? [...] Please find attached new debdiff with fix of above mentioned issues. Getting closer. Does this correctly handle the case of a file with zero streams? (It should error out.) How about a file with leading Let's see: We will read buffer, do no find zeros and go into decoder and then issue an error. NUL bytes, which is also invalid? It will remove the zeros and then start decoding. I agree the file is invalid, but I don't think it could harm to decode any valid stream inside. Does this implementation meet the following requirement (from the spec)? | Stream Padding MUST contain only null bytes. To preserve the | four-byte alignment of consecutive Streams, the size of Stream | Padding MUST be a multiple of four bytes. Empty Stream Padding | is allowed. If these requirements are not met, the decoder MUST | indicate an error. Clearly it does not met this but could be done assuming few extra ifs. Hover, I assume we can save this extra code as soon as we don't loose data. My goal was more to avoid data loss for user rather than providing a specification conformant decoder. I just want to ensure that a user decoding a valid .xz file does not loose any data. If the decoder is more tolerant than the standard, my goal is met. Now if RT requires to have a full standard conformant decoder inside busybox, I can do it. Thanks for your patient work. Thank you for your careful review. Cheers, signature.asc Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
Abou Al Montacir wrote: Hover, I assume we can save this extra code as soon as we don't loose data. That's fine with me. All you'd need to do is error out if there is anything after the first stream. That would make it a conformant decoder and prevent silent data loss, though it would mean busybox couldn't read the XZ files pxz produces. (Context: the spec permits single-stream decoders because there are decoders in the wild that need to be very simple, like the one built into the Linux kernel that unpacks the kernel and initramfs.) On the other hand, if busybox is to start decoding concatenated streams (imitating the standard xz command), then the spec requires also correctly implementing padding. This might sound rigid, but it is important for interoperability --- without such requirements, whenever you share XZ files there would be a lot of confusion about whether it is valid and which implementations can and can't decode it. I think busybox upstream would agree that the spec shouldn't just be ignored. Hoping that clarifies, Jonathan -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20121227163853.GA6256@elie.Belkin
Bug#686502: pxz produces archives broken for busybox's unxz
On Thu, 2012-12-27 at 08:38 -0800, Jonathan Nieder wrote:
Abou Al Montacir wrote:
Hover, I assume we can save this extra code as soon as we don't loose
data.
That's fine with me. All you'd need to do is error out if there is
anything after the first stream. That would make it a conformant
decoder and prevent silent data loss, though it would mean busybox
couldn't read the XZ files pxz produces.
Sure,
(Context: the spec permits single-stream decoders because there are
decoders in the wild that need to be very simple, like the one built
into the Linux kernel that unpacks the kernel and initramfs.)
On the other hand, if busybox is to start decoding concatenated
streams (imitating the standard xz command), then the spec requires
also correctly implementing padding. This might sound rigid, but it
is important for interoperability --- without such requirements,
whenever you share XZ files there would be a lot of confusion about
whether it is valid and which implementations can and can't decode it.
Agree
I think busybox upstream would agree that the spec shouldn't just be
ignored.
You convinced me here. I admit you're right the we should either conform
to spec or conform to spec, no choice there.
I've fixed my patch and think that know it should really be conformant.
I also attached some short samples to be tested. One of them only should
fail to decode.
I really thank you for your support and review as well as for your sense
of details. I admit I've learned from you many things.
Thanks,
Author: Abou Al Montacir abou.almonta...@sfr.fr
Purpose: Fix decompression of multi stream XZ compressed files
(Closes: bug#686502)
--- busybox-1.20.0/archival/libarchive/decompress_unxz.c 2012-04-22 03:33:23.0 +0200
+++ busybox-1.20.0/debian/build/deb/archival/libarchive/decompress_unxz.c 2012-12-27 21:58:49.0 +0100
@@ -44,6 +44,7 @@
struct xz_dec *state;
unsigned char *membuf;
IF_DESKTOP(long long) int total = 0;
+ enum xz_ret r;
if (!global_crc32_table)
global_crc32_table = crc32_filltable(NULL, /*endian:*/ 0);
@@ -59,12 +60,11 @@
strcpy((char*)membuf, HEADER_MAGIC);
iobuf.in_size = HEADER_MAGIC_SIZE;
} /* else: let xz code read check it */
-
- /* Limit memory usage to about 64 MiB. */
+ /* First stream is identical to starting a new stream after finishing decoding an old one */
state = xz_dec_init(XZ_DYNALLOC, 64*1024*1024);
+ r = XZ_OK;
while (1) {
- enum xz_ret r;
if (iobuf.in_pos == iobuf.in_size) {
int rd = safe_read(src_fd, membuf, BUFSIZ);
@@ -73,12 +73,36 @@
total = -1;
break;
}
+ /* No more bytes in stream. Stop */
+ if (rd == 0) {
+break;
+ }
iobuf.in_size = rd;
iobuf.in_pos = 0;
}
+ if (r == XZ_STREAM_END) {
+ /* Eat padding. Stream never starts with zeros, and padding is 32 aligned */
+ while ((iobuf.in_pos iobuf.in_size) (iobuf.in[iobuf.in_pos] == 0)) {
+ iobuf.in_pos += 1;
+ }
+ /* Reached end of buffer. Fill it again from stream */
+ if (iobuf.in_pos == iobuf.in_size) {
+continue;
+ }
+ if(iobuf.in_pos % 4){
+r = XZ_DATA_ERROR;
+ }
+ }
// bb_error_msg(in pos:%d size:%d out pos:%d size:%d,
//iobuf.in_pos, iobuf.in_size, iobuf.out_pos, iobuf.out_size);
- r = xz_dec_run(state, iobuf);
+ /* Initialize decoder for new stream. Limit memory usage to about 64 MiB. */
+ if (r == XZ_STREAM_END) {
+ state = xz_dec_init(XZ_DYNALLOC, 64*1024*1024);
+ r = XZ_OK;
+ }
+ if ((r == XZ_OK) || (r == XZ_UNSUPPORTED_CHECK)){
+ r = xz_dec_run(state, iobuf);
+ }
// bb_error_msg(in pos:%d size:%d out pos:%d size:%d r:%d,
//iobuf.in_pos, iobuf.in_size, iobuf.out_pos, iobuf.out_size, r);
if (iobuf.out_pos) {
@@ -87,7 +111,9 @@
iobuf.out_pos = 0;
}
if (r == XZ_STREAM_END) {
- break;
+ xz_dec_end(state);
+ /* Look for any other streams */
+ continue;
}
if (r != XZ_OK r != XZ_UNSUPPORTED_CHECK) {
bb_error_msg(corrupted data);
@@ -95,7 +121,6 @@
break;
}
}
- xz_dec_end(state);
free(membuf);
return total;
hel.xz
Description: application/xz
hel000lo.xz
Description: application/xz
hello.xz
Description: application/xz
hello.xz
Description: application/xz
hello1.xz
Description: application/xz
signature.asc
Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
On Sat, 2012-12-22 at 10:21 -0800, Jonathan Nieder wrote: What happens if a stream ends at a buffer boundary, followed by padding? Or if padding doesn't fit in the buffer, for that matter? That make very low probability but could happe indeed. I will upload a new patch which fixes this case too. Thank you for your review. Cheers, signature.asc Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
On Sat, 2012-12-22 at 10:21 -0800, Jonathan Nieder wrote:
What happens if a stream ends at a buffer boundary, followed by
padding? Or if padding doesn't fit in the buffer, for that
matter?
Hope that helps,
Please find attached new debdiff with fix of above mentioned issues.
Cheers,
diff -Nru busybox-1.20.0/debian/changelog busybox-1.20.0/debian/changelog
--- busybox-1.20.0/debian/changelog 2012-09-20 08:32:55.0 +0200
+++ busybox-1.20.0/debian/changelog 2012-12-21 21:59:39.0 +0100
@@ -1,3 +1,10 @@
+busybox (1:1.20.0-7.1) unstable; urgency=low
+
+ * Fix decompression of multi stream XZ compressed files
+(Closes: Bug#bug#686502)
+
+ -- Abou Al Montacir abou.almonta...@sfr.fr Thu, 21 Dec 2012 22:00:00 +0100
+
busybox (1:1.20.0-7) unstable; urgency=low
* set CONFIG_FEATURE_COPYBUF_KB from 4 to 64 for all flavours. This
diff -Nru busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch
--- busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch 1970-01-01 01:00:00.0 +0100
+++ busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch 2012-12-24 23:12:05.0 +0100
@@ -0,0 +1,74 @@
+Author: Abou Al Montacir abou.almonta...@sfr.fr
+Purpose: Fix decompression of multi stream XZ compressed files
+ (Closes: bug#686502)
+
+--- busybox-1.20.0/archival/libarchive/decompress_unxz.c 2012-12-24 21:21:47.0 +0100
busybox-1.20.0/debian/build/deb/archival/libarchive/decompress_unxz.c 2012-12-24 23:10:35.0 +0100
+@@ -44,6 +44,7 @@
+ struct xz_dec *state;
+ unsigned char *membuf;
+ IF_DESKTOP(long long) int total = 0;
++ enum xz_ret r;
+
+ if (!global_crc32_table)
+ global_crc32_table = crc32_filltable(NULL, /*endian:*/ 0);
+@@ -59,12 +60,10 @@
+ strcpy((char*)membuf, HEADER_MAGIC);
+ iobuf.in_size = HEADER_MAGIC_SIZE;
+ } /* else: let xz code read check it */
+-
+- /* Limit memory usage to about 64 MiB. */
+- state = xz_dec_init(XZ_DYNALLOC, 64*1024*1024);
++ /* First stream is identical to starting a new stream after finishing decoding an old one */
++ r = XZ_STREAM_END;
+
+ while (1) {
+- enum xz_ret r;
+
+ if (iobuf.in_pos == iobuf.in_size) {
+ int rd = safe_read(src_fd, membuf, BUFSIZ);
+@@ -73,9 +72,25 @@
+ total = -1;
+ break;
+ }
++ /* No more bytes in stream. Stop */
++ if (rd == 0) {
++break;
++ }
+ iobuf.in_size = rd;
+ iobuf.in_pos = 0;
+ }
++ if (r == XZ_STREAM_END) {
++ /* Eat padding. Stream never starts with zeros */
++ while ((iobuf.in_pos iobuf.in_size) (iobuf.in[iobuf.in_pos] == 0)) {
++ iobuf.in_pos += 1;
++ }
++ /* Reached end of buffer. Fill it again from stream */
++ if (iobuf.in_pos == iobuf.in_size) {
++continue;
++ }
++ /* Initialize decoder for new stream. Limit memory usage to about 64 MiB. */
++ state = xz_dec_init(XZ_DYNALLOC, 64*1024*1024);
++ }
+ // bb_error_msg(in pos:%d size:%d out pos:%d size:%d,
+ //iobuf.in_pos, iobuf.in_size, iobuf.out_pos, iobuf.out_size);
+ r = xz_dec_run(state, iobuf);
+@@ -87,7 +102,9 @@
+ iobuf.out_pos = 0;
+ }
+ if (r == XZ_STREAM_END) {
+- break;
++ xz_dec_end(state);
++ /* Look for any other streams */
++ continue;
+ }
+ if (r != XZ_OK r != XZ_UNSUPPORTED_CHECK) {
+ bb_error_msg(corrupted data);
+@@ -95,7 +112,6 @@
+ break;
+ }
+ }
+- xz_dec_end(state);
+ free(membuf);
+
+ return total;
diff -Nru busybox-1.20.0/debian/patches/series busybox-1.20.0/debian/patches/series
--- busybox-1.20.0/debian/patches/series 2012-09-19 22:58:00.0 +0200
+++ busybox-1.20.0/debian/patches/series 2012-12-20 21:54:21.0 +0100
@@ -25,3 +25,6 @@
dont-force-no-alignment-for-s390.patch
stop-checking-ancient-kernel-version.patch
+
+# http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=686502
+fix-unxz-with-multiple-streams.patch
signature.asc
Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
Abou Al Montacir wrote: On Sat, 2012-12-22 at 10:21 -0800, Jonathan Nieder wrote: What happens if a stream ends at a buffer boundary, followed by padding? Or if padding doesn't fit in the buffer, for that matter? [...] Please find attached new debdiff with fix of above mentioned issues. Getting closer. Does this correctly handle the case of a file with zero streams? (It should error out.) How about a file with leading NUL bytes, which is also invalid? Does this implementation meet the following requirement (from the spec)? | Stream Padding MUST contain only null bytes. To preserve the | four-byte alignment of consecutive Streams, the size of Stream | Padding MUST be a multiple of four bytes. Empty Stream Padding | is allowed. If these requirements are not met, the decoder MUST | indicate an error. Thanks for your patient work. Jonathan -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20121225011143.GA23669@elie.Belkin
Bug#686502: pxz produces archives broken for busybox's unxz
Abou Al Montacir wrote:
+--- busybox-1.20.0~/archival/libarchive/decompress_unxz.c2012-12-20
21:51:04.0 +0100
busybox-1.20.0/archival/libarchive/decompress_unxz.c 2012-12-20
21:49:11.0 +0100
+@@ -87,7 +87,17 @@ unpack_xz_stream(transformer_aux_data_t *aux, int src_fd,
int dst_fd)
+ iobuf.out_pos = 0;
+ }
+ if (r == XZ_STREAM_END) {
+-break;
++if (iobuf.in_pos != iobuf.in_size) {
[...]
++}
++// Look for other streams
++continue;
What happens if a stream ends at a buffer boundary, followed by
padding? Or if padding doesn't fit in the buffer, for that
matter?
Hope that helps,
Jonathan
--
To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/20121222182152.GB4568@elie.Belkin
Bug#686502: pxz produces archives broken for busybox's unxz
On Thu, 2012-12-20 at 23:08 +0100, Bastian Blank wrote: On Thu, Dec 20, 2012 at 10:42:41PM +0100, Abou Al Montacir wrote: Can you please test the attached patch How does it implement stream padding? Hi Bastian, As it is implemented, it will iterate until end of stream, but I did not test this particular case. If you can provide an example of files on which it will fail, I can try to fix it. Cheers, signature.asc Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
21.12.2012 17:06, Abou Al Montacir wrote:
On Thu, 2012-12-20 at 23:08 +0100, Bastian Blank wrote:
On Thu, Dec 20, 2012 at 10:42:41PM +0100, Abou Al Montacir wrote:
Can you please test the attached patch
How does it implement stream padding?
Hi Bastian,
As it is implemented, it will iterate until end of stream, but I did not
test this particular case.
Actually it is not:
+ if (iobuf.in_pos == iobuf.in_size) {
+ break;
+ } else {
iobuf is what we've in memory. We may've read some data which
ends in buffer exactly at the end of the stream. There might
be next stream coming, but for it we may need to read a few
more bytes first... At least if I read the code correctly.
It is sorta like testing if we reached end of file by testing
whenever we're at the end of stdio buffer.
Thanks,
/mjt
--
To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/50d46077.6070...@msgid.tls.msk.ru
Bug#686502: pxz produces archives broken for busybox's unxz
On Fri, Dec 21, 2012 at 02:06:31PM +0100, Abou Al Montacir wrote: On Thu, 2012-12-20 at 23:08 +0100, Bastian Blank wrote: On Thu, Dec 20, 2012 at 10:42:41PM +0100, Abou Al Montacir wrote: Can you please test the attached patch How does it implement stream padding? As it is implemented, it will iterate until end of stream, but I did not test this particular case. I ask you how it implements a mandantory feature and you are not able to tell me? If you can provide an example of files on which it will fail, I can try to fix it. Add stream padding as specified in the spec. Bastian -- There's a way out of any cage. -- Captain Christopher Pike, The Menagerie (The Cage), stardate unknown. -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20121221132405.ga23...@waldi.eu.org
Bug#686502: pxz produces archives broken for busybox's unxz
On Fri, 2012-12-21 at 17:13 +0400, Michael Tokarev wrote:
21.12.2012 17:06, Abou Al Montacir wrote:
On Thu, 2012-12-20 at 23:08 +0100, Bastian Blank wrote:
On Thu, Dec 20, 2012 at 10:42:41PM +0100, Abou Al Montacir wrote:
Can you please test the attached patch
How does it implement stream padding?
Hi Bastian,
As it is implemented, it will iterate until end of stream, but I did not
test this particular case.
Actually it is not:
+ if (iobuf.in_pos == iobuf.in_size) {
+ break;
+ } else {
iobuf is what we've in memory. We may've read some data which
ends in buffer exactly at the end of the stream. There might
be next stream coming, but for it we may need to read a few
more bytes first... At least if I read the code correctly.
It is sorta like testing if we reached end of file by testing
whenever we're at the end of stdio buffer.
Hi Michael,
Good catch! I'll fix that by replacing that break by continue, so it
will read on next iteration and will break for end of file.
I'll submit a new patch soon.
Cheers,
signature.asc
Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
On Fri, 2012-12-21 at 14:24 +0100, Bastian Blank wrote: On Fri, Dec 21, 2012 at 02:06:31PM +0100, Abou Al Montacir wrote: On Thu, 2012-12-20 at 23:08 +0100, Bastian Blank wrote: On Thu, Dec 20, 2012 at 10:42:41PM +0100, Abou Al Montacir wrote: Can you please test the attached patch How does it implement stream padding? As it is implemented, it will iterate until end of stream, but I did not test this particular case. I ask you how it implements a mandantory feature and you are not able to tell me? As I told you it will iterate on it and consume it until it finds a new valid header. A smarter way is to eat zeros until next non zero data. Please c.f ยง2.2 on spec R1.0.4 If you can provide an example of files on which it will fail, I can try to fix it. Add stream padding as specified in the spec. I'll provide a new patch for eating zeros and fixing issue pointed by Michael Cheers, signature.asc Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
On Fri, 2012-12-21 at 14:34 +0100, Abou Al Montacir wrote:
Add stream padding as specified in the spec.
I'll provide a new patch for eating zeros and fixing issue pointed by Michael
Please find attached new patch handling padding and fixing issue
highlighted by Michael,
Cheers,
diff -Nru busybox-1.20.0/debian/changelog busybox-1.20.0/debian/changelog
--- busybox-1.20.0/debian/changelog 2012-09-20 08:32:55.0 +0200
+++ busybox-1.20.0/debian/changelog 2012-12-21 21:59:39.0 +0100
@@ -1,3 +1,10 @@
+busybox (1:1.20.0-7.1) unstable; urgency=low
+
+ * Fix decompression of multi stream XZ compressed files
+(Closes: Bug#bug#686502)
+
+ -- Abou Al Montacir abou.almonta...@sfr.fr Thu, 21 Dec 2012 22:00:00 +0100
+
busybox (1:1.20.0-7) unstable; urgency=low
* set CONFIG_FEATURE_COPYBUF_KB from 4 to 64 for all flavours. This
diff -Nru busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch
--- busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch 1970-01-01 01:00:00.0 +0100
+++ busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch 2012-12-21 19:23:12.0 +0100
@@ -0,0 +1,25 @@
+Author: Abou Al Montacir abou.almonta...@sfr.fr
+Purpose: Fix decompression of multi stream XZ compressed files
+ (Closes: bug#686502)
+
+--- busybox-1.20.0~/archival/libarchive/decompress_unxz.c 2012-12-20 21:51:04.0 +0100
busybox-1.20.0/archival/libarchive/decompress_unxz.c 2012-12-20 21:49:11.0 +0100
+@@ -87,7 +87,17 @@ unpack_xz_stream(transformer_aux_data_t *aux, int src_fd, int dst_fd)
+ iobuf.out_pos = 0;
+ }
+ if (r == XZ_STREAM_END) {
+- break;
++ if (iobuf.in_pos != iobuf.in_size) {
++// Initialize decoder for new stream
++xz_dec_end(state);
++state = xz_dec_init(XZ_DYNALLOC, 64*1024*1024);
++// Eat padding
++while (iobuf.in[iobuf.in_pos] == 0){
++ iobuf.in_pos += 1;
++}
++ }
++ // Look for other streams
++ continue;
+ }
+ if (r != XZ_OK r != XZ_UNSUPPORTED_CHECK) {
+ bb_error_msg(corrupted data);
diff -Nru busybox-1.20.0/debian/patches/series busybox-1.20.0/debian/patches/series
--- busybox-1.20.0/debian/patches/series 2012-09-19 22:58:00.0 +0200
+++ busybox-1.20.0/debian/patches/series 2012-12-20 21:54:21.0 +0100
@@ -25,3 +25,6 @@
dont-force-no-alignment-for-s390.patch
stop-checking-ancient-kernel-version.patch
+
+# http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=686502
+fix-unxz-with-multiple-streams.patch
signature.asc
Description: This is a digitally signed message part
Processed: Re: Bug#686502: pxz produces archives broken for busybox's unxz
Processing control commands: forwarded -1 https://bugs.busybox.net/show_bug.cgi?id=5804 Bug #686502 [pxz] pxz produces archives broken for busybox's unxz Set Bug forwarded-to-address to 'https://bugs.busybox.net/show_bug.cgi?id=5804'. reassign -1 src:busybox 1:1.17.1-8 Bug #686502 [pxz] pxz produces archives broken for busybox's unxz Bug reassigned from package 'pxz' to 'src:busybox'. Ignoring request to alter found versions of bug #686502 to the same values previously set Ignoring request to alter fixed versions of bug #686502 to the same values previously set Bug #686502 [src:busybox] pxz produces archives broken for busybox's unxz Marked as found in versions busybox/1:1.17.1-8. affects -1 pxz Bug #686502 [src:busybox] pxz produces archives broken for busybox's unxz Added indication that 686502 affects pxz retitle -1 busybox unxz silently fails to decompress multiple compressed streams on input Bug #686502 [src:busybox] pxz produces archives broken for busybox's unxz Changed Bug title to 'busybox unxz silently fails to decompress multiple compressed streams on input' from 'pxz produces archives broken for busybox's unxz' severity -1 grave Bug #686502 [src:busybox] busybox unxz silently fails to decompress multiple compressed streams on input Severity set to 'grave' from 'important' -- 686502: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=686502 Debian Bug Tracking System Contact ow...@bugs.debian.org with problems -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/handler.s.b686502.135599173926942.transcr...@bugs.debian.org
Bug#686502: pxz produces archives broken for busybox's unxz
On Thu, Dec 20, 2012 at 12:22:12PM +0400, Michael Tokarev wrote: This is a grave bug in busybox. Grave because it causes silent data loss - valid (according to the format specs) input is decompressed only partially. The documentation say: SHOULD support files that have more than one Stream or Stream padding. Please note the should. So missing support is no bug on its own, but it should at least check that there is no trailing garbage. Bastian -- What terrible way to die. There are no good ways. -- Sulu and Kirk, That Which Survives, stardate unknown -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20121220113037.ga27...@waldi.eu.org
Bug#686502: pxz produces archives broken for busybox's unxz
Hi,
Can you please test the attached patch
Cheers,
diff -Nru busybox-1.20.0/debian/changelog busybox-1.20.0/debian/changelog
--- busybox-1.20.0/debian/changelog 2012-09-20 08:32:55.0 +0200
+++ busybox-1.20.0/debian/changelog 2012-12-20 22:04:02.0 +0100
@@ -1,3 +1,9 @@
+busybox (1:1.20.0-7.1) unstable; urgency=low
+
+ * Fix decompression of multi stream XZ files
+
+ -- Abou Al Montacir abou.almonta...@sfr.fr Thu, 20 Dec 2012 22:04:00 +0100
+
busybox (1:1.20.0-7) unstable; urgency=low
* set CONFIG_FEATURE_COPYBUF_KB from 4 to 64 for all flavours. This
diff -Nru busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch
--- busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch 1970-01-01 01:00:00.0 +0100
+++ busybox-1.20.0/debian/patches/fix-unxz-with-multiple-streams.patch 2012-12-20 22:01:15.0 +0100
@@ -0,0 +1,21 @@
+Author: Abou Al Montacir abou.almonta...@sfr.fr
+Purpose: Fix decompression of multi stream XZ compressed files
+ (Closes: bug#686502)
+
+--- busybox-1.20.0~/archival/libarchive/decompress_unxz.c 2012-12-20 21:51:04.0 +0100
busybox-1.20.0/archival/libarchive/decompress_unxz.c 2012-12-20 21:49:11.0 +0100
+@@ -87,7 +87,13 @@
+ iobuf.out_pos = 0;
+ }
+ if (r == XZ_STREAM_END) {
+- break;
++ if (iobuf.in_pos == iobuf.in_size) {
++break;
++ } else {
++xz_dec_end(state);
++state = xz_dec_init(XZ_DYNALLOC, 64*1024*1024);
++continue;
++ }
+ }
+ if (r != XZ_OK r != XZ_UNSUPPORTED_CHECK) {
+ bb_error_msg(corrupted data);
diff -Nru busybox-1.20.0/debian/patches/series busybox-1.20.0/debian/patches/series
--- busybox-1.20.0/debian/patches/series 2012-09-19 22:58:00.0 +0200
+++ busybox-1.20.0/debian/patches/series 2012-12-20 21:54:21.0 +0100
@@ -25,3 +25,6 @@
dont-force-no-alignment-for-s390.patch
stop-checking-ancient-kernel-version.patch
+
+# http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=686502
+fix-unxz-with-multiple-streams.patch
signature.asc
Description: This is a digitally signed message part
Bug#686502: pxz produces archives broken for busybox's unxz
On Thu, Dec 20, 2012 at 10:42:41PM +0100, Abou Al Montacir wrote: Can you please test the attached patch How does it implement stream padding? Bastian -- What kind of love is that? Not to be loved; never to have shown love. -- Commissioner Nancy Hedford, Metamorphosis, stardate 3219.8 -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/20121220220835.ga5...@waldi.eu.org
Bug#686502: pxz produces archives broken for busybox's unxz
package: pxz version: 4.999.9~beta+git537418b-1 severity: important affects: busybox x-debbugs-cc: debian-boot@lists.debian.org,xz-ut...@packages.debian.org,jn...@users.sourceforge.net Hi, pxz (somtimes) produces archives broken for busybox's unxz, while they decompress fine with unxz from xz-utils packges. I noticed when trying to uncompress pxz compressed initramfs files, while this is an universal way to reproduce it: (this is sid, and busybox from squeeze behaves the same.) # get some big archive: ~/t$ apt-get source typo3 Reading package lists... Done Building dependency tree Reading state information... Done Picking 'typo3-src' as source package instead of 'typo3' NOTICE: 'typo3-src' packaging is maintained in the 'Git' version control system at: git://github.com/sir-gawain/debian-typo3.git Skipping already downloaded file 'typo3-src_4.5.19+dfsg1-1.dsc' Skipping already downloaded file 'typo3-src_4.5.19+dfsg1.orig.tar.gz' Skipping already downloaded file 'typo3-src_4.5.19+dfsg1-1.debian.tar.gz' Need to get 0 B of source archives. Skipping unpack of already unpacked source in typo3-src-4.5.19+dfsg1 # preserve it ~/t$ cp typo3-src_4.5.19+dfsg1.orig.tar.gz typo3-src_4.5.19+dfsg1.orig.tar.gz.orig # now show that busybox unxz chokes: ~/t$ gunzip typo3-src_4.5.19+dfsg1.orig.tar.gz ~/t$ pxz -z typo3-src_4.5.19+dfsg1.orig.tar ~/t$ busybox unxz typo3-src_4.5.19+dfsg1.orig.tar.xz ~/t$ tar tf typo3-src_4.5.19+dfsg1.orig.tar 1/dev/null tar: Unexpected EOF in archive tar: Error is not recoverable: exiting now # cleanup ~/t$ rm typo3-src_4.5.19+dfsg1.orig.tar ~/t$ cp typo3-src_4.5.19+dfsg1.orig.tar.gz.orig typo3-src_4.5.19+dfsg1.orig.tar.gz # show that unxz has no problems ~/t$ gunzip typo3-src_4.5.19+dfsg1.orig.tar.gz ~/t$ pxz -z typo3-src_4.5.19+dfsg1.orig.tar ~/t$ unxz typo3-src_4.5.19+dfsg1.orig.tar.xz ~/t$ tar tf typo3-src_4.5.19+dfsg1.orig.tar 1/dev/null ~/t$ echo $? 0 Any help with this is appreciated. cheers, Holger -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/201209021518.41705.hol...@layer-acht.org
Re: Bug#686502: pxz produces archives broken for busybox's unxz
Hi Bastian, On Sonntag, 2. September 2012, Bastian Blank wrote: On Sun, Sep 02, 2012 at 03:18:40PM +0200, Holger Levsen wrote: ~/t$ busybox unxz typo3-src_4.5.19+dfsg1.orig.tar.xz ~/t$ tar tf typo3-src_4.5.19+dfsg1.orig.tar 1/dev/null tar: Unexpected EOF in archive tar: Error is not recoverable: exiting now Where does busybox choke here? I only see tar choking on the result of the decompression. What do you find in the file? understandable, my instructions to reproduce it hide the problem a bit ;) tar fails, because busybox unxz fails to decompress correctly (as can be seen in the filesize) despite exiting with exit code 0: ~/t$ gunzip typo3-src_4.5.19+dfsg1.orig.tar.gz ~/t$ ls -l typo3-src_4.5.19+dfsg1.orig.tar -rw-r--r-- 1 me me 51845120 Aug 18 05:36 typo3-src_4.5.19+dfsg1.orig.tar ~/t$ pxz -z typo3-src_4.5.19+dfsg1.orig.tar ~/t$ busybox unxz typo3-src_4.5.19+dfsg1.orig.tar.xz ~/t$ echo $? 0 ~/t$ ls -l typo3-src_4.5.19+dfsg1.orig.tar -rw-r--r-- 1 me me 25169920 Sep 2 14:32 typo3-src_4.5.19+dfsg1.orig.tar cheers, Holger -- To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/201209021634.44021.hol...@layer-acht.org
