-
Notifications
You must be signed in to change notification settings - Fork 13.3k
#[target_feature]
mismatch on unsafe trait fn vs its impl causes sneaky UB
#139368
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
Comments
I agree this can be a footgun: the safety precondition implied by the Unfortunately, it seems difficult to automatically distinguish mistakes from sound usage:
Making the attribute unsafe is not quite right: ultimately UB only comes from a call to the method, which requires the caller to write an In analogy to unsafe fields, I could see an argument for making |
I agree with substantially everything you said. I'm curious about your opinion on a possible rustc warn-by-default lint for when a trait impl introduces more target feature requirements than the pub trait being implemented. |
I think that would definitely be good idea as a clippy lint. The bar for rustc lints is much higher and it’s always possible to uplift a clippy lint into rustc later. This pattern doesn’t always indicate a mistake, and when it’s actually what you want I don’t think you can silence the lint by writing the code slightly differently, so this lint would probably end up allowed/expected in some places. |
Maybe the attribute should be unsafe to apply to impl methods. rust-lang/rfcs#3715 |
RFC 3715 is interesting because:
Target feature attributes are the latter kind of unsafe so perhaps it should be spelled Note, however that it’s really only relevant for trait impls, not for inherent impls or free functions — without traits in the mix, there’s only one place where preconditions are defined and the caller always knows precisely which item will be called. Requiring the |
I did say it is actually the former kind of unsafe. The |
I agree with this. And so long as it only discharges those requirements, I think My remaining concern is that My use of syntax above is in reference to the feature itself, not a vote for a particular syntactic choice. I'll leave those to people way more qualified than me :) |
I suppose this is a matter of perspective. While drafting my first comment in this issue, I was leaning in the same direction for a while. But ultimately I think it's more useful to view it only as introducing a precondition the caller has to fulfill, because that's the approach that works across the entire language and doesn't require any special pleading for traits. It's language UB to call a We may then use this precondition to call other functions requiring the same target feature, which is why such calls are sound: we're only propagating what we got from the caller. Before target feature 1.1, this was annoyingly explicit: every function with
Returning to trait impls: yes, the body of the impl method can assume that the target feature is available and thus freely call other functions that require those features (which the compiler will happily let you do). But the immediate reason for that is the same as for free/inherent functions: calling the method would be UB if the features weren't available (again: even if the method body is actually empty), so in general it's unsafe to call the method and this precondition must be propagated up the call stack. The only thing that's different about traits is where those preconditions should be documented so the caller can see them. |
This is kind of a key thing though, no? If a trait specifies a safe function, an impl can't (and shouldn't be able to) make that function unsafe by adding new safety obligations to the (previously empty) set. In this case, it sounds like you're implying that adding new safety obligations is fine because the set wasn't originally empty this time. That doesn't seem right to me. So I wouldn't agree it's just a documentation issue, especially if no concrete way to resolve the issue purely at the level of documentation is presented. I don't see a feasible way for this to be solved at the documentation level alone, and I'm skeptical one exists at all. |
I'm not implying unilaterally adding a new safety precondition at the impl level is "fine". A Rust program written that way isn't sound. A trait impl defining an unsafe-to-call method can only rely on the preconditions stated by the trait's documentation for that method, not add new ones that it would like to have. But at the language level, it seems most useful to me to classify trait Foo {
/// # Safety
/// `n` must be a prime number.
unsafe fn foo(n: u32);
}
impl Foo for () {
/// # Safety
/// `n` must be odd. Note that `Foo` requires prime numbers, but
/// composite odd numbers are also fine for this impl.
unsafe fn foo(n: u32) {
// ...
}
} This impl is unsound because the author of the impl forgot that there is an even prime number, so their supposed refinement of
In such a case, I'd still assign the blame for the resulting UB to the fact that the author of In that sense, |
Broadly, I'm onboard with that 👍 In particular, I agree that incorrect refinements should be considered unsound, and that only the trait's safety requirements should be relevant to the caller. Just one thing I'd push back on:
I think "the Rust way" is that whenever possible, built-in tooling (rustc, clippy, etc.) should check this instead of relying on humans. It obviously isn't possible every time (e.g. the odd numbers and primes example), but with Between a rustc lint and a clippy lint, I personally would advocate for a rustc lint because I believe we've all agreed this is an unsoundness issue. But if you all think it should go to clippy instead, we can move the issue there. |
To be clear, I do not agree this is a language soundness issue. As stated before, I'd say it's a "language makes it hard to write sound unsafe code" issue, like the motivation for unsafe fields. You may call that a soundness issue, but it's different from when rustc emits incorrect code or the type checker lets you write I think it would be completely defensible to introduce an But requiring the |
Sorry for the confusion — my opinion is "the impl is unsound and should be flagged" and not "this Rust feature is unsound and should be rolled back." |
I think there's a more general issue here, also: having any Generally, unsafe code cannot rely on the implementation of safe In @hanna-kruppe's prime example: trait Foo {
/// # Safety
/// `n` must be a prime number.
unsafe fn foo(n: u32);
}
impl Foo for () {
/// # Safety
/// `n` must be odd. Note that `Foo` requires prime numbers, but
/// composite odd numbers are also fine for this impl.
unsafe fn foo(n: u32) {
// ...
}
} If it is true that implementing a safe For a very explicit example, this code invokes UB (calls // crate a
pub trait Foo {
// SAFETY: this is always safe to call // *1
unsafe fn bar();
}
// crate b depends on crate a
pub struct LocalType;
impl crate_a::Foo for LocalType {
// SAFETY: this is never safe to call
unsafe fn bar() {
// SAFETY: <() as Foo>::bar is never safe to call, so this is unreachable
unsafe { std::hint::unreachable_unchecked() } // *2
}
}
// crate c depends on crate a
pub fn quux<T: crate_a::Foo>() {
// SAFETY: as per Foo::bar's docs, Foo::bar is always safe to call
unsafe { <T as Foo>::bar() } // *3
}
// bin crate depends on all others
fn main() {
crate_c::quux::<crate_b::LocalType>(); // *4
} It cannot be If It could also be argued that Basically, In the OP example, the If I think this discussion is perhaps evidence that allowing Edit: There are some unsealed safe Footnotes |
I agree that the vast majority of traits with unsafe methods probably should be unsafe, and the exceptions are very subtle. That said, "the blame can't lie in safe code" is a useful heuristic but not 100% right in general. For example, BTreeMap's Of course, unsafe code authors also need to be careful and selective about when and how they trust safe code to be correct. It's definitely wrong to rely on an "open world" set of safe code, such as all impls of a safe trait or any value of a function pointer type, to be correct. That would make compositional soundness proofs impossible, and those are the big advantage of Rust over "just write C and prove that it's memory safe 🙃". But it also doesn't scale to say that unsafe code authors can only trust code they wrote themselves in the same crate. For starters, you often have to trust standard library APIs -- and the standard library shouldn't be unique in this respect. Rust doesn't currently have any way to say "I'm typing |
Nominating for T-lang discussion (hope it's the correct procedure to get team's attention!) @rustbot label +I-lang-nominated |
But if an implementation of a safe trait uses an impl Foo for Bar {
unsafe fn foo() { unreachable!() }
} has to be sound, since it's not discharging any obligations. So in your example, I'd say it's necessarily |
We discussed this in today's lang team meeting, and the consensus was that we should disallow impl methods with more target features enabled than those in the trait definition. Additionally, impls with fewer target features than in the trait definition should fire a refining lint. We would also be okay with disallowing any difference between the trait and impl target features for now, adding the ability to refine later. Hopefully we can get away with a small breaking change here, otherwise we can claw it back with an FCW, lints that transition to an error over an edition, etc. Another aspect we discussed is that possibility that some users might actually want to do this, and while there is a workaround of unsafely calling another function with different target features from the impl, the behavior of inlining around target features might create a performance regression for them. If this does come up we can discuss the possibility of adding a new unsafe attribute to allow such a difference between the trait and impl, but until a use case comes up we aren't discussing it yet. |
Not idea whether it makes a difference for your decision, but note that |
Anyone interested in making that proposal for us? cc @veluca93 |
I'm quite surprised we allow 👍 for FCW'ing when an imply restricts the target features compared to the trait declaration -- us just FCW'ing all uses of |
Sorry, I completely missed this ping - I am also fairly surprised we allow IMO a FCW seems like a reasonable way forward (and I'd be willing to try it out when I have time) I also think a |
I tried this code: (playground)
The "imagine this type is in crate X" is not strictly necessary — this is still a problem within a single crate too. It's just much easier to spot the problem when the code is all together instead of in 3 different and separately evolving crates.
I expected to see this happen: to soundly use unsafe items from a trait, reading the safety comments on the trait and its items should generally be sufficient. A safe (non-
unsafe
) attribute shouldn't be able to be used in a manner that causes hard-to-find unsoundness and UB. This code should either be rejected outright for tightening the trait's safety requirements in the impl, or at least require#[unsafe(target_feature)]
plus raise a lint for the requirements-tightening.Instead, this happened: this code compiles fine with no warning. The attribute is not
unsafe
. Even an unintentional "editing error" is likely to silently cause impls' requirements to diverge from the trait's requirements and lead to unsoundness.Meta
rustc --version --verbose
:From the playground.
The text was updated successfully, but these errors were encountered: