Fix SB UB (without breaking MSRV)

[clubby789]

Implementation of #79 that works on rustc 1.31.

[vorner]

Also, can you make the history / the commits a bit nicer? I mean, there's the change with CI and the change with fixing the UB, so an optimal history would contain two commits, one for each 😇

[vorner]

I don't believe this is actually true/safe.

1. Ever reaching unreachable_unchecked is explicitly an UB. So if the strong_count is at the max value, it simply invokes UB.
2. Even if it „just“ turned off the check inside Arc, this would not be safe. Because it would temporarily overflow to 0 and, in some other thread, could be incremented twice, decremented once, reach 0 again there and get deallocated.

We could use the approach without that optimization, and we could measure it that way to see if there's any actual change in speed. That would be the more elegant way and I suspect it might actually be the case.

If it turns out it is expensive, we could use a different trick:

1. Make a „hollow“ copy of an Arc, one that does not own its ref count. It could be done with eg. std::ptr::read.
2. Call Arc::into_raw on that one.
3. Turn it back into the hollow copy of Arc with Arc::from_raw.
4. mem::forget it, so we get rid of it without calling its destructor.

[clubby789]

That's very true - I must admit I only considered Rc and pasted the working code for Arc. I believe

fn as_ptr(me: &Arc<T>) -> *mut T {
    let ptr = Arc::into_raw(unsafe { std::ptr::read(me) } );
    ptr as *mut T
}

should be safe and work for both variants. Neither ptr::read nor into_raw will affect the refcount (ptr::read is a bitwise copy and into_raw calls mem::forget to avoid running the destructor).
The safety is much easier to verify here - we are calling ptr::read on a *const T derived from an &T which is guarenteed valid.

[vorner]

The previous wasn't safe even for Rc, due to the fact that you could reach the unreachable_unchecked. The first point was true even for Rc.

I know that the read is just a bitwise copy and into_raw currently calls the forget. In the current version, these are enough. Nevertheless, this is with the knowledge of internal implementation. The internal implementation may change and these properties are not guaranteed. The only description is that into_raw shall be paired with from_raw. The read shall then be paired with forget. So it's about future proofing these for possible change in the standard library.

[clubby789]

Once we've agreed on an approach, I'll squash the commits as you suggest 😄

[clubby789]

After some investigation, I think I'd probably reccomend an MSRV bump. This new code is sound on stable, but still unsound on 1.31 (unsure when the change was made) - https://github.com/rust-lang/rust/blob/b6c32da9b0481e3e9d737153286b3ff8aa39a22c/src/liballoc/rc.rs#L391-L395

The current implementation of into_raw uses as_ptr, which gives a *const T that has write permission. 1.31 implements this with &*this, which gives a *const T that does not have write permission.

Applying this change makes this part of the code sound on stable, but the supposed supported version exhibits UB and, as far as I'm aware, cannot be made sound.

If you agree with bumping I'll reimplement with as_ptr and squash, otherwise I'll squash as-is.

[vorner]

This new code is sound on stable, but still unsound on 1.31 (unsure when the change was made)

I'd tend to disagree on this one. Back then, the model wasn't specified and this was at worst a gray area. It was common practice and the compiler didn't mis-compile it. In addition, this is inside the standard library and that one is in a position where it may abuse the knowledge of internal compiler workings. I suspect the change to current implementation was made at the point where the talks about defining the model somewhat better was on the table and until that point it was considered OK by the compiler folks.

Besides, isn't it that for pointers, it matters what permissions one actually uses, not what they have?

I do hope nobody is actually using 1.31 in practice. But if they are, bumping the MSRV will only cause them staying at the previous version of the code which has the same problem. So bumping it solves nothing.

[vorner]

Hello

This fell asleep for some time. I've taken the liberty of picking the interesting commits, rebasing them and merging it as #86. It'll be part of the next release.

Thanks again for finding the cause.