[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Alternative nSelectors patch (Was: bzip2 1.0.7 released)


Hi Mark,

This seems to me like a better patch than my proposal, so I retract my
proposal and vote for this one instead.

The one thing that concerned me was that, it would be a disaster -- having
ignored all selectors above 18002 -- if subsequent decoding actually *did*
manage somehow to try to read more than 18002 selectors out of s->selectorMtf,
because we'd be reading uninitialised memory.  But this seems to me can't
happen because, after the selector-reading loop, you added

+      if (nSelectors > BZ_MAX_SELECTORS)
+        nSelectors = BZ_MAX_SELECTORS;

and the following loop:

      /*--- Undo the MTF values for the selectors. ---*/
      ...

is the only place that reads s->selectorMtf, and then only for the range
0 .. nSelectors-1.

So it seems good to me.  Does this sync with your analysis?

J


On 01/07/2019 01:36, Mark Wielaard wrote:

Hi,

On Fri, 2019-06-28 at 13:10 +0200, Mark Wielaard wrote:

It seems to me to be important to now split BZ_MAX_SELECTORS into these two
parts so as to make it clear to everybody that we're accepting (decompressing)
a slightly larger set of inputs than we create (a la that old saying about
network protocol implementations), so as to tolerate other compressors.


That seems good. The attached patch does this and makes it possible to
decode the problematic bz2 file.


Sorry, it is a bit too late here to properly document this patch and
explain why I think it is a better one than the "split-max-selectors"
fix. But hopefully the new testsuite example and the comment in the
patch make clear what my thinking is.

This resolved both the issue with the large file reported as with the
new test suite file (lbzip2/32767.bz2). The whole testsuite passes now,
even under valgrind and with gcc -fsanitize=undefined.

Comments on the patch idea more than welcome.

Thanks,

Mark