Maybe BufMut trait should be unsafe?

[Kixunil]

I was thinking that it's likely that some unsafe code will rely on properties of BufMut. More specifically:

• That remaining_mut returns correct value
• bytes_mut always returns the same slice (apart from advance())
• has_remaining_mut returns correct value
• bytes_vectored fills the correct data

Thus, I'd suggest making the trait unsafe to specify that unsafe code might rely on those properties and the implementors must ensure they hold.

[carllerche]

What does making the trait unsafe gain over making individual fns unsafe?

[Kixunil]

unsafe trait basically declares "this trait is unsafe to implement", thus other unsafe code might rely on it working properly. unsafe fn means "this function is unsafe to call"

They are different things and I believe making BufMut unsafe would make it more powerul.

[seanmonstar]

I don't know about making the trait itself unsafe. Seems heavy-handed. Are there actual situations that would require this?

[Kixunil]

I tried to search for them and I must say I didn't find anything that clearly, undeniably relies on correctness of the implementation. Or in other words, I didn't find any code in the wild that is safe and would cause UB if the trait wasn't implemented correctly. Yet. Keeping it safe seems like a footgun, but admittedly there being at least one unsafe fn is "heads up" sign.

[Ralith]

I think it's reasonable to judge that the correctness of calling an unsafe trait method is subject to the implementation of that trait complying with the trait's documented interface, but this does seem a bit unclear; maybe something to raise at the rust guidelines level?

[Kixunil]

According to https://doc.rust-lang.org/nomicon/safe-unsafe-meaning.html

The decision of whether to mark your own traits unsafe depends on the same sort of consideration. If unsafe code can't reasonably expect to defend against a broken implementation of the trait, then marking the trait unsafe is a reasonable choice.

So it seems that the question boils down to can unsafe code reasonably defend against e.g. bytes_mut returning a different buffer each time? I guess that might be pretty difficult.

One thing that occurred to me is that while a method is unsafe, implementor could forget about other methods having special restrictions. But maybe I'm too paranoid?

[sfackler]

What is an example of unsafe code that relies/would rely on those requirements?

[Kixunil]

Let's assume for instance that the ability to rewind gets implemented by suggested traits as discussed in #330. Naturally, the desire to add rewindable() combinator, which creates a wrapper type around the original BufMut to add rewinable features comes. Without the requirements being fulfilled, such wrapper might cause UB because of relying on the fact that bytes_mut returns the same bufer every time its called.

[sfackler]

What is some specific code that relies on that specific requirement?

[Kixunil]

pub struct Rewindable<B: BufMut> {
    buf: B,
    offset: usize,
}

impl<B: BufMut> Rewindable<B> {
    pub fn written(&mut self) -> &mut [u8] {
        unsafe {
                // Safety: we track how much data was written in `offset` field,
                // So unless `bytes_mut` returns a different buffer, then that memory is initialized.
                self.buf.bytes_mut()[..self.offset].assume_init_mut()
        }
    }
}

impl<B: BufMut> BufMut for Rewindable<B> {
// ...
unsafe fn advance(&mut self, amount: usize) {
self.offset += amount;
}
}

[sfackler]

That won't work with non-contiguous BufMut implementations anyway.

[Kixunil]

Double checking again, Chain relies on remaining_mut to have correct return value. If remaining_mut of a returns 0 despite still having remaining data, then advance_mut will be called on b without data being written there.

[carllerche]

But it is possible to implement BufMut completely safely... The BufMut implementation can only expose UB when it, itself, uses unsafe. There is nothing inherently unsafe about implementing BufMut.

[Kixunil]

In that case Chain is unsound, right?

[carllerche]

How is Chain unsound?

[Kixunil]

If remaining_mut of a returns 0 despite still having remaining data, then advance_mut will be called on b without data being written there.

[carllerche]

@Kixunil You are correct that a caller of BufMut is not able to defend against a broken implementation. According to the snippet you pasted, BufMut should be unsafe.