[PATCH 2/2] (g)unzip: Optimize inflate_codes()

[Rob Landley]

On Friday 12 February 2010 21:47:19 Denys Vlasenko wrote:
> On Friday 12 February 2010 00:46, Joakim Tjernlund wrote:
> > Denys Vlasenko <vda.linux at googlemail.com> wrote on 2010/02/12 00:28:44:
> > > On Friday 12 February 2010 00:14, Joakim Tjernlund wrote:
> > > > Denys Vlasenko <vda.linux at googlemail.com> wrote on 2010/02/11 
23:52:30:
> > > > > From: Denys Vlasenko <vda.linux at googlemail.com>
> > > > > To: Joakim Tjernlund <joakim.tjernlund at transmode.se>
> > > > > Cc: busybox at busybox.net, Rob Landley <rob at landley.net>
> > > > > Date: 2010/02/11 23:52
> > > > > Subject: Re: [PATCH 2/2] (g)unzip: Optimize inflate_codes()
> > > > >
> > > > > On Thursday 11 February 2010 08:42, Joakim Tjernlund wrote:
> > > > > > > Great. But looks like you forgot to send the patch without this
> > > > > > > debug. The only patch I found has this:
> > > > > > >
> > > > > > > +                                       /* Align out addr */
> > > > > > > +                                       if (e < 3)
> > > > > > > +                                               fprintf(stderr,
> > > > > > > "error
> > > > >
> > > > > len:%d\n", e);
> > > > >
> > > > > > Hehe, here we go then. Looking at the gzip code I think it is
> > > > > > crap though. The upstream gzip code is old and unoptimized. One
> > > > > > should just scrap it and redo it with zlib instead.
> > > > >
> > > > > It segfaults on 277 Mb gz file (a source tree of old openoffice
> > > > > version):
> > > >
> > > > what arch?
> > >
> > > x86-32
> > >
> > > > > # time ./busybox_old gunzip <OOo_2.0.2_src.tar.gz >/dev/null
> > > > >
> > > > > real    0m13.616s
> > > > > user    0m13.493s
> > > > > sys     0m0.116s
> > > > > # time ./busybox gunzip <OOo_2.0.2_src.tar.gz >/dev/null
> > > > > /bin/bash: line 1: 15635 Segmentation fault      ./busybox gunzip <
> > > > > OOo_2.0. 2_src.tar.gz > /dev/null
> > > > >
> > > > > real    0m2.198s
> > > > > user    0m2.186s
> > > > > sys     0m0.011s
> > > >
> > > > Maybe you could stick that debug printout back?
> > >
> > > # ./busybox gunzip <OOo_2.0.2_src.tar.gz | md5sum
> > > gunzip: error len:2
> > > gunzip: error len:2
> > > 85bff4b99d1516ace2e8d11bcc4718f5  -
> >
> > I was afraid this could happen. Either this .gz is compressed with an old
> > faulty gzip or my patch is buggy :(
> > Could you try to recompress the archive with latest gzip?
>
> Recompressed with gzip 1.3.12, which made archive bigger (!!?):

At a guess, -9 isn't the default?  (Default optimized for speed, not size.)

And if the external gunzip can decompress the archive and get the data back, 
then there's nothing wrong with the archive and busybox had better be able to 
deal with that specific test case.  (Test cases that break your code but not 
other implementations are precious commodities.  I go out of my way to hang on 
to 'em...)

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds