[Toybox] [PATCH] Clean up xz a good amount

[enh]

On Wed, Feb 28, 2024 at 9:13 AM enh <enh at google.com> wrote:
>
> On Tue, Feb 27, 2024 at 8:34 PM Rob Landley <rob at landley.net> wrote:
> >
> > On 2/27/24 21:27, Oliver Webb wrote:
> > > 2 are help text, do_xzcat was due to replacing the case statement for error handling with a array
> > > If I had to guess, xz_dec_run or xz_dec_bcj_reset
> > >
> > > It was the case statement I stripped out in xz_dec_bcj_reset, it works with the attached patch
> >
> > Let's see..
> >
> > > diff --git a/toys/pending/xzcat.c b/toys/pending/xzcat.c
> > > index 5177f243..b50c9524 100644
> > > --- a/toys/pending/xzcat.c
> > > +++ b/toys/pending/xzcat.c
> > > @@ -55,6 +55,7 @@ enum xz_ret {
> > >    // truncated or otherwise corrupt.
> > >  };
> > >
> > > +
> > >  // Passing input and output buffers to XZ code
> > >  // Only the contents of the output buffer from out[out_pos] onward, and
> > >  // the variables in_pos and out_pos are modified by the XZ code.
> >
> > Whitespace change.
> >
> > > @@ -212,7 +213,7 @@ struct xz_dec_bcj {
> > >  // in an x86 instruction.
> > >  static int bcj_x86_test_msbyte(char b)
> > >  {
> > > -  return !b || !~b;
> > > +  return !b || b == 0xFF;
> > >  }
> >
> > NOP.
> >
> > >  static size_t bcj_x86(struct xz_dec_bcj *s, char *buf, size_t size)
> > > @@ -639,6 +640,20 @@ enum xz_ret xz_dec_bcj_run(struct xz_dec_bcj *s, struct xz_dec_lzma2 *lzma2,
> > >   */
> > >  enum xz_ret xz_dec_bcj_reset(struct xz_dec_bcj *s, char id)
> > >  {
> > > +  switch (id) {
> > > +  case BCJ_X86:
> > > +  case BCJ_POWERPC:
> > > +  case BCJ_IA64:
> > > +  case BCJ_ARM:
> > > +  case BCJ_ARMTHUMB:
> > > +  case BCJ_SPARC:
> > > +    break;
> > > +
> > > +  default:
> > > +    /* Unsupported Filter ID */
> > > +    return XZ_OPTIONS_ERROR;
> > > +  }
> > > +
> > >    s->type = id;
> > >    s->ret = XZ_OK;
> > >    s->pos = 0;
>
> ah, crap, that's another thing to put on the riscv64 to-do list...
> (thanks for bringing that to light!)

so, TIL that upstream already added a risc-v bcj implementation...

...but i only learned that because i was looking at
https://www.openwall.com/lists/oss-security/2024/03/29/4 which was
fascinating in many ways.

(rob will of course be delighted to hear of systemd's involvement in
the exploit chain :-) )

> > NOP unless you hit the error path, what are the values...
> >
> >   enum {
> >     BCJ_X86 = 4,        /* x86 or x86-64 */
> >     BCJ_POWERPC = 5,    /* Big endian only */
> >     BCJ_IA64 = 6,       /* Big or little endian */
> >     BCJ_ARM = 7,        /* Little endian only */
> >     BCJ_ARMTHUMB = 8,   /* Little endian only */
> >     BCJ_SPARC = 9       /* Big or little endian */
> >   } type;
> >
> > Since id is char, which is unsigned in toybox, this is:
> >
> >   if ((id-4)>5) return XZ_OPTIONS_ERROR;
> >
> > > @@ -2860,16 +2875,17 @@ void do_xzcat(int fd, char *name)
> > >      if (ret == XZ_OK || ret == XZ_UNSUPPORTED_CHECK)
> > >        continue;
> > >
> > > -    else if (ret == XZ_STREAM_END) {
> > > -      xz_dec_end(s);
> > > -      return;
> > > -    }
> > > -
> > >      if (fwrite(out, 1, b.out_pos, stdout) != b.out_pos) {
> > >        msg = "Write error\n";
> > >        goto error;
> > >      }
> > >
> > > +    if (ret == XZ_STREAM_END) {
> > > +      xz_dec_end(s);
> > > +      return;
> > > +    }
> > > +
> > > +
> > >      msg = (ret-3 < ARRAY_LEN(errors)) ? errors[ret-3] : "Bug!";
> > >      goto error;
> > >    }
> >
> > There's the bug, moving a return before the fwrite().
> >
> > Rob
> > _______________________________________________
> > Toybox mailing list
> > Toybox at lists.landley.net
> > http://lists.landley.net/listinfo.cgi/toybox-landley.net