Switch `BufMut::bytes_mut` from `MaybeUninit` to `&mut UninitSlice`, a type ownec by `bytes` for this purpose #433

carllerche · 2020-10-16T23:31:22Z

The way BufMut uses MaybeUninit can lead to unsoundness.

This replaces MaybeUnit with a type owned by bytes so we can ensure the usage patterns are sound.

Refs: #328

carllerche · 2020-10-17T03:49:02Z

Kixunil

Looks good.

I think it'd be helpful to have at least write_slice(&mut self, bytes: &[u8]) and &mut write_byte(byte: u8, index: usize) methods for the "almost safe" code to use.

Also I suggest pinging Ralf once the PR is redy to merge.. He knows unsafe better than me. :)

("almost safe" - the only unsafety is calling advance with proper values)

Kixunil · 2020-10-17T06:26:25Z

src/buf/uninit_slice.rs

+    /// let mut slice = &mut data[..];
+    /// let ptr = BufMut::bytes_mut(&mut slice).as_ptr();
+    /// ```
+    pub fn as_ptr(&self) -> *const u8 {


This is almost useless It can't be read from until the caller wrote to *mut u8 version of (why would the caller use *const if there's *mut already available?) it and it can't be written to.
Address of the pointer can be compared, hashed, debugged...

I think this should be documented somehow.

Kixunil · 2020-10-17T06:29:07Z

src/buf/uninit_slice.rs

+    }
+
+    /// Return a raw pointer to the slice's buffer.
+    ///


I'd suggest documenting safety:

The consumer must not read from the memory behind the pointer

The consumer must not write uninit bytes into memory behind the pointer

(also aliasing rules, but that should be obvious)

Technically, writing uninit bytes into the memory should be fine if the caller does not advance accordingly.

Not if UninitSlice was constructed from &mut [u8] - same issue why there was a bug.

Ah right! Way subtle

carllerche · 2020-10-17T17:09:56Z

cc @RalfJung this fix our unsoundness? :)

carllerche · 2020-10-17T20:05:05Z

@Kixunil I applied feedback.

Kixunil

I think it'd be useful to also test that slices actually get overwritten when write_slice and write_byte re used as them failing to do so is UB. (Of course, those tests must be performed on UninitSlice constructed from &mut [u8].)

Kixunil · 2020-10-18T05:47:35Z

src/buf/uninit_slice.rs

+    pub fn write_byte(&mut self, index: usize, byte: u8) {
+        assert!(index < self.len());
+
+        unsafe { self.as_mut_ptr().offset(index as isize).write(byte) };


This could in theory overflow, even if unlikely. I'd suggest writing self[index..].as_mut_ptr().write(byte).
That way assert above is not strictly needed but I'd keep it for documentation value. (The repeated check should be optimized-out.)

Kixunil · 2020-10-18T05:49:13Z

src/buf/uninit_slice.rs

+    /// # Safety
+    ///
+    /// The caller **must not** read from the referenced memory and **must not**
+    /// write uninitialized bytes to the slice either.


Putting emphasis on "uninitialized" might be nice?

tests/test_buf_mut.rs

+    let mut data = [b'b', b'a', b'r'];
+
+    let slice = unsafe { UninitSlice::from_raw_parts_mut(data.as_mut_ptr(), 3) };
+    slice.write_slice(b"a");


RalfJung · 2020-10-18T12:23:38Z

src/buf/uninit_slice.rs

+///
+/// Returned by `BufMut::bytes_mut()`, the referenced byte slice may be
+/// uninitialized. The wrapper provides safe access without introducing
+/// undefined behavior.


It would be good to say what the concrete invariant/protocol for this type is, and in particular how it differs from MaybeUninit.

if I understand correctly, the key difference is that the contents may be uninitialized, but the API ensures that once they are initialized, they will never be de-initialized again. Is that correct?

Yes, that's correct. Especially it allows constructing UninitSlice from already-initialized slice (&mut [u8]).

RalfJung · 2020-10-18T12:26:24Z

cc @RalfJung this fix our unsoundness? :)

I have not reviewed your entire BufMut API surface, it is a bit too big for that.^^ But this seems to plug the hole that is caused by MaybeUninit not protecting against "de-initialization".

src/buf/uninit_slice.rs

Darksonn · 2020-10-18T13:17:50Z

src/buf/uninit_slice.rs

