Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why is ErasablePtr not implemented for Weak<T>? #85

Open
Kijewski opened this issue Mar 22, 2023 · 3 comments
Open

Why is ErasablePtr not implemented for Weak<T>? #85

Kijewski opened this issue Mar 22, 2023 · 3 comments

Comments

@Kijewski
Copy link

The build.rs mentions

// NB: Requires this impl to cover T: ?Sized, which is not the case as of 2020-09-01.

But couldn't you add an implementation for a sized T, so one can use Weak<T> together with ptr_union?

@eggyal
Copy link

eggyal commented Mar 15, 2024

Worth noting that Weak::into_raw has covered T: ?Sized since rust 1.51 (of which @CAD97 is no doubt aware since he stabilised it, so presumably the impl can now be added irrespective?

@CAD97
Copy link
Owner

CAD97 commented Mar 16, 2024

It just escaped me that this was actually properly possible to implement now. I've been dancing on a small rewrite/restructuring of the erasable crate for a while now, probably contributing to missing this.

The reason I didn't think about limiting the impl to just T: Sized at the time is that I was more thinking about the slice-dst usecase than the ptr-union one.

@CAD97
Copy link
Owner

CAD97 commented May 2, 2024

Rotating back to this: ErasablePtr has a requirement that Self::into_raw must be non-null and (critically) dereferenceable (enough to be unerased). Weak does not satisfy either of these properties, as a Weak::new()-created weak reference dangles and Weak::as_ptr specifically allows for the raw pointer form of such Weak to be dangling, unaligned, and/or null. The current implementation returns usize::MAX as *const T, which is dangling+unaligned, but (usize::MAX as *const T).wrapping_byte_add(offset_of!(RcInner<T>, value)) (dangling+unaligned) or 0 as *const T (dangling+null).

Note that it is fully possible to get the canonical dangling Weak for unsized pointee types by going through an unsizing coercion, e.g. Weak<[T; N]> to Weak<[T]>.

Additionally, please note that the dangling Weak relaxation of pointer properties is not limited to only canonical Weak::new() handles, but also extends to Weak handles after the last strong reference has been released. Weak is within its rights to canonicalize dangling references on Weak::into_raw.

For this reason, while impl<T: Sized> Erasable for Weak<T> could be sound, using Weak<T> in a ptr union is at best questionable. Making BuilderN<…, Weak<_>, …> will be unsound unless Weak guarantees dangling weak's into_raw is aligned, and making UnionN<…, Weak<_>, …> will be unsound unless Weak guarantees a pure into_raw. The latter I can see happening, but the former seems unlikely1.

…Actually, this has made me realize that Thin has the same requirement of pure into_raw, and ErasablePtr directly implies it doesn't. I understand the requirements for erasable to be sound much better now than I did when I first wrote the crate, so I suppose it's time for a v2.0 that fixes these issues. Thankfully I believe that it's only ever doc ambiguity rather than true unsoundness.

Footnotes

  1. I see guaranteeing non-null as more likely a guarantee to provide than aligned, as it's an easier niche to utilize. Providing both isn't possible given Ts with both a high alignment and zero size (e.g. [f64x4; 0]) since every non-null aligned address for RcInner<T> is potentially a valid allocation address, as even null().wrapping_sub(1) can dereference to align bytes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants