Weak refcounted pointers can dangle

[CAD97]

... so fix internal comments to reflect as such.

This is a followup to the changes in #74160. cc @rust-lang/lang: we did FCP on said PR (allowing Weak::into_raw/from_raw) with unfortunately incomplete information. Thankfully, this doesn't mean anything yet, so long as we're careful. Explanation below.

cc specifically @RalfJung, @mahkoh, from rust-lang/rfcs#3041, where this was brought to my attention.

@bors rollup=always
(comments only change)

---

The important example:

let ptr: Weak<[u8]> = Weak::<[u8; 32]>::new();
Weak::from_raw(Weak::into_raw(ptr));

Unsize coercions work on Weak, so we've been able to have dangling Weak to unsized T since Weak::new was stable.

This is where #74160 comes in. Now we have Weak::as_raw, Weak::into_raw, and Weak::from_raw. These mean that it's possible to go from "whatever pointer Weak<T> stores" to *const T and back, even in the case that T: ?Sized (as of #74160). This offsetting currently requires knowing the alignment.

When #74160 was merged, it did so without realizing that unsized dangling Weak could be constructed, and we said it stabilized the use of align_of_val_raw for "potentially dangling pointers to sized T and valid pointers to unsized T". We unfortunately ignored "valid pointers to dropped unsized T" and "dangling pointers to unsized T unsized with valid metadata."

The simple assumption from this API being stable now is that any future work around custom (potentially thin) DSTs will require all DSTs to have alignment determinable from the pointer metadata (and thus align_of_val_raw can be safe). However, with careful tweaks to the implementation of ArcInner/RcBox^[1], we can avoid this requirement.

The issue at hand is that converting between Weak<T> and *const T requires knowing the alignment of T. There are three ways to sidestep this requirement:

• Don't offset dangling weak in the three methods from Allow Weak::as_ptr and friends for unsized T #74160 (or otherwise detect and use another sentinel). This would add a branch to these methods, but would also be potentially useful (as we could give out a null pointer for dangling weak).
• Always store Weak<T> as a pointer-to-T, rather than pointer-to-ArcInner/RcBox^[1]. This pessimizes the non-dangling case, as it will always have to align_of_val and backwards offset to touch the reference count.
• Drop #[repr(C)] layout and store with front padding instead of internal padding, PLUS store a reference to the reference count (rather than the allocation head). This only pessimizes the final deallocation (as it has to offset backwards to get the allocation head to deallocate), but it greatly complicates the implementation, just so that the offset between the Weak<T> pointer and *const T pointer is a consistent two words.
• And of course, the fourth, just say that this is allowed, and require any thin DSTs to have statically known alignment. (There is, though, at least one "dynamically aligned type" usecase I can think of: trait objects with thin pointer / inline vtable pointer.)

So, unless I'm mistaken, nothing about the stable Arc/Rc API strictly requires that align_of_val_raw is possible. It's just that the choice is between align_of_val_raw or making Arc/Rc even more complicated to avoid calling align_of_val_raw.

(Side note: some version of this analysis should probably go in lang-notes somewhere as notes on restrictions to any future design of custom (potentially thin) dynamically sized types.)

[1]: off-topic, but we really should probably unify those names, probably to ArcInner/RcInner

[rust-highfive]

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

[RalfJung]

This is not "the same safety requirements as align_of_val_raw", though. align_of_val_raw quite deliberately only talks slices, trait objects, and extern types -- for all other unsiezd tails, "it is not allowed to call this function".

[CAD97]

This is super awkward to specify cleanly without copying over the entire docs from align_of_val_raw. When I made this adjustment, I figured that "comes from an unsize coercion" would be a tighter bound than align_of_val_raw spells out, since the only types of unsize coercion are for slices and trait objects. "Comes from unsize coercion" is in fact a simpler spelling of the requirement for trait objects (valid vptr). For slices, it's exactly equivalent iff it's impossible to name a fixed-size array type that has a layout close to the size of isize::MAX (such that RcInner<T> is too big). I think that the error "values of the type rc::RcBox<[u128; N]> are too big for the current architecture" is enough for that, though I do think relying on said error for soundness is suboptimal.

(Also, we could potentially relax align_of_val_raw to limit the magnitude of alignment (which it probably should anyway) rather than size.)

[RalfJung]

The reason I don't like "comes from an unsize coercion" is that it is open-ended -- it is true for the unsizing coercions that currently exist, but that set might change in the future. The align_of_val_raw docs are quite deliberately stated in a way that is not open-ended in this way.

That's also why "comes from an unsize coercion" is not tighter than the bound on align_of_val_raw.

[CAD97]

Side note: if I counted right, #74160 is on beta right now (going stable on Jan 1), so if we're super worried about this, we could technically still back it out. I think it's unnecessary, but I don't make the call, and it's the safe option.

[joshtriplett]

@CAD97 Thank you for bringing this up. Even though it might not be necessary, I don't want anyone having to deal with this in a hurry over the holidays. Let's go over the long-term implications with you in more detail at a @rust-lang/lang design meeting in January (the 2021-01-06 meeting is still available), and make the decision then.

[CAD97]

Closing in favor of #80764, per T-lang consensus in today's meeting.