+    ///
+    /// assert_eq!(b"bar", &data[..]);
+    /// ```
+    pub fn write_slice(&mut self, src: &[u8]) {


Should this allow src to be shorter than self? Alternatively, should it be named copy_from_slice to mirror the method from std?

Good question, I suspect allowing writes of shorter slices would be more practical for consumers but if we decide to have same-length-slice then copy_from_slice is a better name.

I will rename this to copy_from_slice.

src/buf/uninit_slice.rs

Darksonn · 2020-10-18T14:00:59Z

I compiled it with rustdoc and looked at the documentation. I didn't catch any soundness issues by looking at it.

Darksonn · 2020-10-18T14:56:58Z

src/buf/uninit_slice.rs

+    ///
+    /// let slice = unsafe { UninitSlice::from_raw_parts_mut(ptr, len) };
+    /// ```
+    pub unsafe fn from_raw_parts_mut<'a>(ptr: *mut u8, len: usize) -> &'a mut UninitSlice {


Should we have a constructor that takes an &mut [u8]?

Maybe (also) impl<'a> From<&'a mut [u8]> for &'a mut UninitSlice

Both sounds good to me. I think we can do that in a follow up PR.

Co-authored-by: Alice Ryhl <alice@ryhl.io>

carllerche added 2 commits October 16, 2020 16:24

sketch out new BufMut::bytes_mut

9bad867

tweaks

b8fe6f5

fmt

252a31a

carllerche requested a review from seanmonstar October 17, 2020 03:50

docs

d0ab1de

carllerche marked this pull request as ready for review October 17, 2020 05:09

carllerche added 2 commits October 16, 2020 22:13

fmt::Debug

8fea13c

fmt

00b6d89

carllerche requested review from taiki-e, Darksonn and hawkw October 17, 2020 05:16

Kixunil reviewed Oct 17, 2020

View reviewed changes

add additional helpers

8853151

fix core

e7ae35c

Kixunil reviewed Oct 18, 2020

View reviewed changes

RalfJung reviewed Oct 18, 2020

View reviewed changes

Darksonn reviewed Oct 18, 2020

View reviewed changes

carllerche and others added 3 commits October 19, 2020 15:22

Update src/buf/uninit_slice.rs

a8fdfd1

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Update src/buf/uninit_slice.rs

200ed46

Co-authored-by: Alice Ryhl <alice@ryhl.io>

Update src/buf/uninit_slice.rs

38e2fe1

Co-authored-by: Alice Ryhl <alice@ryhl.io>

seanmonstar approved these changes Oct 19, 2020

View reviewed changes

apply feedback

1a87a33

carllerche merged commit e0d8413 into master Oct 19, 2020

carllerche deleted the uninit-slice branch October 19, 2020 22:48

carllerche mentioned this pull request Oct 20, 2020

Implementation of BufMut for &mut [u8] is unsound #328

Closed

dekellum mentioned this pull request Dec 18, 2020

Consider renaming Buf::bytes() and BufMut::bytes_mut() #447

Closed

dependabot bot mentioned this pull request Mar 15, 2021

Bump bytes from 0.5.4 to 1.0.1 Lionjudge9061-corp/mobilecoin#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch `BufMut::bytes_mut` from `MaybeUninit` to `&mut UninitSlice`, a type ownec by `bytes` for this purpose #433

Switch `BufMut::bytes_mut` from `MaybeUninit` to `&mut UninitSlice`, a type ownec by `bytes` for this purpose #433

carllerche commented Oct 16, 2020 •

edited

carllerche commented Oct 17, 2020

Kixunil left a comment

Kixunil Oct 17, 2020

Kixunil Oct 17, 2020

carllerche Oct 17, 2020

Kixunil Oct 17, 2020

carllerche Oct 17, 2020

carllerche commented Oct 17, 2020

carllerche commented Oct 17, 2020

Kixunil left a comment

Kixunil Oct 18, 2020

Kixunil Oct 18, 2020

This comment was marked as resolved.

RalfJung Oct 18, 2020

Kixunil Oct 18, 2020

RalfJung commented Oct 18, 2020 •

edited

Darksonn Oct 18, 2020

Kixunil Oct 18, 2020

carllerche Oct 19, 2020

Darksonn commented Oct 18, 2020

Darksonn Oct 18, 2020

Kixunil Oct 18, 2020

carllerche Oct 19, 2020

Switch BufMut::bytes_mut from MaybeUninit to &mut UninitSlice, a type ownec by bytes for this purpose #433

Switch BufMut::bytes_mut from MaybeUninit to &mut UninitSlice, a type ownec by bytes for this purpose #433

Conversation

carllerche commented Oct 16, 2020 • edited

carllerche commented Oct 17, 2020

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllerche commented Oct 17, 2020

carllerche commented Oct 17, 2020

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Oct 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn commented Oct 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Switch `BufMut::bytes_mut` from `MaybeUninit` to `&mut UninitSlice`, a type ownec by `bytes` for this purpose #433

Switch `BufMut::bytes_mut` from `MaybeUninit` to `&mut UninitSlice`, a type ownec by `bytes` for this purpose #433

carllerche commented Oct 16, 2020 •

edited

RalfJung commented Oct 18, 2020 •

edited