This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new 9a8400ea86 fix(arrow-buffer): prevent use-after-free in
Buffer::shrink_to_fit when MemoryReservation::resize panics (#10932)
9a8400ea86 is described below
commit 9a8400ea86447d2b83a348285b5554170c7a830c
Author: RIchard Baah <[email protected]>
AuthorDate: Thu Sep 3 07:27:10 2026 -0400
fix(arrow-buffer): prevent use-after-free in Buffer::shrink_to_fit when
MemoryReservation::resize panics (#10932)
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax.
-->
- Closes #10379.
# Rationale for this change
Buffer maintains two pointers that must always agree: `Buffer::ptr` (the
raw *const u8 into the allocation) and `Bytes::ptr` (the NonNull<u8>
that owns the allocation). Before this fix, `shrink_to_fit` only updated
`Buffer::ptr` if `try_realloc` returned Ok. But `try_realloc` can panic;
specifically inside `MemoryReservation::resize`, a user-supplied
callback after the realloc syscall already succeeded and `Bytes::ptr`
was updated. That left the two pointers out of sync: `Bytes::ptr`
pointed to the new (valid) allocation, `Buffer::ptr` still pointed to
the old (freed) allocation. Any caller that used `catch_unwind` to
recover from the panic could then call `buf.as_slice()` and read freed
memory.
<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
replaced the conditional if try_realloc(...).is_ok() { update ptr } with
a PtrSync drop guard that unconditionally syncs `Buffer::ptr` to
`Bytes::ptr` + offset when it drops. Since Rust runs destructors during
panic unwinding, the guard fires before the panic propagates.
<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
# Are these changes tested?
yes. a similar test to what was used in #10379
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
3. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
If this PR claims a performance improvement, please include evidence
such as benchmark results.
-->
# Are there any user-facing changes?
no
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
---
arrow-buffer/src/buffer/immutable.rs | 75 ++++++++++++++++++++++++++++++------
arrow-buffer/src/bytes.rs | 11 +++++-
2 files changed, 74 insertions(+), 12 deletions(-)
diff --git a/arrow-buffer/src/buffer/immutable.rs
b/arrow-buffer/src/buffer/immutable.rs
index c5d026db8d..5d2a1f80f4 100644
--- a/arrow-buffer/src/buffer/immutable.rs
+++ b/arrow-buffer/src/buffer/immutable.rs
@@ -224,17 +224,16 @@ impl Buffer {
if desired_capacity < self.capacity()
&& let Some(bytes) = Arc::get_mut(&mut self.data)
{
- if bytes.try_realloc(desired_capacity).is_ok() {
- // Realloc complete - update our pointer into `bytes`:
- self.ptr = if is_empty {
- bytes.as_ptr()
- } else {
- // SAFETY: we kept all elements leading up to the offset
- unsafe { bytes.as_ptr().add(offset) }
- }
- } else {
- // Failure to reallocate is fine; we just failed to free up
memory.
- }
+ bytes
+ .try_realloc(desired_capacity, |base| {
+ self.ptr = if is_empty {
+ base.as_ptr()
+ } else {
+ // SAFETY: we kept all elements leading up to the
offset
+ unsafe { base.as_ptr().add(offset) }
+ };
+ })
+ .ok(); // Failure to reallocate is fine; we just failed to
free up memory.
}
}
@@ -1158,4 +1157,58 @@ mod tests {
assert_eq!(buffer_back.as_slice(), expected.as_slice());
}
}
+
+ #[test]
+ #[cfg(feature = "pool")]
+ fn test_shrink_to_fit_panicking_reservation() {
+ use std::panic::{AssertUnwindSafe, catch_unwind};
+
+ use crate::pool::{MemoryPool, MemoryReservation};
+
+ #[derive(Debug)]
+ struct PanicPool;
+
+ #[derive(Debug)]
+ struct PanicReservation {
+ panicked: bool,
+ }
+
+ impl MemoryReservation for PanicReservation {
+ fn size(&self) -> usize {
+ 0
+ }
+ fn resize(&mut self, _: usize) {
+ if !self.panicked {
+ self.panicked = true;
+ panic!("intentional panic in resize");
+ }
+ }
+ }
+
+ impl MemoryPool for PanicPool {
+ fn reserve(&self, _: usize) -> Box<dyn MemoryReservation> {
+ Box::new(PanicReservation { panicked: false })
+ }
+ fn available(&self) -> isize {
+ isize::MAX
+ }
+ fn used(&self) -> usize {
+ 0
+ }
+ fn capacity(&self) -> usize {
+ usize::MAX
+ }
+ }
+
+ let pool = PanicPool;
+ let data: Vec<u8> = (0..8).collect();
+ let mut buf = Buffer::from_slice_ref(data.as_slice());
+ buf.claim(&pool);
+
+ // shrink_to_fit panics because PanicReservation::resize panics, but
+ // Buffer::ptr must stay consistent with Bytes::ptr (no
use-after-free).
+ let _ = catch_unwind(AssertUnwindSafe(|| buf.shrink_to_fit()));
+
+ assert_eq!(buf.as_slice(), data.as_slice());
+ }
}
diff --git a/arrow-buffer/src/bytes.rs b/arrow-buffer/src/bytes.rs
index ef3ede7e43..4db48998ac 100644
--- a/arrow-buffer/src/bytes.rs
+++ b/arrow-buffer/src/bytes.rs
@@ -124,7 +124,15 @@ impl Bytes {
/// Returns `Err` if the memory was allocated with a custom allocator,
/// or the call to `realloc` failed, for whatever reason.
/// In case of `Err`, the [`Bytes`] will remain as it was (i.e. have the
old size).
- pub(crate) fn try_realloc(&mut self, new_len: usize) -> Result<(), ()> {
+ ///
+ /// `on_reallocated` is called after [`Bytes`] has updated its internal
+ /// pointer, but before resizing the memory reservation, which may call
user
+ /// code.
+ pub(crate) fn try_realloc(
+ &mut self,
+ new_len: usize,
+ on_reallocated: impl FnOnce(NonNull<u8>),
+ ) -> Result<(), ()> {
if let Deallocation::Standard(old_layout) = self.deallocation {
if old_layout.size() == new_len {
return Ok(()); // Nothing to do
@@ -152,6 +160,7 @@ impl Bytes {
self.ptr = ptr;
self.len = new_len;
self.deallocation = Deallocation::Standard(new_layout);
+ on_reallocated(ptr);
#[cfg(feature = "pool")]
{
