Repository: protocolbuffers/protobuf (language: python)

Issue:
Duplicate buffer for thread-safe aliasing.
Fixes protocolbuffers#10697: UnsafeDirectNioDecoder modifies shared buffer instance when performing aliased read.

I've included a test-ish reproducer in form of a unit test that fails fairly reliably, let me know whether you'd want this merged.



Discussion / hints:
Can someone review this? This is an actual thread-safety issue (albeit a probably pretty rarely used code path).

Fix patch:
diff --git a/java/core/src/main/java/com/google/protobuf/CodedInputStream.java b/java/core/src/main/java/com/google/protobuf/CodedInputStream.java
index 85b7a5bcae1e..f0940941b05a 100644
--- a/java/core/src/main/java/com/google/protobuf/CodedInputStream.java
+++ b/java/core/src/main/java/com/google/protobuf/CodedInputStream.java
@@ -1306,7 +1306,8 @@ static boolean isSupported() {
     }
 
     private UnsafeDirectNioDecoder(ByteBuffer buffer, boolean immutable) {
-      this.buffer = buffer;
+      // Duplicate to avoid non-threadsafe modifications in slice()
+      this.buffer = buffer.duplicate();
       address = UnsafeUtil.addressOffset(buffer);
       limit = address + buffer.limit();
       pos = address + buffer.position();


Test patch:
diff --git a/java/core/src/test/java/com/google/protobuf/CodedInputStreamTest.java b/java/core/src/test/java/com/google/protobuf/CodedInputStreamTest.java
index f73cb3b0eecf..f9b0c806b50d 100644
--- a/java/core/src/test/java/com/google/protobuf/CodedInputStreamTest.java
+++ b/java/core/src/test/java/com/google/protobuf/CodedInputStreamTest.java
@@ -28,6 +28,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.function.Supplier;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -1405,6 +1406,41 @@ public void testReadByteBufferAliasing() throws Exception {
     }
   }
 
+  @Test
+  public void testByteBufferInputStreamReadBytesWithAliasConcurrently() {
+    int size = 127;
+    assertThat(CodedOutputStream.computeInt32SizeNoTag(size)).isEqualTo(1);
+    ByteBuffer input = ByteBuffer.allocateDirect(1 + size);
+    input.put(0, (byte) size);
+
+    Supplier<ByteString> embeddedBytes =
+        () -> {
+          try {
+            final CodedInputStream inputStream = CodedInputStream.newInstance(input, true);
+            inputStream.enableAliasing(true);
+            return inputStream.readBytes();
+          } catch (IOException e) {
+            throw new RuntimeException(e);
+          }
+        };
+
+    assertThat(embeddedBytes.get().size()).isEqualTo(size);
+
+    // Concurrent reader should have no impact.
+    int iterations = 100000;
+    new Thread(
+            () -> {
+              for (int i = 0; i < iterations; i++) {
+                ByteString unused = embeddedBytes.get();
+              }
+            })
+        .start();
+
+    for (int i = 0; i < iterations; i++) {
+      assertThat(embeddedBytes.get().size()).isEqualTo(size);
+    }
+  }
+
   @Test
   public void testIterableByteBufferInputStreamReadBytesWithAlias() throws Exception {
     ByteArrayOutputStream byteArrayStream = new ByteArrayOutputStream();