Repository navigation
GH-238: Stop TestEmptyArrowBuf writing past the end of a native allocation - #1426
Merged
Merged
Conversation
testEmptyArrowBuf wrapped the address of Netty's empty buffer in a 1024-byte ArrowBuf and wrote 10 bytes through it. That address comes from ByteBuffer.allocateDirect(0), which is an 8-byte native allocation, so the test wrote 2 bytes past its end. On 64-bit Windows an 8-byte heap block has no slack, so those bytes can land on the next block's header. The forked test JVM then exits with a non-zero code after its tests pass, which surefire reports as "Error occurred in starting fork" for arrow-memory-netty. This is the most common failure of the Windows CI job. Allocate the 1024 bytes from the test's allocator instead.
andygrove
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
October 31, 2026 23:33
lidavidm
approved these changes
Nov 1, 2026
lidavidm
left a comment
lidavidm
left a comment
Member
There was a problem hiding this comment.
Thanks for tracking this down! That said, the purpose of the original test is now a mystery to me.
Member
|
I kicked CI... |
Member
|
Ah, and @jbonofre wants to code freeze for release so I'll hold off merging for now |
jbonofre
approved these changes
Nov 1, 2026
jbonofre
left a comment
jbonofre
left a comment
Member
There was a problem hiding this comment.
Thanks @andygrove! It looks good to me.
The fix makes sense. The old test wrapped a 1024 byte ArrowBuf around Netty's empty buffer, which is backed by only an 8 byte native allocation, so the setByte calls wrote past the end of it. That would explain the intermittent STATUS_HEAP_CORRUPTION on Windows. Backing the buffer with a real 1024 byte allocation and freeing it with try-with-resources is the right fix.
Good catch!
jbonofre
added a commit
that referenced
this pull request
Nov 1, 2026
…24-byte ArrowBuf (#1427) ## What's Changed `TestNettyAllocator.testMemoryUsage` wrapped a 1024-byte `NO_OP` `ArrowBuf` around the memory address of `PooledByteBufAllocatorL`'s empty buffer, whose native allocation is only a few bytes. The test only reads `memoryAddress()` today, so nothing overruns yet, but any read or write through that buffer would run past the end of the native allocation. That's the same problem #1426 fixes in `TestEmptyArrowBuf`. The buffer isn't needed. The memory usage log the test waits for comes from the status thread that the `PooledByteBufAllocatorL` constructor starts when TRACE logging is enabled, so the test now constructs the allocator directly and drops the `ArrowBuf`. All `memory-netty` tests pass locally on JDK 21 with Error Prone enabled. Closes #238.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's Changed
TestEmptyArrowBuf.testEmptyArrowBufwrapped the address of Netty's empty buffer, an 8-byte native allocation, in a 1024-byteArrowBufand wrote 10 bytes through it. The test now backs the no-op buffer with 1024 bytes from its own allocator, so it writes only memory it owns. The assertions are unchanged.This overrun causes the intermittent "Error occurred in starting fork" failure of
arrow-memory-nettyin the Windows CI job. #238 has the analysis.Before this change,
testEmptyArrowBuffails on macOS when amallocinterposer ends 8-byte allocations at a guard page. With it, the wholememory-nettysuite passes under the same interposer and normally. One green Windows run would not prove much, because the failure hit about 7% of runs.Closes #238.