Java: ByteString / UnsafeDirectNioDecoder share ByteBuffer instance in non-threadsafe way

[mernst-github]

Version: v3.21.6 Language: Java

Any platform with UnsafeDirectNioDecoder.isSupported

Multiple aliasing UnsafeDirectNioDecoders reading from the same string concurrently may observe corrupted input:

A ByteString created via UnsafeByteOperations.unsafeWrap(ByteBuffer) from a single direct buffer is an instance of NioByteString which holds this buffer. NioByteString.newCodedInput shares the buffer instance with the returned UnsafeDirectNioDecoder. If enableAliasing, readBytes will return a read-only slice of the input buffer. That's all fine. However, the way it creates the slice is by modifying the buffer positions (UnsafeDirectNioDecoder#slice sets position and limit, slices, then unsets).

This is not thread-safe: if multiple decoders operate on the same buffer, they will interfere with each other.

RopeByteString, in contrast, does this correctly: it passes duplicates of its buffers (asReadOnlyByteBufferList). NioByteString#newCodedInput should do the same and pass asReadOnlyByteBuffer(), so each stream has its own instance to muck around with.

Workaround: instead of doing string.newCodedInput(), call string.substring(0).newCodedInput() which performs the intended buffer duplication.

Repro:

public class UnsafeDirectNioDecoderTest {

  public static void main(String[] args) throws IOException {
    // Sets up a buffer with a single embedded byte string: <len=1><1 byte payload>
    ByteBuffer bb = ByteBuffer.allocateDirect(2);
    bb.put((byte) 1);
    bb.put((byte) '!');
    bb.flip();
    ByteString string = UnsafeByteOperations.unsafeWrap(bb.asReadOnlyBuffer());

    // Note: bb contents no longer modified from here. This is not an application problem.

    // Fine with single reader.
    {
      CodedInputStream input = string.newCodedInput();
      input.enableAliasing(true);
      assertEquals(1, input.readBytes().size());
    }

    // Multiple concurrent readers observe corruption:
    for (int i = 0; i < 5; ++i) {
      new Thread(() -> {
        try {
          while (true) {
            CodedInputStream input = string.newCodedInput();
            input.enableAliasing(true);
            assertEquals(1, input.readBytes().size());
          }
        } catch (IOException e) {
          e.printStackTrace();
        }
      }).start();
    }
  }

  static void assertEquals(int expected, int actual) {
    if (expected != actual) {
      throw new IllegalStateException(String.format("%d != %d", expected, actual));
    }
  }
}

This reproducer should simply run silently. Instead it generates exceptions like: com.google.protobuf.InvalidProtocolBufferException: While parsing a protocol message, the input ended unexpectedly in the middle of a field. This could mean either that the input has been truncated or that an embedded message misreported its own length. or java.lang.IllegalStateException: 1 != 2