ocaml-lib-devel

[Ocaml-lib-devel] Some small bug fixes to extlib

[Robert A. <bob...@ed...>]

Hello,

We've found some tiny bugs in extlib-1.5:

- off by one error in IO.ml for writing 16 bit integers
- off by one error in unzip.ml in reading of uncompressed blocks
- fix possible under-reading in unzip.ml

Patch attached.

Bob

Re: [Ocaml-lib-devel] Some small bug fixes to extlib

[Janne H. <jjh...@gm...>]

Hi,

> We've found some tiny bugs in extlib-1.5:
>
> - off by one error in IO.ml for writing 16 bit integers
> - off by one error in unzip.ml in reading of uncompressed blocks
> - fix possible under-reading in unzip.ml
>
> Patch attached.

Do you happen to have test cases for these failures?  I'd like to add
test cases for these into the extlib test suite.

Janne

Re: [Ocaml-lib-devel] Some small bug fixes to extlib

[Robert A. <bob...@ed...>]

On Mon, 2007-11-26 at 20:51 +0200, Janne Hellsten wrote:
> Hi,
> 
> > We've found some tiny bugs in extlib-1.5:
> >
> > - off by one error in IO.ml for writing 16 bit integers
> > - off by one error in unzip.ml in reading of uncompressed blocks
> > - fix possible under-reading in unzip.ml
> >
> > Patch attached.
> 
> Do you happen to have test cases for these failures?  I'd like to add
> test cases for these into the extlib test suite.

Actually, looking at it again, I'm not sure the third one is a bug: the
code that is already present handles the case of under-reading.

I've attached a short ocaml program with test cases for the other two.

Bob

Re: [Ocaml-lib-devel] Some small bug fixes to extlib

[Janne H. <jjh...@gm...>]

Hi Rob,

Thanks for the test case, I agree that the write_i16 range check is
indeed erroneous, -32768 should be accepted as it can be represented a
signed 16-bit integer.

  (* Bug was that write_i16 did not accept -0x8000 *)
  let out = IO.output_string () in
  print_string "Test IO.write_i16: ";
  let ()  =
    try IO.write_i16 out (-0x8000); print_string "OK\n"
    with IO.Overflow _ -> print_string "FAIL\n"

I'm not familiar with the Unzip module, but it seems like you know
what the problem is.  I'll let Nicolas answer this one.

I'll integrate your test, let's see if we'll incorporate the patch as well.

Janne

On Nov 27, 2007 4:36 PM, Robert Atkey <bob...@ed...> wrote:
>
>
> On Mon, 2007-11-26 at 20:51 +0200, Janne Hellsten wrote:
> > Hi,
> >
> > > We've found some tiny bugs in extlib-1.5:
> > >
> > > - off by one error in IO.ml for writing 16 bit integers
> > > - off by one error in unzip.ml in reading of uncompressed blocks
> > > - fix possible under-reading in unzip.ml
> > >
> > > Patch attached.
> >
> > Do you happen to have test cases for these failures?  I'd like to add
> > test cases for these into the extlib test suite.
>
> Actually, looking at it again, I'm not sure the third one is a bug: the
> code that is already present handles the case of under-reading.
>
> I've attached a short ocaml program with test cases for the other two.
>
> Bob
>

Re: [Ocaml-lib-devel] Some small bug fixes to extlib

[Janne H. <jjh...@gm...>]

I applied your patch for the write_i16 but I didn't apply the Unzip fix yet.

I also added both the Unzip and IO test cases into the test suite
(/cvsroot/ocaml-lib/extlib-test).  Applying your second unzip change
to extlib-dev does make the test case pass.

