Adding soundness requirement to unsafe trait after the fact: possible less-breaking outs

Context: discovery on Twitter and patch.

I have had the unfortunate honor of finding an extremely subtle soundness issue with Erasable::unerase's requirements for implementors (link is to prior version). The short of it is that 1.0.0 documentation suggests creating a temporary shared reference (&_) from the input pointer, and doing so can potentially cause undefined behavior for some cases the Erasable trait is supposed to support.

The new documentation (link to documentation for git head) explicitly calls out the issue, and with version 1.1.0 published, implementors of the trait will be required to uphold the new, slightly more restrictive requirements for implementation. The issue is that this is a semver-compatible release.

I've taken some measures to make this less breaking of a fix, and detail them below. I'd like community input on if this seems reasonable, and if perhaps there's a better way to do this than what I've implemented currently. (Additionally, the sooner I can publish the fixed version and yank the problematic one, the better. v1.0.0 is the only version currently published.)

The migration path is made possible via a new default-provided associated const on the trait, ACK_1_1_0. It's whole point is that it is defaulted to false and implementors must override it to true. (To aid in this, the ERASABLE_ENFORCE_1_1_0_SEMANTICS environment variable will remove the default implementation.) Additionally, users of the Erasable trait are encouraged to check the value of ACK_1_1_0 if they actually would be made unsound by unerase manifesting a temporary shared reference.

On top of this, the edge case that would be unsound is a very unlikely edge case: there has to be an erasable pointer type with shared mutability that is shared between threads, such that a write happens that potentially races with the reference in unerase. Oh, and the trait and method are already unsafe and exist to work with raw pointers, so an implementor should be paying attention to these details. It's not a soundness hole in safe code (that my libraries have written), it's a potential soundness hole given a use of my unsafe APIs in a way previously documented as OK but that I can't think of a real way to end up running into without contrived code specifically to exploit it.

But still, I pride myself in my code being sound, even in obscure edge cases that require unsafe (but are technically not disallowed by documentation), so the issue needs to be addressed. And if I can do so without a semver-major version bump, that'd be ideal. (And yes, the README does loudly call this out now.))

4 Likes

A technically valid option is to yank 1.0.0, and then that leaves (technically) no requirements on 1.1.0 as there is no published version anymore.

But this is not a good solution because a) it goes against the spirit of semver and b) there isn't any compile-time breakage! So the solution of just fixing the documentation in 1.1.0 and yanking 1.0.0 is the worst possible option, worse even than leaving 1.0.0 up, because it's pretending to be a better choice without actually being and better. Anything written against 1.0.0 would just silently work on modern versions and contain the extremely subtle soundness hazard.

For what it's worth, erasable v1.1.0 has been released with this upgrade path.

The use of the guard const is also convenient in that it can somewhat transparently be removed in the future if deemed unnecessary (just #[doc(hidden)] it with a default), and it allows shimming from a 1.0 release to a 2.0 release to still be possible, if that is deemed desirable in the future. (And if specialization allows it, it could theoretically be a bidirectional shim.)

1 Like

The constant seems like a good / least bad idea to guard against it, to be honest.

Plus yanking a crate's version does not completely prevent it from being used, given that:

existing crates locked to a yanked version will still be able to download the yanked version to use it.

1 Like