Replacement over stale PR: Fix bytes mut asymptotics

[NobodyXu]

Replacement for #525 as it has been stale for really long.

Fixes #524
Closes #525

[NobodyXu]

@Darksonn Request for review please.

I have rebased #525 against master, this fixed the sanitizer and stable/nightly CI.

I also fixed the minrust CI by removing use of offset_from and fixed the "must-use" error in {bytes, bytes_mut}::release_shared by manually dropping Box::from_raw.

[Darksonn]

This looks reasonable. The documentation is well written.

I have one nit:

[NobodyXu]

The documentation is well written.

They are actually written by @steffahn , I just rebased them against master and fixed the CI.

[Darksonn]

Thanks @NobodyXu and @steffahn!

[NobodyXu]

@Darksonn Can we make a new release? It has been a long time since the last release.

[Darksonn]

Yeah, it looks like it's time for a release.

[steffahn]

Ah, nice that you got it working. Did the issues with #525 just disappear? (Test failure, and some UB in miri.) Or did you do something to solve them? Maybe if their origin was in unrelated code, they got fixed in the meantime?

Last time I took a look around for where the problems could come from, I got hung up on hard to understand code, and details such as code seemingly relying on (maybe(?) questionable) things such as that the uninitialized portion of a Vec gets preserved during a resize¹. I don't even know if that part of the code was actually related to the issues; and Iʼd have to look back into it again to find where I thought I saw these (potential) problems, and if I do, I'll probably open a new issue.

Footnotes

1. Which then made me wonder whether or not perhaps the standard library's resizing code for empty or almost empty Vecs is just really inefficient since it would copy all the uninitialized contents of the Vec. (And this the rabbit hole got so deep that, I suppose, it kinda kept me from getting around to spending the time to pick up #525 again.) ↩

[NobodyXu]

Ah, nice that you got it working. Did the issues with #525 just disappear? (Test failure, and some UB in miri.) Or did you do something to solve them? Maybe if their origin was in unrelated code, they got fixed in the meantime?

Rebasing against master fixed almost all of them except for minrust (due to use of offset_from) and miri (due to must use return value of Box::from_raw).

I fixed them and it just works.

[Darksonn]

code seemingly relying on (maybe(?) questionable) things such as that the uninitialized portion of a Vec gets preserved during a resize

The standard library Vec certainly doesn't guarantee that the uninitialized portion is preserved.

[steffahn]

I fixed them and it just works.

Oh, well, that's nice I guess 😅. I wasn't really thinking there were any problems in my change of the code anyways (as also indicated by the fact that, at the time, problems only started to appear after some changes on master).

[steffahn]

The standard library Vec certainly doesn't guarantee that the uninitialized portion is preserved.

Alright. I'll try to find where in bytes I thought I saw code that seemed to rely on this, and check if it still exists and if yes, open an issue 😇

[Darksonn]

If you look at the release PR I just posted, you'll see quite a few PRs related to fixing miri. It was probably those that fixed most of the CI issues on your PR.

Alright. I'll try to find where in bytes I thought I saw code that seemed to rely on this, and check if it still exists and if yes, open an issue

Thanks!

[steffahn]

Alright. I'll try to find where in bytes I thought I saw code that seemed to rely on this, and check if it still exists and if yes, open an issue 😇

Haven’t looked back into the code yet, instead – as a first step – I tried a more general approach for determining whether or not there’s a problem at all, by utilizing miri, with a modified standard library that avoids copying any not-considered-initialized portion of the Vec when it grows

(click to see the diff)

diff --git a/library/alloc/src/raw_vec.rs b/library/alloc/src/raw_vec.rs
index b0f4529abdf..b32afee9e43 100644
--- a/library/alloc/src/raw_vec.rs
+++ b/library/alloc/src/raw_vec.rs
@@ -397,7 +397,12 @@ fn grow_amortized(&mut self, len: usize, additional: usize) -> Result<(), TryRes
         let new_layout = Layout::array::<T>(cap);
 
         // `finish_grow` is non-generic over `T`.
-        let ptr = finish_grow(new_layout, self.current_memory(), &mut self.alloc)?;
+        let ptr = finish_grow(
+            len * mem::size_of::<T>(),
+            new_layout,
+            self.current_memory(),
+            &mut self.alloc,
+        )?;
         self.set_ptr_and_cap(ptr, cap);
         Ok(())
     }
