Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsoundness due to construction of &mut [u8] and Box<[u8]> in Clone impl #522

Closed
saethlin opened this issue Dec 31, 2021 · 1 comment · Fixed by #523
Closed

Unsoundness due to construction of &mut [u8] and Box<[u8]> in Clone impl #522

saethlin opened this issue Dec 31, 2021 · 1 comment · Fixed by #523

Comments

@saethlin
Copy link
Contributor

saethlin commented Dec 31, 2021

This example is minimized from the protobuf crate's test suite, using bytes v1.1.0:

fn main() {
    let bytes = bytes::Bytes::from(vec![1, 2, 3, 4]);
    let buf: &[u8] = &bytes; // Create a shared reference to the byte data
    bytes.clone(); // Create a unique reference to the byte data (oops?)
    drop(buf); // Use the original shared reference to the byte data
}

I think this is pretty solidly unsound under any reasonable aliasing formulation. Miri's stacked borrow checker flags the use of buf as the problematic line because its access was invalidated by constructing the &mut [u8], but I think a different sort of aliasing model could flag the creation of the &mut itself as the problem, because it aliases.

A diff which could fix this looks like this:

diff --git a/src/bytes.rs b/src/bytes.rs
index c4fbd4a..1a9a9b7 100644
--- a/src/bytes.rs
+++ b/src/bytes.rs
@@ -935,7 +935,7 @@ unsafe fn promotable_odd_drop(data: &mut AtomicPtr<()>, ptr: *const u8, len: usi
 
 unsafe fn rebuild_boxed_slice(buf: *mut u8, offset: *const u8, len: usize) -> Box<[u8]> {
     let cap = (offset as usize - buf as usize) + len;
-    Box::from_raw(slice::from_raw_parts_mut(buf, cap))
+    Box::from_raw(slice::from_raw_parts(buf, cap) as *const [u8] as *mut [u8])
 }
 
 // ===== impl SharedVtable =====

But per Stacked Borrows, this just gets over one hurdle only to crash straight into another, since Box has unique access just the same as &mut. I suspect this could be patched up as well by forsaking Box entirely within the internals of this library. But that might be kind of invasive, so I would prefer to get some feedback from maintainers first. What do you all think?

Updated to reflect the fact that Box asserts uniqueness, even in the absence of Stacked Borrows.

@saethlin saethlin changed the title Unsoundness due to construction of &mut [u8] and/or Box<[u8]> in Clone impl Unsoundness due to construction of &mut [u8] and possibly Box<[u8]> in Clone impl Dec 31, 2021
@saethlin saethlin changed the title Unsoundness due to construction of &mut [u8] and possibly Box<[u8]> in Clone impl Unsoundness due to construction of &mut [u8] and Box<[u8]> in Clone impl Dec 31, 2021
@Darksonn
Copy link
Contributor

Darksonn commented Jan 1, 2022

I agree that this is unsound.

saethlin added a commit to saethlin/bytes that referenced this issue Jan 2, 2022
Previously, this code produced a &mut[u8] and a Box<[u8]> to the shared
allocation upon cloning it. If the underlying allocation were actually
shared, such as through a &[u8] from the Deref impl, creating either of
these types incorrectly asserted uniqueness of the allocation.

This fixes the example in tokio-rs#522, but Miri still does not pass on this
test suite with -Zmiri-tag-raw-pointers because Miri does not currently
understand int to pointer casts.
saethlin added a commit to saethlin/bytes that referenced this issue Jan 2, 2022
Previously, this code produced a &mut[u8] and a Box<[u8]> to the shared
allocation upon cloning it. If the underlying allocation were actually
shared, such as through a &[u8] from the Deref impl, creating either of
these types incorrectly asserted uniqueness of the allocation.

This fixes the example in tokio-rs#522, but Miri still does not pass on this
test suite with -Zmiri-tag-raw-pointers because Miri does not currently
understand int to pointer casts.
Darksonn pushed a commit that referenced this issue Apr 6, 2022
Previously, this code produced a &mut[u8] and a Box<[u8]> to the shared
allocation upon cloning it. If the underlying allocation were actually
shared, such as through a &[u8] from the Deref impl, creating either of
these types incorrectly asserted uniqueness of the allocation.

This fixes the example in #522, but Miri still does not pass on this
test suite with -Zmiri-tag-raw-pointers because Miri does not currently
understand int to pointer casts.
lelongg pushed a commit to lelongg/bytes that referenced this issue Jan 9, 2023
Previously, this code produced a &mut[u8] and a Box<[u8]> to the shared
allocation upon cloning it. If the underlying allocation were actually
shared, such as through a &[u8] from the Deref impl, creating either of
these types incorrectly asserted uniqueness of the allocation.

This fixes the example in tokio-rs#522, but Miri still does not pass on this
test suite with -Zmiri-tag-raw-pointers because Miri does not currently
understand int to pointer casts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants