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