@@ -416,7 +421,12 @@ fn grow_exact(&mut self, len: usize, additional: usize) -> Result<(), TryReserve
         let new_layout = Layout::array::<T>(cap);
 
         // `finish_grow` is non-generic over `T`.
-        let ptr = finish_grow(new_layout, self.current_memory(), &mut self.alloc)?;
+        let ptr = finish_grow(
+            len * mem::size_of::<T>(),
+            new_layout,
+            self.current_memory(),
+            &mut self.alloc,
+        )?;
         self.set_ptr_and_cap(ptr, cap);
         Ok(())
     }
@@ -446,6 +456,7 @@ fn shrink(&mut self, cap: usize) -> Result<(), TryReserveError> {
 // much smaller than the number of `T` types.)
 #[inline(never)]
 fn finish_grow<A>(
+    len_u8: usize,
     new_layout: Result<Layout, LayoutError>,
     current_memory: Option<(NonNull<u8>, Layout)>,
     alloc: &mut A,
@@ -463,7 +474,16 @@ fn finish_grow<A>(
         unsafe {
             // The allocator checks for alignment equality
             intrinsics::assume(old_layout.align() == new_layout.align());
-            alloc.grow(ptr, old_layout, new_layout)
+            let new = alloc.allocate(new_layout);
+            if let Ok(new_ptr) = new {
+                ptr::copy_nonoverlapping(
+                    ptr.as_ptr() as *const MaybeUninit<u8>,
+                    new_ptr.as_mut_ptr() as *mut MaybeUninit<u8>,
+                    len_u8,
+                );
+                alloc.deallocate(ptr, old_layout);
+            }
+            new
         }
     } else {
         alloc.allocate(new_layout)

(applied to 866d5c621c9b87f82a5aa53c3dfc1e477181f33b)

and provided I didn’t mess up that “growing” code, there is still a problem

(using miri 0.1.0 (b938529 2022-07-25) that comes with rustc 1.64.0-nightly (4493a0f47 2022-08-02))

rm -rf ~/.cache/miri; MIRI_LIB_SRC=PATH/TO/CHECKOUT/OF/rust/library cargo +nightly miri test

test reserve_shared_reuse ... error: Undefined Behavior: reading memory at alloc456687[0xd..0x16], but memory is uninitialized at [0xd..0x16], and this operation requires initialized memory
   --> /home/frank/forks/rust/library/core/src/array/equality.rs:152:13
    |
152 |             crate::intrinsics::raw_eq(a, b)
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reading memory at alloc456687[0xd..0x16], but memory is uninitialized at [0xd..0x16], and this operation requires initialized memory
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: backtrace:
    = note: inside `<u8 as std::array::equality::SpecArrayEq<u8, 9>>::spec_eq` at /home/frank/forks/rust/library/core/src/array/equality.rs:152:13
    = note: inside `std::array::equality::<impl std::cmp::PartialEq for [u8; 9]>::eq` at /home/frank/forks/rust/library/core/src/array/equality.rs:12:9
    = note: inside `std::array::equality::<impl std::cmp::PartialEq<[u8; 9]> for [u8]>::eq` at /home/frank/forks/rust/library/core/src/array/equality.rs:52:22
    = note: inside `std::cmp::impls::<impl std::cmp::PartialEq<&[u8; 9]> for &[u8]>::eq` at /home/frank/forks/rust/library/core/src/cmp.rs:1513:13
note: inside `reserve_shared_reuse` at /home/frank/forks/rust/library/core/src/macros/mod.rs:40:21
   --> tests/test_bytes.rs:573:5
    |
573 |     assert_eq!(&*bytes, b"!123ex123");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at tests/test_bytes.rs:562:1
   --> tests/test_bytes.rs:562:1
    |
561 |   #[test]
    |   ------- in this procedural macro expansion
562 | / fn reserve_shared_reuse() {
563 | |     let mut bytes = BytesMut::with_capacity(1000);
564 | |     bytes.put_slice(b"Hello, World!");
565 | |     drop(bytes.split());
...   |
574 | |     assert_eq!(bytes.capacity(), 2009);
575 | | }
    | |_^
    = note: this error originates in the macro `assert_eq` which comes from the expansion of the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace