From cf11c125e6948dc2ad82da95e005cdbf1dd3d3cc Mon Sep 17 00:00:00 2001 From: Frank McSherry Date: Tue, 22 Sep 2026 14:03:44 -0400 Subject: [PATCH 1/6] Derive byte pointers and lengths from one mutable slice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Call the backing object’s DerefMut implementation once in construction and regeneration. Derive both the pointer and length from that slice so custom dereference implementations cannot make them disagree. --- bytes/src/lib.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/bytes/src/lib.rs b/bytes/src/lib.rs index 4c41b52e4..501cbe7bb 100644 --- a/bytes/src/lib.rs +++ b/bytes/src/lib.rs @@ -71,7 +71,12 @@ pub mod arc { Arc::get_mut(&mut sequestered) .unwrap() .downcast_mut::() - .map(|a| (a.as_mut_ptr(), a.len())) + .map(|a| { + // Acquire one slice, so that the two next calls must agree. + // Otherwise, adversarial implementations could lie to use. + let slice = a.deref_mut(); + (slice.as_mut_ptr(), slice.len()) + }) .unwrap(); BytesMut { @@ -131,8 +136,11 @@ pub mod arc { // Only possible if this is the only reference to the sequestered allocation. if let Some(boxed) = Arc::get_mut(&mut self.sequestered) { let downcast = boxed.downcast_mut::()?; - self.ptr = downcast.as_mut_ptr(); - self.len = downcast.len(); + // Acquire one slice, so that the two next calls must agree. + // Otherwise, adversarial implementations could lie to use. + let slice = downcast.deref_mut(); + self.ptr = slice.as_mut_ptr(); + self.len = slice.len(); Some(true) } else { From 741cab81b1aba704dde36bec4de5fb48917d7579 Mon Sep 17 00:00:00 2001 From: Frank McSherry Date: Tue, 22 Sep 2026 14:03:46 -0400 Subject: [PATCH 2/6] Clear the byte view before regenerating its backing slice A backing DerefMut implementation can invalidate the old slice and then panic. Reset the view before calling it so a caught panic leaves an empty buffer that still owns the backing object. --- bytes/src/lib.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/bytes/src/lib.rs b/bytes/src/lib.rs index 501cbe7bb..ab8ef0dfb 100644 --- a/bytes/src/lib.rs +++ b/bytes/src/lib.rs @@ -115,6 +115,11 @@ pub mod arc { /// value indicates whether this occurred. A `None` value indicates that the /// downcast to `B` failed and the type is not correct. /// + /// # Panics + /// + /// If the backing object's `deref_mut` panics, `self` is left empty. + /// The backing object remains owned by `self`. + /// /// # Examples /// /// ``` @@ -135,7 +140,14 @@ pub mod arc { pub fn try_regenerate(&mut self) -> Option where B: DerefMut+'static { // Only possible if this is the only reference to the sequestered allocation. if let Some(boxed) = Arc::get_mut(&mut self.sequestered) { + // This is standard library code, and should not panic / unwind. + // If this ever changes, we should move the (ptr, len) pair first. let downcast = boxed.downcast_mut::()?; + // The backing object's `deref_mut` may invalidate the old slice and then panic. + // Clear the view first so a caught panic cannot expose an invalid pointer. + self.ptr = std::ptr::NonNull::::dangling().as_ptr(); + self.len = 0; + // Acquire one slice, so that the two next calls must agree. // Otherwise, adversarial implementations could lie to use. let slice = downcast.deref_mut(); From f27f300389f1715a3c63bacae7d04a0db11eb0dd Mon Sep 17 00:00:00 2001 From: Frank McSherry Date: Tue, 22 Sep 2026 14:03:47 -0400 Subject: [PATCH 3/6] Preserve the Send bound on erased byte storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep the constructor’s Send requirement in the erased owner type. Clarify that BytesMut is neither Send nor Sync, while its immutable Bytes views are both. --- bytes/src/lib.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/bytes/src/lib.rs b/bytes/src/lib.rs index ab8ef0dfb..6a5f7b654 100644 --- a/bytes/src/lib.rs +++ b/bytes/src/lib.rs @@ -39,7 +39,10 @@ pub mod arc { use std::sync::Arc; use std::any::Any; - /// A thread-safe byte buffer backed by a shared allocation. + /// A mutable byte slice backed by a shared allocation. + /// + /// This type is neither `Send` nor `Sync`. + /// It can produce immutable [`Bytes`] views that are both `Send` and `Sync`. /// /// An instance of this type contends that `ptr` is valid for `len` bytes, /// and that no other reference to these bytes exists, other than through @@ -54,7 +57,7 @@ pub mod arc { /// Importantly, this is unavailable for as long as the struct exists, which may /// prevent shared access to ptr[0 .. len]. I'm not sure I understand Rust's rules /// enough to make a stronger statement about this. - sequestered: Arc, + sequestered: Arc, } impl BytesMut { @@ -66,7 +69,7 @@ pub mod arc { // stable for the lifetime of `sequestered`. The `Arc` also serves as our // source of truth for the allocation, which we use to re-connect slices // of the same allocation. - let mut sequestered = Arc::new(bytes) as Arc; + let mut sequestered = Arc::new(bytes) as Arc; let (ptr, len) = Arc::get_mut(&mut sequestered) .unwrap() @@ -203,7 +206,7 @@ pub mod arc { /// Importantly, this is unavailable for as long as the struct exists, which may /// prevent shared access to ptr[0 .. len]. I'm not sure I understand Rust's rules /// enough to make a stronger statement about this. - sequestered: Arc, + sequestered: Arc, } // Synchronization happens through `self.sequestered`, which means to ensure that even From 3c11933079909b0c55f42f155b4dea697c025d13 Mon Sep 17 00:00:00 2001 From: Frank McSherry Date: Tue, 22 Sep 2026 14:03:48 -0400 Subject: [PATCH 4/6] Protect inline byte storage with UnsafeCell Arc bookkeeping can form shared references covering bytes stored inline in the backing object. Wrap the owner in UnsafeCell so those references can coexist with writes to the disjoint mutable slice. Document the aliasing and synchronization argument. --- bytes/src/lib.rs | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/bytes/src/lib.rs b/bytes/src/lib.rs index 6a5f7b654..48829efb6 100644 --- a/bytes/src/lib.rs +++ b/bytes/src/lib.rs @@ -38,6 +38,7 @@ pub mod arc { use std::ops::{Deref, DerefMut}; use std::sync::Arc; use std::any::Any; + use std::cell::UnsafeCell; /// A mutable byte slice backed by a shared allocation. /// @@ -57,7 +58,7 @@ pub mod arc { /// Importantly, this is unavailable for as long as the struct exists, which may /// prevent shared access to ptr[0 .. len]. I'm not sure I understand Rust's rules /// enough to make a stronger statement about this. - sequestered: Arc, + sequestered: Arc>, } impl BytesMut { @@ -69,10 +70,14 @@ pub mod arc { // stable for the lifetime of `sequestered`. The `Arc` also serves as our // source of truth for the allocation, which we use to re-connect slices // of the same allocation. - let mut sequestered = Arc::new(bytes) as Arc; + // `Arc` bookkeeping can form shared references covering the backing object. + // `UnsafeCell` lets those references coexist with writes to bytes stored inline in that object. + // The mutable slice must still remain disjoint from every published `Bytes` slice. + let mut sequestered = Arc::new(UnsafeCell::new(bytes)) as Arc>; let (ptr, len) = Arc::get_mut(&mut sequestered) .unwrap() + .get_mut() .downcast_mut::() .map(|a| { // Acquire one slice, so that the two next calls must agree. @@ -145,7 +150,7 @@ pub mod arc { if let Some(boxed) = Arc::get_mut(&mut self.sequestered) { // This is standard library code, and should not panic / unwind. // If this ever changes, we should move the (ptr, len) pair first. - let downcast = boxed.downcast_mut::()?; + let downcast = boxed.get_mut().downcast_mut::()?; // The backing object's `deref_mut` may invalidate the old slice and then panic. // Clear the view first so a caught panic cannot expose an invalid pointer. self.ptr = std::ptr::NonNull::::dangling().as_ptr(); @@ -206,22 +211,22 @@ pub mod arc { /// Importantly, this is unavailable for as long as the struct exists, which may /// prevent shared access to ptr[0 .. len]. I'm not sure I understand Rust's rules /// enough to make a stronger statement about this. - sequestered: Arc, + sequestered: Arc>, } // Synchronization happens through `self.sequestered`, which means to ensure that even // across multiple threads the referenced range of bytes remains valid. unsafe impl Send for Bytes { } - // `Sync` holds because everything reachable through `&Bytes` is read-only or atomic: - // `Deref` yields `&[u8]` (and `u8: Sync`), the mutating methods take `&mut self`, and - // cloning only touches the atomic `Arc` refcount. There is no interior mutability and - // no path to a `&mut` from a shared reference. + // `Sync` holds because accesses through `&Bytes` are read-only or atomic. + // `Deref` yields `&[u8]` (and `u8: Sync`), the mutating methods take `&mut self`, and cloning only changes the atomic `Arc` refcount. + // The backing object is inside an `UnsafeCell`, but `&Bytes` exposes only immutable byte slices and no mutable references. + // A `BytesMut` sharing the allocation can only write to a disjoint byte range. + // Regeneration accesses the backing object only after `Arc::get_mut` establishes uniqueness. // - // Note this requires only that the sequestered payload `B` be `Send` (enforced by - // `BytesMut::from`), not `Sync`: `B` is never exposed by reference, so it is never - // shared across threads. The only cross-thread use of `B` is its destructor, which may - // run on whichever thread drops the last `Arc` clone -- and that needs `Send`, not `Sync`. + // Note this requires only that the sequestered payload `B` be `Send` (enforced by `BytesMut::from`), not `Sync`. + // `B` is never exposed by reference, so it is never shared across threads. + // The only cross-thread use of `B` is its destructor, which may run on whichever thread drops the last `Arc` clone -- and that needs `Send`, not `Sync`. unsafe impl Sync for Bytes { } impl Bytes { From 42d5e3555d1949cca05372d8c7914422f1d4fec6 Mon Sep 17 00:00:00 2001 From: Frank McSherry Date: Tue, 22 Sep 2026 14:15:48 -0400 Subject: [PATCH 5/6] Clear the byte view before downcasting its owner Discard the old view before exposing the contents of UnsafeCell during regeneration. A failed downcast now leaves an empty view while retaining the owner for a retry with the correct type. Document this behavior and the ownership invariant, and address the pointer and wording nits. --- bytes/src/lib.rs | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/bytes/src/lib.rs b/bytes/src/lib.rs index 48829efb6..7d8d65f4d 100644 --- a/bytes/src/lib.rs +++ b/bytes/src/lib.rs @@ -55,9 +55,9 @@ pub mod arc { len: usize, /// Shared access to underlying resources. /// - /// Importantly, this is unavailable for as long as the struct exists, which may - /// prevent shared access to ptr[0 .. len]. I'm not sure I understand Rust's rules - /// enough to make a stronger statement about this. + /// The backing object is accessed only during construction or unique regeneration. + /// Regeneration clears the old view before accessing the cell's contents. + /// See the safety argument above `Bytes`'s `Sync` implementation. sequestered: Arc>, } @@ -81,7 +81,7 @@ pub mod arc { .downcast_mut::() .map(|a| { // Acquire one slice, so that the two next calls must agree. - // Otherwise, adversarial implementations could lie to use. + // Otherwise, adversarial implementations could lie to us. let slice = a.deref_mut(); (slice.as_mut_ptr(), slice.len()) }) @@ -118,10 +118,11 @@ pub mod arc { /// Regenerates the BytesMut if it is uniquely held. /// - /// If uniquely held, this method recovers the initial pointer and length - /// of the sequestered allocation and re-initializes the BytesMut. The return - /// value indicates whether this occurred. A `None` value indicates that the - /// downcast to `B` failed and the type is not correct. + /// If uniquely held, this method obtains a fresh mutable slice from the backing object and re-initializes the BytesMut. + /// `Some(true)` indicates success. + /// `Some(false)` indicates that the allocation is shared and leaves the view unchanged. + /// `None` indicates that the downcast to `B` failed and leaves the view empty. + /// The backing object remains owned by `self`, so regeneration with the correct type can be retried. /// /// # Panics /// @@ -148,16 +149,16 @@ pub mod arc { pub fn try_regenerate(&mut self) -> Option where B: DerefMut+'static { // Only possible if this is the only reference to the sequestered allocation. if let Some(boxed) = Arc::get_mut(&mut self.sequestered) { - // This is standard library code, and should not panic / unwind. - // If this ever changes, we should move the (ptr, len) pair first. - let downcast = boxed.get_mut().downcast_mut::()?; - // The backing object's `deref_mut` may invalidate the old slice and then panic. - // Clear the view first so a caught panic cannot expose an invalid pointer. - self.ptr = std::ptr::NonNull::::dangling().as_ptr(); + // Clear the view before accessing the cell's contents. + // References formed during downcasting can invalidate the old pointer for inline storage. + // The backing object's `deref_mut` may also invalidate the old slice and then panic. + // Neither a failed downcast nor a caught panic may leave the old pointer accessible. + self.ptr = std::ptr::dangling_mut::(); self.len = 0; + let downcast = boxed.get_mut().downcast_mut::()?; // Acquire one slice, so that the two next calls must agree. - // Otherwise, adversarial implementations could lie to use. + // Otherwise, adversarial implementations could lie to us. let slice = downcast.deref_mut(); self.ptr = slice.as_mut_ptr(); self.len = slice.len(); @@ -208,9 +209,9 @@ pub mod arc { len: usize, /// Shared access to underlying resources. /// - /// Importantly, this is unavailable for as long as the struct exists, which may - /// prevent shared access to ptr[0 .. len]. I'm not sure I understand Rust's rules - /// enough to make a stronger statement about this. + /// The backing object is accessed only during construction or unique regeneration. + /// Regeneration clears the old view before accessing the cell's contents. + /// See the safety argument above `Bytes`'s `Sync` implementation. sequestered: Arc>, } @@ -222,7 +223,8 @@ pub mod arc { // `Deref` yields `&[u8]` (and `u8: Sync`), the mutating methods take `&mut self`, and cloning only changes the atomic `Arc` refcount. // The backing object is inside an `UnsafeCell`, but `&Bytes` exposes only immutable byte slices and no mutable references. // A `BytesMut` sharing the allocation can only write to a disjoint byte range. - // Regeneration accesses the backing object only after `Arc::get_mut` establishes uniqueness. + // Regeneration accesses the backing object only after `Arc::get_mut` establishes uniqueness and the old view is cleared. + // Only a fresh slice from `DerefMut` can reinstall the view; a failed downcast or panic leaves it empty. // // Note this requires only that the sequestered payload `B` be `Send` (enforced by `BytesMut::from`), not `Sync`. // `B` is never exposed by reference, so it is never shared across threads. From 3baebc48d26254bd3067d1a97c3bce925e9070c8 Mon Sep 17 00:00:00 2001 From: Frank McSherry Date: Tue, 22 Sep 2026 15:10:38 -0400 Subject: [PATCH 6/6] Label extract_to invariants as implementation documentation --- bytes/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bytes/src/lib.rs b/bytes/src/lib.rs index 7d8d65f4d..3e3bf8ad4 100644 --- a/bytes/src/lib.rs +++ b/bytes/src/lib.rs @@ -96,7 +96,7 @@ pub mod arc { /// Extracts [0, index) into a new `Bytes` which is returned, updating `self`. /// - /// # Safety + /// # Implementation /// /// This method first tests `index` against `self.len`, which should ensure that both /// the returned `Bytes` contains valid memory, and that `self` can no longer access it. @@ -235,7 +235,7 @@ pub mod arc { /// Extracts [0, index) into a new `Bytes` which is returned, updating `self`. /// - /// # Safety + /// # Implementation /// /// This method first tests `index` against `self.len`, which should ensure that both /// the returned `Bytes` contains valid memory, and that `self` can no longer access it.