Rob, how extensively have you tested your Unzip change?  I'm not
doubting that your change wouldn't work, I just don't have any test
coverage to support your case. :(

Janne

On Nov 27, 2007 9:28 PM, Janne Hellsten <jjh...@gm...> wrote:
> Hi Rob,
>
> Thanks for the test case, I agree that the write_i16 range check is
> indeed erroneous, -32768 should be accepted as it can be represented a
> signed 16-bit integer.
>
>   (* Bug was that write_i16 did not accept -0x8000 *)
>   let out = IO.output_string () in
>   print_string "Test IO.write_i16: ";
>   let ()  =
>     try IO.write_i16 out (-0x8000); print_string "OK\n"
>     with IO.Overflow _ -> print_string "FAIL\n"
>
> I'm not familiar with the Unzip module, but it seems like you know
> what the problem is.  I'll let Nicolas answer this one.
>
> I'll integrate your test, let's see if we'll incorporate the patch as well.
>
> Janne
>
>
> On Nov 27, 2007 4:36 PM, Robert Atkey <bob...@ed...> wrote:
> >
> >
> > On Mon, 2007-11-26 at 20:51 +0200, Janne Hellsten wrote:
> > > Hi,
> > >
> > > > We've found some tiny bugs in extlib-1.5:
> > > >
> > > > - off by one error in IO.ml for writing 16 bit integers
> > > > - off by one error in unzip.ml in reading of uncompressed blocks
> > > > - fix possible under-reading in unzip.ml
> > > >
> > > > Patch attached.
> > >
> > > Do you happen to have test cases for these failures?  I'd like to add
> > > test cases for these into the extlib test suite.
> >
> > Actually, looking at it again, I'm not sure the third one is a bug: the
> > code that is already present handles the case of under-reading.
> >
> > I've attached a short ocaml program with test cases for the other two.
> >
> > Bob
> >
>

Re: [Ocaml-lib-devel] Some small bug fixes to extlib

[Robert A. <bob...@ed...>]

On Tue, 2007-11-27 at 22:24 +0200, Janne Hellsten wrote:
> I applied your patch for the write_i16 but I didn't apply the Unzip fix yet.
> 
> I also added both the Unzip and IO test cases into the test suite
> (/cvsroot/ocaml-lib/extlib-test).  Applying your second unzip change
> to extlib-dev does make the test case pass.
> 
> Rob, how extensively have you tested your Unzip change?  I'm not
> doubting that your change wouldn't work, I just don't have any test
> coverage to support your case. :(

I have tested the modified Unzip module against the rt.jar files from
several versions of the Sun JVM, plus assorted other jar files, and it
seems to work. It is interesting that in all of these files, the only
uncompressed block that triggers the bug was in the file
sun/util/resources/TimeZoneNames_fr.class in the jre/lib/rt.jar file of
Sun's JVM version 1.6.0.01.

I've made the test case a little more convincing now by taking the
output of zlib and using that as the test data (there was a harmless
mistake in the test data I gave previously: the order of the bits in the
first byte was switched).

The new test case is:

let test_unzip_bug1 () =
  let test data =
    let input    = IO.input_string data in
    let unzipped = Unzip.inflate input in
      try
	let str      = IO.read_all unzipped in
	  assert (str = "XY")
      with Unzip.Error Unzip.Invalid_data -> assert false
  in
    (* this is "XY" compressed by zlib at level 9 *)
    test "\x78\xda\x8b\x88\x04\x00\x01\x0b\x00\xb2";
    (* this is "XY" compressed by zlib at level 0 *)
    test "\x78\x01\x01\x02\x00\xfd\xff\x58\x59\x01\x0b\x00\xb2"

The unmodified Unzip is OK on the first one, but fails on the second.
The modified Unzip succeeds on both.

I've attached the C program I used to generate the test data. I guess a
whole range of test data could be generated to test the Unzip module.

Bob

Re: [Ocaml-lib-devel] Some small bug fixes to extlib

[Robert A. <bob...@ed...>]

On Wed, 2007-11-28 at 19:17 +0000, Robert Atkey wrote:

> The unmodified Unzip is OK on the first one, but fails on the second.
> The modified Unzip succeeds on both.

I should add that the bug is that the Unzip module attempts to calculate
the 16bit one's complement of a value by 0x10000 - x, where it should be
0xffff - x. Uncompressed blocks in the DEFLATE format are stored as:
  size of data (2 bytes, little endian)
  one's complement of size of data (2 bytes, little endian)
  the data...

The unzip module checks the agreement between the two sizes incorrectly
and throws an exception on valid data.

Bob

Re: [Ocaml-lib-devel] Some small bug fixes to extlib

[Janne H. <jjh...@gm...>]

Hi Robert,

> I've attached the C program I used to generate the test data. I guess a
> whole range of test data could be generated to test the Unzip module.

I created a bit more test coverage with your zlib-test.c and created a
second Unzip test case.  The generator program is in
extlib-test/util/zlib-test.  The coverage is nowhere near perfect but
should be better than nothing.

I've committed your bug fix into CVS after having tested Unzip module
with the new, somewhat improved test.

Now all we need is a new release :)

Janne