Bug 41628

Created attachment 52152 [details] [review]
Proposed patch

Could you send this to xorg-devel for review.

I'm confused about why -b 9 still bails out early.  I have no trouble 
uncompressing files compressed with 'compress -b 9' here.  Do you have an 
example where compress itself fails?

(In reply to comment #2)
> Could you send this to xorg-devel for review.

Posted, waiting for moderator.

> I'm confused about why -b 9 still bails out early.  I have no trouble 
> uncompressing files compressed with 'compress -b 9' here.  Do you have an 
> example where compress itself fails?

Have you tried any non-trivial input?  What compress do you use?

AFAICS, uncompress works well for trivial inputs as:
  echo test | compress -b 9 | uncompress

However, it fails for larger inputs where all 2^9 code words are used.  E.g. this fails to uncompress for me with ncompress, gzip and busybox uncompress:
  cat src/fontfile/decompress.c | compress -b 9

The problem is here:
http://cgit.freedesktop.org/xorg/lib/libXfont/tree/src/fontfile/decompress.c?id=bd48ad11fd#n326

For maxbits == 9, this allows incrementing n_bits to 10.  Therefore, the next code will be read as 10 bit code, and eventually trigger code > file->free_ent error check.  This problem seems to date back to the original BSD compress:

http://minnie.tuhs.org/cgi-bin/utree.pl?file=4.3BSD-Reno/src/usr.bin/compress/compress.c

My original concern was this could lead to buffer overflow in libXfont (which uses dynamic allocation for tab_suffix/tab_prefix, while other implementations usually use static buffers large enough for maxbits == 16), but that can't happen due to checks here (maxmaxcode is 1 << maxbits and not changed while uncompressing):

http://cgit.freedesktop.org/xorg/lib/libXfont/tree/src/fontfile/decompress.c?id=bd48ad11fd#n276

I think it can be done both ways.  I decided to reject 9 early, given that it's unlikely to be used and handled incorrectly in most cases anyway.

(In reply to comment #3)
> Have you tried any non-trivial input?  What compress do you use?

No, just a couple of trivial testcases.  I'm using compress(1) from Lion which is based on FreeBSD:
__FBSDID("$FreeBSD: src/usr.bin/compress/compress.c,v 1.21 2003/06/14 13:41:31 trhodes Exp $");

http://opensource.apple.com/source/file_cmds/file_cmds-212/compress/compress.c

> However, it fails for larger inputs where all 2^9 code words are used.  E.g.
> this fails to uncompress for me with ncompress, gzip and busybox uncompress:
>   cat src/fontfile/decompress.c | compress -b 9

Works here with BSD compress

cat src/fontfile/decompress.c | compress -b 9 > a.Z
uncompress a.Z
diff a src/fontfile/decompress.c
 
> The problem is here:
> http://cgit.freedesktop.org/xorg/lib/libXfont/tree/src/fontfile/decompress.c?id=bd48ad11fd#n326
> 
> For maxbits == 9, this allows incrementing n_bits to 10.  Therefore, the next
> code will be read as 10 bit code, and eventually trigger code > file->free_ent
> error check.  This problem seems to date back to the original BSD compress:
> 
> http://minnie.tuhs.org/cgi-bin/utree.pl?file=4.3BSD-Reno/src/usr.bin/compress/compress.c
> 
> My original concern was this could lead to buffer overflow in libXfont (which
> uses dynamic allocation for tab_suffix/tab_prefix, while other implementations
> usually use static buffers large enough for maxbits == 16), but that can't
> happen due to checks here (maxmaxcode is 1 << maxbits and not changed while
> uncompressing):
> 
> http://cgit.freedesktop.org/xorg/lib/libXfont/tree/src/fontfile/decompress.c?id=bd48ad11fd#n276
> 
> I think it can be done both ways.  I decided to reject 9 early, given that it's
> unlikely to be used and handled incorrectly in most cases anyway.

Ok, given that there are bad versions of compress in the wild with issues, it seems reasonable to reject -b 9

Reviewed and pushed, thanks.

(In reply to comment #4)
> No, just a couple of trivial testcases.  I'm using compress(1) from Lion which
> is based on FreeBSD:
> __FBSDID("$FreeBSD: src/usr.bin/compress/compress.c,v 1.21 2003/06/14 13:41:31
> trhodes Exp $");

...

> Works here with BSD compress

Ok, thank you for the correction.  I confirm compress -b 9 | uncompress works with that version.  uncompress has the same issue, but it seems to be compensated by the same bug in compress, which got fixed in ncompress.  I can uncompress 9-bit BSD compress output with ncompress too.

> Ok, given that there are bad versions of compress in the wild with issues, it
> seems reasonable to reject -b 9

They are probably fewer than I thought, and less relevant, so either way should be fine.