-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
null trait object raw pointers are UB #63851
Comments
trait T { }
fn main() {
a(unsafe { std::mem::zeroed() });
}
fn a(_: *mut dyn T) {} ; ...
call void @_ZN10playground1a17hbc18b5e956a8e54dE({}* %1, [3 x i64]* noalias readonly align 8 dereferenceable(24) %2), !dbg !145
; ... The vtable is marked as |
Thanks for reporting! In rust-lang/unsafe-code-guidelines#166 we have been discussing, rather theoretically, the validity requirements for wide raw pointers. We were not actually aware that there already are validity requirements in place for them. That seems very surprising to me. Was this ever discussed in depth, or did it "just happen"? Cc @rust-lang/wg-unsafe-code-guidelines @eddyb |
@bjorn3 on top of that, |
If we decide this is not a bug, we'd have to adjust https://doc.rust-lang.org/nightly/nomicon/what-unsafe-does.html (and also the reference). It would be a very special case to require anything for raw pointers that are not dereferenced. |
The assumption is that the vtable is a valid reference, so that all raw pointers have valid pointee types.
IMO, if the information associated with that In custom DST terms, we could treat There's also something to be said about having a null vtable, that Go people have some experience with (but I don't, so I'm not the best person to comment on it). |
The curious part here is that no one involved in the UCG realized that this is the current status.
Where is there a complication here? I mean not in terms of the implementation, that shouldn't be a primary concern in terms of language design, but in terms of documentation and teaching. Requiring vtables of raw pointers to be valid complicates the language as well, it would be an exception from the principle to not require things off of raw pointers.
I was more thinking of |
I don't even think I'm the first to come up with that, but I can't remember who did. In the end, I guess it's a tradeoff between:
(these are not the only outcomes, but they should be somewhat representative) |
If one reads the custom DST proposal, there's a reason that we expect raw pointers to DSTs to have valid metadata. The rawness of the pointer is unrelated to the rawness of the metadata, imo -- even if you have |
There's also the ability for methods taking (It claims that "even today you could UB in safe code with |
It seems like this is at least somewhat expected, because if all pointers could be Also, do any of the choices here have any impacts to the optimizations to never reload vtables when multiple calls get made? |
FWIW, |
That won't hold up with custom DST anyway, e.g. if
Isn't the issue there just that calling methods with a "raw receiver type" will be unsafe? |
But if we go for
Yeah it would be nice to not tie anything to the
So "it's UB to call with invalid Overall I feel like we should look into this a bit more, but I can see how the "low-level compromise" lets us focus on It's almost like raw pointers are |
Turns out that for raw slices, safe code can cause them to have invalid metadata: 123456789 as *const [(); usize::max_value()] as *const [()] as *const [u64] Ouch. |
@RalfJung Wow, so we couldn't actually have |
Looks like probably it should be just So, I was just interpreting that length requirement the wrong way so far. I'll adjust the various PRs (Reference, Nomicon, Miri) accordingly. |
@RalfJung The issue is not invalid metadata -- invalid metadata is fine. The big thing that you want is initialized metadata.
|
"initialized" as a term turned out to be pretty unsuited. I mean "valid" specifically as in "validity invariant". |
Sure, whatever. It needs to be a valid value of type |
@ubsan You can circumvent that by defining raw pointers as having |
@eddyb you can, but I don't think there's a reasonable argument for it. Creating and destructuring raw pointers safely seems really nice to me. |
"really nice" is IMO not a sufficient argument for introducing this kind of UB. We should have some optimization that's worth it, ir some amount of spec simplification that's worth it. The "default" answer should be "let's avoid UB, to make unsafe code author's lives easier". |
@RalfJung how does one introduce UB in this case? Are people generally returning I would argue that this is removing UB that people get when calling methods through a |
That would be an example of an operation that becomes UB under your semantics. Doesn't seem too strange to me to zero-initialize a struct with a wide raw pointer field.
There is literally no program that is UB under my semantics but not under yours. This is not about the safety invariant, it is about the validity invariant. |
@RalfJung shrug I don't have a horse in this race. I just think they're nicer semantics. If you think it's more important to allow |
Opening after some discussion in rust-lang/miri#918 (comment)
This playground snippet crashes on an illegal instruction on release: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=7c69493026add62256996d204e1278c0
Are vtable ptrs in trait object ptrs NonNull? Is that intended? This (2 year old) comment would suggest so rust-lang/rfcs#433 (comment)
CC @RalfJung
The text was updated successfully, but these errors were encountered: