-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Document subtleties of ManuallyDrop
#130279
Conversation
@scottmcm what did you nominate this for? should this PR be blocked on that or is this about a sperate issue than adding documentation warning about what is a potentially dangerous pattern right now? |
cc @rust-lang/opsem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some wording/style nitpicks
@Noratrieb I'd much rather just remove the subtleties so we don't have to document them :P Dunno whether that's practical, though. I just don't want to do a bunch of ManuallyDrop -> MaybeUninit everywhere now, then undo it again shortly. |
The subtlety of “using the value after it's been [manually] dropped” continues to be annoying. The one for moving can and should be fixed, but the case of derives using the value after it's been dropped is a clear cut violation of the safety contract, unfortunately. (This is why unsafe postconditions on the caller are hard.) The diff for patching move-based holes can be minimized by utilizing a temporary interim impl of |
Such an impl is available in Yandros's maybe-dangling crate. Should this link be added to the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me, but I find it deeply unpleasant. I'll leave it open for the lang team nomination though.
Just saw this comment by @pnkfelix from the lang team meeting
A The upshot is that to make code like this not trigger Miri UB, we'd have to
I don't think we should do either of these. We could stop adding |
The outcome we discussed in the lang team meeting is that we should document this behavior for now, but also document that it is likely to change in the future once RFC 3336 (MaybeDangling) is implemented. I see that the PR already does this, so I think we can move forward with merging it. @rustbot label -I-lang-nominated |
/// # Interaction with `Box` | ||
/// | ||
/// Currently, once the `Box<T>` inside a `ManuallyDrop<Box<T>>` is dropped, | ||
/// moving the `ManuallyDrop<Box<T>>` is [considered to be undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think documentation should clarify that this also applies to any types that directly contain Box<_>
, like (Box<T>, u8)
or struct Foo(Box<T>);
.
I also believe that per current rules, calling ManuallyDrop::drop()
on any type that directly contains Box<_>
is insta-UB, even if you don’t move it afterwards. Technically, it doesn’t neccessarily count as producing an invalid value:
“Producing” a value happens any time a value is assigned to or read from a place, passed to a function/primitive operation or returned from a function/primitive operation.
but at least touching it in any way (like creating a reference) is certainly UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't name the place after calling ManuallyDrop::drop
, everything is fine; the place isn't touched after that point. Because ManuallyDrop
does not have any drop glue, it does not get a drop_in_place
terminator at the end of scope, and thus the place is never "used" implicitly by the language, and no reference is created unless you create it yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reworded the text to clarify that the UB also applies to types containing Box
.
I am pretty sure that calling ManuallyDrop::drop()
on a type containing Box
is not UB. rust-lang/unsafe-code-guidelines#245 only states that using the ManuallyDrop
later is UB. If you believe that ManuallyDrop::drop()
is insta-UB, then I would like a citation to prove otherwise.
Of note, Miri doesn't seem to complain anything if I create a reference to an already-dropped ManuallyDrop<Box<T>>
. I believe this is because we are still undecided on whether the existence of a &T
implies that the T
must be valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading a reference, I don’t think I can prove that it’s insta-UB. This is certainly UB per reference though:
let mut x = ManuallyDrop::new(Box::new(42));
unsafe { ManuallyDrop::drop(&mut x); }
let _y = &x;
since it produces (by writing to a place) an invalid value. Reference takes a conservative stance and declares references to invalid values to be invalid:
A reference or Box that is dangling, misaligned, or points to an invalid value (in case of dynamically sized types, using the actual dynamic type of the pointee as determined by the metadata).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From rust-lang/unsafe-code-guidelines#412:
Note that the Rust reference currently answers ["whether a reference requires the pointed-to data to be valid"] with "yes", but in my view this is mostly because we haven't yet figured out what exactly the weaker requirement is that we actually want to impose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know. I still think that documentation should be coherent: if we document this as being unsound in reference, it should be also documented as being unsound in other places that deal with the same situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found out that the nightly version of the reference now states:
A reference or
Box<T>
must be aligned, it cannot be dangling, and it must point to a valid value (in case of dynamically sized types, using the actual dynamic type of the pointee as determined by the metadata). Note that the last point (about pointing to a valid value) remains a subject of some debate.
This PR is still labelled with S-waiting-for-team. Which team is it waiting on? |
@bors r=thomcc,traviscross rollup=always |
…llaumeGomez Rollup of 11 pull requests Successful merges: - rust-lang#130279 (Document subtleties of `ManuallyDrop`) - rust-lang#130517 (Add the library workspace to the suggested rust-analyzer config) - rust-lang#130820 (Fix diagnostics for coroutines with () as input.) - rust-lang#130833 (Fix the misleading diagnostic for `let_underscore_drop` on type without `Drop` implementation) - rust-lang#130845 (Utf8Chunks: add link to Utf8Chunk) - rust-lang#130850 (Pass Module Analysis Manager to Standard Instrumentations) - rust-lang#130861 (Use `mem::offset_of!` for `sockaddr_un.sun_path`) - rust-lang#130862 (rustdoc: do not animate :target when user prefers reduced motion) - rust-lang#130868 (Update FIXME comment in s390x_unknown_linux_*.rs) - rust-lang#130879 (Pass correct HirId to late_bound_vars in diagnostic code) - rust-lang#130880 (add missing FIXME(const-hack)) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#130279 - theemathas:manually-drop-docs, r=thomcc,traviscross Document subtleties of `ManuallyDrop` After seeing rust-lang#130140 and rust-lang#130141, I figured that `ManuallyDrop` needs documentation explaining its subtleties, hence this PR. See also rust-lang/unsafe-code-guidelines#245
After seeing #130140 and #130141, I figured that
ManuallyDrop
needs documentation explaining its subtleties, hence this PR.See also rust-lang/unsafe-code-guidelines#245