Make naked functions incompatible with certain attributes

[bstrie]

See the comment thread beginning at #90957 (comment) for justification.

Commit breakdown:

1. Fix copy/paste errors in check_attr

2. Consolidate checking for attributes not compatible with #[naked]

   Currently #[inline] and #[track_caller] are incompatible with #[naked], and there are more such attributes on the way.

   In preparation for this, consolidate the existing checks into a central location so that they can all be handled together.

   In addition, clean up the wording of the existing error messages.

3. Forbid #[target_feature] from being used on naked functions

   Per the comment at Tracking Issue for RFC #2972: Constrained Naked Functions  #90957 (comment) , make these two attributes incompatible out of an abundance of caution. This may be allowed in the future.

4. Forbid testing attributes from being used with #[naked]

   Per the comment at Tracking Issue for RFC #2972: Constrained Naked Functions  #90957 (comment) , this prevents #[naked] from being used with #[test], #[should_panic], #[ignore], and #[bench].

   This also fixes an oversight where the #[naked]-related lint "undefined_naked_function_abi" was not being properly registered; it is now possible to allow/deny it as per usual.

[rust-highfive]

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

[rust-highfive]

r? @jackh726

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

[GuillaumeGomez]

Error explanations look good for me (they don't need a "fixed" example in this case considering how simple they are). Please wait for approval from someone on the compiler team before r+.

[bstrie]

@GuillaumeGomez thanks, these were so simple that I wasn't even sure if they deserved their own extended explanations. Is it policy that every error should ideally have its own extended explanation someday? If not, what's the cutoff point for when not to make an extended explanation?

[GuillaumeGomez]

If there is an error with an error code, then it should have an explanation (easy to remember 😆). There is a tracking issue for this as well but it might need to be updated (new error codes have been added I suppose).

[bstrie]

@GuillaumeGomez right, but I have the same problem when deciding if an error is too simple to deserve its own error code. :P

Also, I could use your advice in resolving the build failure. The problem is the doctest for one of the extended descriptions:

    ```compile_fail
    #[target_feature(enable = "sse2")]
    #[naked]
    unsafe fn foo() {}
    ```

The problem is that I'm using compile_fail instead of compile_fail,E0789. This is deliberate, because target_feature will fail to compile with a different error code when used on a platform that doesn't support sse2. How do I deal with platform-specific tests in this context?

[jackh726]

Would it better to have a single error code for invalid attribute mixing? (Or maybe just X + naked?)

[jackh726]

I could imagine in the future more attributes falling into this category.

[bstrie]

Would it better to have a single error code for invalid attribute mixing? (Or maybe just X + naked?)

Possibly, I'm open to suggestions as to what is most appropriate here. Alternatively we could have just two error codes, one for codegen attributes + naked and one for testing attributes + naked.

[bstrie]

Thinking about it, I like the idea of consolidating these error codes into just "codegen attributes + naked" and "testing attributes + naked". As a bonus, we can avoid having an explicit example for target_feature, which sidesteps the need to bend over backwards to make this doctest platform-specific.

[GuillaumeGomez]

The problem is that I'm using compile_fail instead of compile_fail,E0789. This is deliberate, because target_feature will fail to compile with a different error code when used on a platform that doesn't support sse2. How do I deal with platform-specific tests in this context?

You can use ignore with parens explaining why and to add it into the tidy checker.

[npmccallum]

@bstrie Would it be better to have an allow list for acceptable naked functions attributes? This would deny all new attributes without any manual intervention.

[bstrie]

Would it be better to have an allow list for acceptable naked functions attributes? This would deny all new attributes without any manual intervention.

I think that would be an unnecessary maintenance burden. IMO, when a feature is stabilized, it is the responsibility of the people behind that feature to ensure that it works with all existing language features. Thus, with naked functions it is our responsibility to ensure that it works correctly with all existing attributes, and to prevent it from being used with any attributes that it is incompatible with. Then, once naked functions are stabilized, it is the responsibility of future language features (e.g. new attributes) to ensure that they work correctly with all existing language features, which will include naked functions. Unless someone wants to argue why naked functions represent some greater risk than usual, I consider it outside the scope of this stabilization effort to try to pre-emptively prevent problems that may possibly arise from interactions with an unknowable list of future features.

[jackh726]

I still am not sure if it makes sense to have two separate errors (codegen + testing). And even, if it's worth having a single error code for all "invalid attribute combinations". I think I'd feel more comfortable looping in either @rust-lang/wg-diagnostics or @rust-lang/compiler for thoughts.

[npmccallum]

I think that would be an unnecessary maintenance burden. IMO, when a feature is stabilized, it is the responsibility of the people behind that feature to ensure that it works with all existing language features.

How do we know that new feature authors have done this due diligence regarding naked functions? They add their new feature to the allow list with appropriate test coverage.

All new features would be opt-in. So at least we wouldn't break existing code. However, where people do opt in, the failures can be subtle and hard to debug. Even worse, they might not fail but instead silently succeed with security implications (i.e. the stack is corrupted).

I'm on the fence on an allowlist, but leaning towards pro.

[bstrie]

To ease any decision regarding error codes, here's a summary of the situation:

1. #[naked] is currently incompatible with #[inline] and #[track_caller].
2. Using #[naked] and #[track_caller] together produces an error with a code, whereas using #[naked] together with #[inline] produces an error with no code.
3. #[naked] needs to also reject #[target_feature] and #[test], which is implemented in this PR.
4. The first version of this PR made every combination of #[naked] and these four attributes into its own unique error code, making for four error codes total.
5. As of this writing, the current version of this PR consolidates #[inline], #[track_caller], and #[target_feature] into a single error code for "codegen attributes incompatible with #[naked]", while #[test] still has its own error code; two error codes total.
6. Other alternatives:
   • Have no error codes for any of these errors.
   • Have one broad error code for all errors of the form "you used two attributes that were incompatible" (it's possible that this already exists).
   • Have one error code solely for errors of the form "you used #[naked] with an incompatible attribute".

[bstrie]

How do we know that new feature authors have done this due diligence regarding naked functions?

It is assumed that the lang team will hold feature authors to this standard before allowing a feature to be stabilized. It's not impossible for things to slip through--accidents happen--but this is the acceptable risk that comes with stabilizing any feature.

Even worse, they might not fail but instead silently succeed with security implications (i.e. the stack is corrupted).

The threat in question involves a future attribute #[foo] that is silently incompatible with naked functions. The proposed mitigation is to prevent all but a hand-selected list of attributes on naked functions. However, for this threat to be realized the author of the naked function would have to manually add #[foo] to the naked function; every other existing user of naked functions cannot possibly be affected (at least not by any mechanism that the allowlist proposal would be capable of preventing). Thus, it would be impossible to realize such a threat merely by upgrading one's toolchain; you would have to first upgrade your toolchain, and then consciously adopt #[foo] into your codebase in order to be affected. Finally, #[foo] would have to be an attribute that affects codegen somehow, and (correct me if I'm wrong) it seems reasonable to assume that any security-minded user will have integration tests in place that would detect a failure on the PR that adds #[foo] to their naked functions. Thus IMO the benefit here is small.

Meanwhile, the cost is very large. I find it eminently reasonable to assume that a feature author that adds an attribute that affects codegen will perform their due diligence in remembering to deny its use on #[naked] (especially since they will be forced to update the reference page on codegen attributes, which will by then contain the documentation on attributes that may not be used with naked). I also find it reasonable to assume that feature authors that add an attribute that does not affect codegen will reasonably assume that they do not need to arbitrarily allow their attribute on naked functions; I cannot think of any other precedent for this in the language. What seems unreasonable to me would be forcing these latter authors to remember to manually allow their attribute to be used with naked functions; in practice I suspect they would simply not, and users of naked functions would be frustrated by the inability to do things that should obviously be allowed. Between the burden on feature authors and the burden on users who would be prevented from doing things for no reason (especially when considering that this policy would also prevent third-party proc macros from being used with naked functions), I feel that the cost outweighs the benefit, IMO.

[npmccallum]

How do we know that new feature authors have done this due diligence regarding naked functions?

It is assumed that the lang team will hold feature authors to this standard before allowing a feature to be stabilized. It's not impossible for things to slip through--accidents happen--but this is the acceptable risk that comes with stabilizing any feature.

Even worse, they might not fail but instead silently succeed with security implications (i.e. the stack is corrupted).

The threat in question involves a future attribute #[foo] that is silently incompatible with naked functions. The proposed mitigation is to prevent all but a hand-selected list of attributes on naked functions. However, for this threat to be realized the author of the naked function would have to manually add #[foo] to the naked function; every other existing user of naked functions cannot possibly be affected (at least not by any mechanism that the allowlist proposal would be capable of preventing). Thus, it would be impossible to realize such a threat merely by upgrading one's toolchain; you would have to first upgrade your toolchain, and then consciously adopt #[foo] into your codebase in order to be affected. Finally, #[foo] would have to be an attribute that affects codegen somehow,

I agree up until this point.

and (correct me if I'm wrong) it seems reasonable to assume that any security-minded user will have integration tests in place that would detect a failure on the PR that adds #[foo] to their naked functions. Thus IMO the benefit here is small.

This is a very bad assumption. All users are impacted by these problems regardless if they are security-minded or not. And the interactions between these attributes are non-obvious.

Meanwhile, the cost is very large.

Given that the number of users that consumes Rust features is exponentially larger than the number of Rust feature developers, the total cost is significantly smaller to the first group than to the second.

I find it eminently reasonable to assume that a feature author that adds an attribute that affects codegen will perform their due diligence in remembering to deny its use on #[naked] (especially since they will be forced to update the reference page on codegen attributes, which will by then contain the documentation on attributes that may not be used with naked).

I would hope so indeed. But when this fails, it fails spectacularly. Trying to bisect this will be very painful. OTOH, being able to do a git blame against an allowlist gives us the relevant commit(s) immediately.

I also find it reasonable to assume that feature authors that add an attribute that does not affect codegen will reasonably assume that they do not need to arbitrarily allow their attribute on naked functions; I cannot think of any other precedent for this in the language.

This is because for other features that are this dangerous, like asm, you don't just automatically get integration. For example, if you want to pass a new kind of value to asm you have to explicitly allow support for it in the asm functionality. No built-in hurdle like this exists for naked functions. The problem here is, I think, unique because we have something potentially very dangerous that exists precisely at the junction of an integration point (function attributes) where the number of combinations is high and difficult to reason about.

What seems unreasonable to me would be forcing these latter authors to remember to manually allow their attribute to be used with naked functions; in practice I suspect they would simply not, and users of naked functions would be frustrated by the inability to do things that should obviously be allowed.

But... doing this is safe. And fixing the oversight is trivial if there are no safety concerns. If you stabilize your feature and realize you've forgotten to add it to the allowlist, this should be a trivial patch that can be merged within the next release window. It isn't like you have to go back to the drawing board.

It seems to me like you're arguing that it is reasonable to expect feature developers to remember to consider all the subtle interactions between codegen attributes but not reasonable to expect the same feature developers to remember to add one item to an allowlist. The former is far more work than the latter.

Between the burden on feature authors and the burden on users who would be prevented from doing things for no reason (especially when considering that this policy would also prevent third-party proc macros from being used with naked functions), I feel that the cost outweighs the benefit, IMO.

It seems to me like you're optimizing for the burden of the feature authors rather than for the safety of the consumers of this feature.

[bstrie]

And the interactions between these attributes are non-obvious.

Certainly, which is part of the problem with concerning ourselves with general hypotheticals. It would be easier to discuss the likely threat of this scenario if we had a more concrete example.

Given that the number of users that consumes Rust features is exponentially larger than the number of Rust feature developers, the total cost is significantly smaller to the first group than to the second.

As mentioned, users looking to use attributes on their naked functions would also be bearing this cost. This is especially true for users of custom proc macros, since Rust cannot add external proc macros to the allowlist.

It seems to me like you're arguing that it is reasonable to expect feature developers to remember to consider all the subtle interactions between codegen attributes but not reasonable to expect the same feature developers to remember to add one item to an allowlist.

Rather, Rust's orthogonality usually allows feature developers to ignore most of Rust's features when implementing new features. The only existing features that need to be considered are those that have some likely overlap with the new feature; this quality makes new feature development much more tractable than it would be otherwise. This is why I suggest that it is likely that the author of a new codegen attribute (which will plausibly interact poorly with #[naked]) is likely to consider its interaction with #[naked], and why the author of a non-codegen attribute (which will not interact poorly with #[naked]) is not likely to consider its interaction with #[naked].

It seems to me like you're optimizing for the burden of the feature authors rather than for the safety of the consumers of this feature.

I suspect that we may have gotten sidetracked with the safety argument here. The reason that the attributes in this PR are being made incompatible with #[naked] is not for safety, but for future compatibility. The goal AIUI is to be able to leave the door open for a future version of Rust to have the option to lower a naked function to a semantically-equivalent global_asm! invocation. But even for safety, I consider the allowlist approach less than stellar; I am less concerned with future developments adding a #[foo] attribute incompatible with naked functions (which the allowlist approach would protect against) than I am with future developments making e.g. the #[doc] attribute incompatible with naked functions (which the allowlist approach would not protect against, and would result in vulnerabilities merely from updating one's toolchain).

Despite the amount of spilled ink here, I actually don't care all that much which approach is taken. My primary concern is with getting naked functions stabilized ASAP, and for the immediate future the allowlist approach and the blocklist approach will be semantically identical, and there's nothing stopping a future version of Rust from switching from one to the other. For maximum expediency, I will bring up to the lang team and implement whichever they currently think is the best approach.

[Aaron1011]

This is especially true for users of custom proc macros, since Rust cannot add external proc macros to the allowlist.

Custom proc macro attributes are all expanded (and removed) early in compilation. The only attributes remaining after macro expansion are built-in attributes, so the allowlist would only need to consider built-in attributes

[nikomatsakis]

I was reading the back and forth, @npmccallum and @bstrie, but I'm feeling a bit lost. @npmccallum, are you arguing against an allow-list? Do you have an alternative proposal?

[bstrie]

@nikomatsakis , @npmccallum is arguing that the compiler should have a list of attributes that are allowed to be present on #[naked], and should reject any attribute not on the list. That would differ from what this PR currently implements, which is a list of attributes that should be rejected from appearing with #[naked], where any unlisted attributes are allowed. The difference lies solely in the process through which future attributes are added to the language, and whether by-default they will be allowed with #[naked] unless otherwise specified, or whether by-default they will be forbidden with #[naked] unless otherwise specified. It's a procedural concern: the risk of someone forgetting to add an item to the allowlist when they should, versus the risk of someone forgetting to add an item to the blocklist when they should.

[bstrie]

On Zulip Josh indicated a preference for an allowlist, so I'll make that change and push a new version of this PR. In the meantime I'll message the relevant groups on Zulip regarding the error code question so that we can keep moving there as well.

[nikomatsakis]

I see. I think I am in favor of the "allow-list" approach that @npmccallum is advocating -- #[naked] functions seem special enough that it's ok to be more restrictive on them. We can always change to a "deny-list" in the future.

[bors]

☔ The latest upstream changes (presumably #94121) made this pull request unmergeable. Please resolve the merge conflicts.

[jackh726]

Status:

• Needs rebase
• Needs change to "allow list"
• Need clarification on how we want error codes to look (share error code with others, one error code, or multiple error codes)

[Dylan-DPC]

@bstrie any updates on this?

[bstrie]

@Dylan-DPC yep, I have an update to this PR coming imminently.

[estebank]

In the output, can we also point with secondary spans to the other non applicable attributes?

[estebank]

Can we point at #[naked] as well here, and maybe at the item name too?

[JohnCSimon]

@bstrie
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive