Repository navigation
Conversation
BufferLedger.retain added to the reference count with getAndAdd and only then checked that the old count was positive. A retain on a released buffer (count 0) threw but left the count at the increment, so the next retain succeeded on memory that had already been freed and possibly reused. Increment with a compareAndSet loop that refuses to move the count off 0, keeping the existing IllegalArgumentException. Resetting the count before throwing would only narrow the window: between the failed add and the reset another thread's retain sees a positive count and succeeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BufferLedger.retain(ArrowBuf, BufferAllocator) and transferOwnership associated the memory with the target allocator without checking that it was still allocated. On a closed buffer whose memory had been freed, they created a ledger with a count of 1 over the freed memory and returned a usable buffer; closing it then failed in AllocationManager.release. AllocationManager.associate now refuses to add a reference once the memory has been freed (no owning ledger), checked under the lock the final release holds while freeing it. A buffer whose own ledger is released can still be retained while another allocator's ledger keeps the memory alive, as TestBaseAllocator.testAllocator_shareOwnership does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Measure what the fix costs: the compareAndSet loop in retain, which retries when threads contend on one buffer, and the freed-memory check in AllocationManager.associate, against buffer allocation and close for scale. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| // Once the last ledger is released the memory is freed (owningLedger is null), and a new | ||
| // association would revive it (https://github.com/apache/arrow-java/issues/906). The final | ||
| // release frees the memory under this lock, so it can't happen between this check and the | ||
| // association. The constructor's own association runs before owningLedger is set. | ||
| Preconditions.checkState( | ||
| !retain || owningLedger != null, "The ArrowBuf's memory has already been released"); |
There was a problem hiding this comment.
This check treats owningLedge == null as "memory freed", but release() only clears it at the end (line 177), after releaseBytes(), release0() and listener.onRelease(). If any of those throws (a user AllocationListener.onRelease, for example), the memory is already freed while owningLedger still points at the old ledger. This check then passes, and retain(buf, other) or transferOwnership(buf, other) returns a live buffer over freed memory, which is the #906 scenario again. transferOwnership would also call releaseBytes a second time on the old allocator.
Could we clear it before freeing? In release():
owningLedger = null;
oldAllocator.releaseBytes(getSize());
release0();
oldAllocator.getListener().onRelease(getSize());None of those three calls read owningLedger. If release0() throws, we leak memory instead of allowing a use-after-free, which is the safer failure.
A test with a listener whose onRelease throws would cover this.
| // still closed: no access to memory that may already belong to another buffer | ||
| assertThrows(IllegalStateException.class, () -> buf.getByte(0)); | ||
| // and a stray release cannot free the chunk a second time | ||
| assertThrows(IllegalStateException.class, () -> buf.getReferenceManager().release()); |
There was a problem hiding this comment.
This stray release() throws, but decrement() has already applied addAndGet(-1), so buf.refCnt() is now -1. ArrowBuf.ensureAccessible() only rejects refCnt() == 0, so after this line getByte() and setByte() pass the check and touch freed memory. It's the same revival this PR fixes for retain, through release this time.
Adding this after the line should fail today:
assertEquals(0, buf.refCnt());
assertThrows(IllegalStateException.class, () -> buf.getByte(0));The fix mirrors your retain change: in decrement(), check that bufRefCnt >= decrement before substracting. It already runs under the allocationManager lock, so it doesn't need a CAS loop.
What's Changed
BufferLedger.retainnow uses acompareAndSetloop that never moves the count off 0, so aretainon a released buffer keeps throwing instead of reviving freed memory. The second commit closes the same hole inretain(ArrowBuf, BufferAllocator)andtransferOwnership, which returned a usable buffer over freed memory.releaseis unchanged: once nothing can lift a count off 0, it can't free the memory twice.lidavidm suggested on #906 resetting the count before throwing. That only narrows the window, because another thread's
retaincan see the positive count before the reset (the Tests table). The Benchmark section measures the loop's cost.Fix from XTDB
The
retainfix is @jarohen's: XTDB has shipped it in its patchedBufferLedgersince xtdb/xtdb@5576310, after the bug crashed its JVM with aSIGSEGV(xtdb/xtdb#5013). This PR brings it upstream with tests and a benchmark, and differs from XTDB'sretainonly wheremainhas moved on since XTDB's copy: it uses theAtomicIntegerFieldUpdaterfrom GH-1038, and keeps the historical-log call before the loop, so a failedretainstill shows whenBaseAllocator.DEBUGis on. The cross-allocator fix is new.Tests
The first commit's tests, repeated locally with
@RepeatedTest:maintestRetainAfterCloseThrowsEveryTimetestFailedRetainDoesNotReviveReleasedMemorytestRetainRacingFinalReleaseNeverRevivesBufferReset-before-throw's 48 failures were 26
retains succeeding after the buffer was freed, and 22 secondreleases to 0 failing inAllocationManager.releasewithExpecting a mapping for allocator and reference manager. The race test takes about 0.1 to 0.3 s. The second commit's two closed-buffer tests fail without its check.Benchmark
BufferLedgerBenchmarks, average ns per operation: JMH 1.37 with-wi 5 -w 1s -i 5 -r 1sand 10 forks (5 for 4 and 8 threads), on an Apple M2 Pro with Zulu JDK 21; ± is JMH's 99.9% confidence interval.mainretainReleaseUncontendedretainReleaseContended2retainReleaseContended4retainReleaseContended8retainToAllocatorCloseUncontended, the loop adds about 1.6 ns per
retain+release, against about 115 ns forallocateClose. At 2 threads aretainthat loses the race retries, so the pair is slower and noisier; many threads updating one buffer's count at once isn't a pattern Arrow code is expected to have. From 4 threads up,release's lock dominates. The cross-allocator check costs nothing measurable.AI assistance (Claude Code) was used during development; the complete diff and test results were reviewed by the author.
Closes #906.
🤖 Generated with Claude Code