Repository navigation
Calling retain on a closed ArrowBuf revives it, into an undefined state #906
Description
Describe the bug, including details regarding any error messages, version, and platform.
@Test
fun testArrowBufRetainBug() {
RootAllocator().use { al ->
val buf = al.buffer(10)
buf.close()
// correctly throws
assertThrows<IllegalArgumentException> { buf.referenceManager.retain() }
assertEquals(0, buf.refCnt()) // nope, it's 1
// doesn't throw, ref-count is 1 - so the caller assumes they've successfully taken a reference
// but the underlying memory has already been reclaimed and re-used
assertThrows<IllegalArgumentException> { buf.referenceManager.retain() }
}
}Caused by the getAndAdd in BufferLedger.retain(int) - this is what leaves the ref-count positive, so on the next call, this doesn't fail.
Some kind of compareAndSet instead, perhaps?
Cheers,
James
Activity
lidavidm commentedon Nov 6, 2025
Hmm, I'd argue that after the first exception the buffer is already in an undefined state. compareAndSet would require a CAS loop - might be a pessimization for the common case? We could reset back to 0 before throwing instead?
jarohen commentedon Nov 6, 2025
That'd be a completely reasonable resolution 🙂
Yep, understood. You folks got benchmarks we could check this against? Suspect ours are a bit high-level, might get lost in noise.
No urgency on our part, we've put more guards in 🙂
lidavidm commentedon Nov 7, 2025
I'm not too sure if we have benchmarks - coverage is spotty for this repo. There's also the question of the right benchmark (frankly, I would assume an application should not be spamming reference count updates, in which case a CAS loop would presumably not cost anything over an atomic add?).
I would love to use the new FFM APIs instead...
lidavidm commentedon Nov 7, 2025
@jbonofre not sure if you have opinions or experience from your arrow-java application :)
jarohen commentedon Nov 7, 2025
That's what we're starting to do here 🙂
lidavidm commentedon Nov 10, 2025
Would resetting the count before throwing be a reasonable solution for your case? That way we only touch the error path
lidavidm commentedon Nov 10, 2025
I would like to hear more if there's anything that can be shared :)
EDIT: there's an existing request: #511
IOOBE/segfault - vendor in proposed fix for apache/arrow-java#906, re…
IOOBE/segfault - vendor in proposed fix for apache/arrow-java#906, re…
jarohen commentedon Nov 18, 2025
hey @lidavidm - turns out we had to put this fix in in the end anyway (see linked commits) because it led to problems on our longer running environments.
fwiw the CAS loop didn't seem to add anything to our benchmarks AFAICT 🙂
IOOBE/segfault - vendor in proposed fix for apache/arrow-java#906, re…
IOOBE/segfault - vendor in proposed fix for apache/arrow-java#906, re…
Revert "IOOBE/segfault - vendor in proposed fix for apache/arrow-java…
apacheGH-906: Do not revive freed memory on retain or transfer to ano…
apacheGH-906: Add JMH benchmarks for BufferLedger retain and release
apacheGH-906: Do not revive freed memory in another allocator
apacheGH-906: Add JMH benchmarks for BufferLedger retain and release
apacheGH-906: Do not revive a released buffer on retain
apacheGH-906: Do not revive freed memory in another allocator
apacheGH-906: Add JMH benchmarks for BufferLedger retain and release
apacheGH-906: Do not revive a released buffer on retain
apacheGH-906: Do not revive freed memory in another allocator
apacheGH-906: Add JMH benchmarks for BufferLedger retain and release