Skip to content

Commit

Permalink
Duplicate buffer for thread-safe aliasing. (#19405)
Browse files Browse the repository at this point in the history
Fixes #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.

Closes #19405

COPYBARA_INTEGRATE_REVIEW=#19405 from mernst-github:mernst/aliasing 1d97209
PiperOrigin-RevId: 703588369
  • Loading branch information
mernst-github authored and copybara-github committed Dec 6, 2024
1 parent 5030b0b commit fb180a6
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit fb180a6

Please sign in to comment.