bytedeco / javacpp

The missing bridge between Java and native C++
Other
4.49k stars 583 forks source link

Pointer.asByteBuffer() behavior on large arrays. #557

Closed masterav10 closed 2 years ago

masterav10 commented 2 years ago

Discovered this while attempting to use a large BytePointer as a native cache. When attempting to get a ByteBuffer from the higher end of the array (position > Integer.MAX_VALUE), the buffer's capacity, limit, and position get truncated or is otherwise invalid.

A simple test case to demonstrate the problem:

    @Test public void testByteBufferIndexingUsingLargePointers() {
        final long bigSize = Integer.MAX_VALUE + 1L;

        try(BytePointer pointer = new BytePointer(bigSize))
        {
            pointer.fill(1);
            pointer.put(pointer.capacity() - 1L, (byte) 100);

            ByteBuffer buffer = pointer.position(Integer.MAX_VALUE)
                    .asByteBuffer();

            assertEquals(0L, buffer.position());
            assertEquals(1L, buffer.remaining());
            assertEquals((byte)100, buffer.get());
        }
    }

An easy solution would be to reimplement the method to work like ByteBuffer.slice(). The returned ByteBuffer would have a position of 0, limit + capacity of remaining(), and we simply shift the address by the appropriate number of bytes.

My assessment is that fixing this will have a relatively low impact on client code; it usually doesn't make sense to mess with position() and limit() in absolute numbers if at all. I imagine most people are using the returned ByteBuffer as a way to read the data stored in the pointer.

Unless there is something glaring I'm going to take stab at fixing this.

saudet commented 2 years ago

The thing with Pointer.asByteBuffer() is that it's meant to copy the address and position as is...

Anyway, since Pointer.getPointer() has been introduced, I don't recommend using Pointer.position() for anything else than advanced optimization. So, just use Pointer.getPointer(), it should already do what you need it to do here, but please let me know if this isn't the case.

masterav10 commented 2 years ago

Your presented solution does indeed work.

It's still a bug based on how the method is documented. I don't like the idea of throwing an exception if one of the parameters is outside the supported range of a ByteBuffer.

The thing with Pointer.asByteBuffer() is that it's meant to copy the address and position as is...

Is there some benefit of preserving the address and position? Other than for posterity.

saudet commented 2 years ago

Right, the docs could be updated to clarify more explicitly what it's doing. Mind sending a pull request for that?

As for not changing the behavior, well, for backward compatibility. There's no reason to "fix" this because it works with Pointer.getPointer(), which we should be using anyway.

saudet commented 2 years ago

Thanks for the contribution! Let me know if you encounter anything else unclear like this.