Buffer should throw IndexOutOfBoundsException reading beyond the logical end

[gmcrobert]

Version

Which version(s) did you encounter this bug ? Vertx 4.0

Context

If you create an empty Buffer using Buffer.buffer() and then append some data, you can read beyond the logical end of the buffer.

Do you have a reproducer?

An Empty buffer correctly identifies that it is wrong to read past the logical end of the buffer so this test passes:

@Test
void correctBufferBehaviourTest1() {
    Buffer buffer = Buffer.buffer();
    assertThrows(IndexOutOfBoundsException.class, () -> buffer.getInt(4));
}

If I create a buffer using a hint then it also correctly detects that it is wrong to read past the logical end of the buffer so this test passes:

@Test
void correctBufferBehaviourTest2() {
    Buffer buffer = Buffer.buffer(4);
    buffer.appendBuffer(Buffer.buffer(new byte[] {0, 0, 0, 99}));
    assertEquals(4, buffer.length());
    assertEquals(99, buffer.getInt(0));
    assertThrows(IndexOutOfBoundsException.class, () -> buffer.getInt(4));
}

If I create an empty buffer and append some data then I can read beyond the logical end of the buffer so this test fails but it should pass:

@Test
void questionableBufferBehaviour() {
    Buffer buffer = Buffer.buffer();
    buffer.appendBuffer(Buffer.buffer(new byte[] {0, 0, 0, 99}));
    assertEquals(4, buffer.length());
    assertEquals(99, buffer.getInt(0));
    assertThrows(IndexOutOfBoundsException.class, () -> buffer.getInt(4));
}

[vietj]

did you check with Vert.x 3 ?

[gmcrobert]

Version 3.9.5 displays the same behaviour.

[gmcrobert]

So it looks like this problem arises because the get... methods map directly to the Netty ByteBuf equivalent methods which don't honour the readIndex/writeIndex and readableBytes but do honour the end of the physical buffer. In Netty, the read... methods do honour the readIndex/writeIndex and readableBytes so will throw an IndexOutOfBoundsException but the behaviour of the reads are similar to nio buffers in that they increase the readIndex. To behave properly, I think that you will have to put a check in that ensures the area being fetched falls between the readIndex and writeIndex. The alternative is to do nothing as it has been like that for some time and maybe make it a little clearer in the documentation but, in my view, the current system can return invalid results so it would be better to fix it. I am happy to make any code or doc changes if need it to be done but I will need some guidance.

[vietj]

@gmcrobert I agree with you - I will check the behavior on Netty side for the getXXX methods and get back to this

[vietj]

Actually as I read it, indeed getXXX method only cares about capacity and not the reader or writer index.

I need to check how this behaves with buffer having a different reader index too.

[vietj]

so I think we will add checks using the writer index of the buffer instead of the capacity

[vietj]

See https://github.com/eclipse-vertx/vert.x/issues/3775