Fundamental Soundness Problem

[someguynamedjosh]

UPDATE: A fix has been implemented. 0.16.0+ is no longer unsound in the described way, but support for template parameters had to be cut in the process.

TL;DR: Ouroboros has a soundness problem. A fix has been attempted but it was also unsound. Currently the compiler does not actually produce unsound machine code, but it may begin doing so at any point in the future. Migrate code to use self_cell instead.

More details:

Currently, Ouroboros works internally by creating a struct where all uses of 'this are replaced by 'static. However, a recent addition to Miri checks that references passed to functions are valid during the entire execution of the function, even when those references are passed inside a struct. This poses an issue for dropping self-referencing values, as the reference becomes invalid during the dropping process. 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.

A fix was attempted, where the struct would be turned in to a raw byte array and only transformed back into the underlying data type inside each function. This is allowable as a reference can be created and die over the body of a function, as long as the reference was created inside the function. However, this is also not sound if the original struct contains padding. There is no way to initialize padding bytes to a known value that Miri will accept, so when it is cast to an array some of the bytes contain uninitialized values. This is not acceptable (despite the fact that these bytes are never read) due to the potential for optimizations that may read from the uninitialized bytes. Besides which, this fix does not allow for template or constant parameters as there is no way to check the size of a templated type without giving specific, concrete values for the template parameters.

At this point, I'm just going to move on to other things. self_cell appears to be an excellent alternative for anyone using this crate.

[someguynamedjosh]

Rustsec PR: rustsec/advisory-db#1707

[Bwallker]

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

[Voultapher]

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

[rklaehn]

Is it clear that self_cell does not have this issue, or is it possible that it just did not get the same amount of scrutiny?

[Voultapher]

@rklaehn from what I can tell it doesn't have the same issue because it doesn't store the values as 'static. Instead it stores the value as NonNull<u8> and materializes lifetimes within the functions. It has an extensive test suite that is tested with miri. But this stuff is really complex and I can't exclude the possibility that it has future soundness issues as it has had in the past.

[danielhenrymantilla]

@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 (rust-lang/rfcs#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);

[billy1624]

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

[dignifiedquire]

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

[RalfJung]

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 rust-lang/rfcs#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.

[RalfJung]

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 rust-lang/rfcs#3336.

[someguynamedjosh]

Thank you @Bwallker and @danielhenrymantilla for the suggestion, I've tried it out and it appears to make the test suite pass under miri. It still won't support template parameters, but it's enough for now. I'll be publishing a version of the library that uses this fix soon and update the security advisory accordingly.

[edward-shen]

@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?

[someguynamedjosh]

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.

[Voultapher]

@joshua-maros given the issued rustsec advisory I wonder if previous versions should be yanked from crates.io, what do you think?

[someguynamedjosh]

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.

[Voultapher]

Thanks for clarifying.

[RalfJung]

IMO yanking is entirely unnecessary here, given there are (AFAIK) no reports of this unsoundness causing a concrete issue.

[Voultapher]

Out of genuine interest, and having been in the same situation before. My understanding behind the motivation to yank in such situations was that it sends a clear signal to users, which might also run tests with miri, that their current version of the crate may cause issues with future compilers and that upgrading is necessary to avoid those issues.

[RalfJung]

🤷 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.