ouroboros: Fundamental Soundness Problem

Hey, I just wanted to say thank you for all the hard work you put into ouroboros.

EDIT: ManuallyDrop currently does not prevent the miri error, which I’d argue to be a bug (ManuallyDrop should include MaybeDangling, shouldn’t it? cc @RalfJung)

ManuallyDrop is not currently specified to have the MaybeDangling behavior, and in the codegen backend (to my knowledge) there is no exception for ManuallyDrop. So this is not a bug, Miri is behaving as intended (and it would be bad for Miri to make an exception here if codegen doesn’t also have such an exception).

This looks like a good use-case to add to https://github.com/rust-lang/rfcs/pull/3336. Would it be possible for someone to prepare a brief writeup of what ouroboros needs, and how MaybeDangling is required to achieve that goal?

Effectively, since self-referencing structs are not allowed in vanilla Rust, there is no allowance for dropping data during a function that has also been given a reference to that data. There’s usually no way to pass a and &a to the same function.

FWIW, there is currently one exception to this, and that is a &UnsafeCell<T> reference. Those must be dereferenceable on function entry, but they are permitted to be deallocated while the function runs.

I intend to continue now that a fix has been found. I was planning on leaving due to there being nothing else I could do, since that is no longer the case I will keep working on it.

Did you consider transmuting to an array of MaybeUninit<u8> instead to avoid the padding/unitialized bytes problem?

🤷 IMO the advisory is good enough, people that care enough will see the warning in cargo audit and it’s not worth disrupting everyone else.

That’s just my opinion though.

To be clear, when I said “intended behavior” I meant that this is not a Miri bug. I do think we should change this. But I can’t just change this in Miri, codegen needs to be adjusted first, and that probably needs an RFC, and that’s why I wrote https://github.com/rust-lang/rfcs/pull/3336.

I will be doing that but only after people have had a chance to migrate. I had previously yanked a version too soon after uploading a fix, and it caused a lot of chaos as many libraries were suddenly broken and unable to compile. Especially since the compiler does not currently actually produce unsound code, this issue is not pressing.

@joshua-maros, do you mind clarifying if you intend to continue working on ouroboros moving forward? The original post mentioned that you planned on moving on. Is that still the case (and 0.16 is the last version) or have plans changed?

Any suggestions for alternatives that allow generics? self_cell is not a real replacement for even simple things, like wrapping a MutexGuard unfortunately.

Hey @joshua-maros, thanks for the efforts of maintaining ouroboros for years!!

@joshua-maros wrapping in ManuallyDrop should palliate the issue at the moment, until we get that magic of ManuallyDrop extracted into its own wrapper type, MaybeDangling (https://github.com/rust-lang/rfcs/pull/3336).

• EDIT: ManuallyDrop currently does not prevent the miri error, which I’d argue to be a bug (ManuallyDrop should include MaybeDangling, shouldn’t it? cc @ralfjung)

Until then, MaybeUninit does very much work; you may just lose Option-discriminant-elision optimizations:

// passes miri:
let b = Box::into_raw(Box::new(42_u8));
let r = ::core::mem::MaybeUninit::new(unsafe { &*b });
(|b, _r| {
    drop(unsafe { Box::from_raw(b) });
})(b, r);