KryoException because Input.require() does not properly compact full Input buffer

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Use Kryo to serialize at least two objects of lengths M and N
2. Create an new Input of size M.
3. Read the first object from the Input, then attempt to read the second object.

(See an example below where M=N=512b.)

What is the expected output? What do you see instead?

Instead of deserialization, a a "Buffer underflow" KryoException is thrown.

Exception in thread "main" java.lang.RuntimeException: 
com.esotericsoftware.kryo.KryoException: Buffer underflow.
    at org.bailis.kryobug.main(KryoBug.java:64)
Caused by: com.esotericsoftware.kryo.KryoException: Buffer underflow.
    at com.esotericsoftware.kryo.io.Input.require(Input.java:159)
    at com.esotericsoftware.kryo.io.Input.readBytes(Input.java:321)
    at com.esotericsoftware.kryo.io.Input.readBytes(Input.java:306)
    at org.bailis.kryobug.main(KryoBug.java:61)

What version of the Kryo are you using?
2.21

Please provide any additional information below.

This appears to have been introduced as part of Issue 89 (r352).

It appears that, to fix this bug, the first invocation of fill() in require() 
should only occur if remaining > 0. This can be accomplished via the patch 
below.

Fortunately, 2.20 does not have this error since it compacts correctly:
https://code.google.com/p/kryo/source/browse/trunk/src/com/esotericsoftware/kryo
/io/Input.java?r=338#148

Example below:

ByteBuffer buf = ByteBuffer.allocate(1024);
ByteBufferOutputStream byteBufferOutputStream = new ByteBufferOutputStream(buf);
Output testOutput = new Output(byteBufferOutputStream);
testOutput.writeBytes(new byte[512]);
testOutput.writeBytes(new byte[512]);
testOutput.flush();

ByteBufferInputStream testInputs = new ByteBufferInputStream();
buf.flip();
testInputs.setByteBuffer(buf);
Input input = new Input(testInputs, 512);
byte[] toRead = new byte[512];
input.readBytes(toRead);

//this will throw a KryoException
input.readBytes(toRead);

PATCH

Index: src/com/esotericsoftware/kryo/io/Input.java
===================================================================
--- src/com/esotericsoftware/kryo/io/Input.java (revision 384)
+++ src/com/esotericsoftware/kryo/io/Input.java (working copy)
@@ -154,14 +154,18 @@
        if (remaining >= required) return remaining;
        if (required > capacity) throw new KryoException("Buffer too small: capacity: " + capacity + ", required: " + required);

+       int count;
+
        // Try to fill the buffer.
-       int count = fill(buffer, limit, capacity - limit);
-       if (count == -1) throw new KryoException("Buffer underflow.");
-       remaining += count;
-       if (remaining >= required) {
-           limit += count;
-           return remaining;
-       }
+       if (remaining > 0) {
+           count = fill(buffer, limit, capacity - limit);
+           if (count == -1) throw new KryoException("Buffer underflow.");
+           remaining += count;
+           if (remaining >= required) {
+               limit += count;
+                   return remaining;
+       }
+        }

        // Was not enough, compact and try again.
        System.arraycopy(buffer, position, buffer, 0, remaining);

Original issue reported on code.google.com by pbailis@gmail.com on 7 Aug 2013 at 1:55

[GoogleCodeExporter]

The implementation of optional() may have a similar problem.

Original comment by pbailis@gmail.com on 7 Aug 2013 at 3:08

[GoogleCodeExporter]

Thanks for this bug report. It is reproducible and your fix seems to work just 
fine. All unit tests are green.  There is a similar place in 
ByteBufferInput#require which can be fixed in the same way.

I'll test a bit more and commit a fix.

-Leo

Original comment by romixlev on 8 Aug 2013 at 8:49

[GoogleCodeExporter]

@pbailis: I'm not so sure about optional. Do you have a test case for checking 
if it is broken or not?

Original comment by romixlev on 8 Aug 2013 at 8:53

[GoogleCodeExporter]

@romixlev: I haven't tested optional(); it may not be problematic, but it 
looked like a similar code path was introduced in the r352, which is why I 
pointed it out!

Original comment by pbailis@gmail.com on 8 Aug 2013 at 11:28

[GoogleCodeExporter]

OK. Fix is committed into trunk. Optional is not touched yet. If anyone will 
report problems, we'll fix it.

-Leo

Original comment by romixlev on 9 Aug 2013 at 9:33

• Changed state: Fixed