Skip to content
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

Unsafe Extern Blocks #3484

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Sep 7, 2023

Rendered


Continuation of #3439, very sorry for the mix up, but when I went to make #3477 I deleted my rfcs repo fork and re-forked. I had forgotten that there was an open PR at the time.

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Sep 7, 2023
Co-authored-by: Jacob Lifshay <programmerjake@gmail.com>
@kpreid
Copy link

kpreid commented Sep 14, 2023

Perhaps non-unsafe extern blocks should still be specified as potentially allowed — in case, in the future, there is some target ABI where (some) declarations can be statically verified to be sound at compile or link time? (For example, something like WebAssembly but which had its own, Rust-compatible, labeling of function safety, and checked function signature compatibility, so it is not possible to cause UB by mis-calling a function, nor to call an unsafe function by declaring it as safe.) That is, instead of saying “the grammar now requires unsafe”, say “unsafe is (still) syntactically optional, but semantically required in all current cases”.

This would minimize churn in the language definition if any such targets appear in the future. (But maybe that's an insignificant concern since support for previous editions is still required.)

@Lokathor
Copy link
Contributor Author

a future RFC can always relax required keywords in certain situations, it wouldn't even be an edition-break issue at that point.

but let's save that for a future RFC when we actually have a clear and specific fully working case.

@thomcc
Copy link
Member

thomcc commented Sep 15, 2023

One concern about making the functions inside the unsafe extern safe to call by default, is that it may make it easy to accidentally make some functions incorrectly safe when updating to a new edition.

For example, I could imagine people updating their projects, solving issues one by one, and resolving this issue by just changing an extern block to unsafe extern, failing to realize that they've (likely incorrectly) made all the function declarations in the block safe to use.

(Keep in mind that not every project uses cargo fix, especially if there are not a large number of problems)

@programmerjake
Copy link
Member

One concern about making the functions inside the unsafe extern safe to call by default, is that it may make it easy to accidentally make some functions incorrectly safe when updating to a new edition.

maybe have a weak keyword safe:

unsafe extern "C" {
    safe fn sqrtf(v: f32) -> f32;
    unsafe fn printf(s: *const c_char, ...);
}

@nikomatsakis
Copy link
Contributor

I'm frustrated by this RFC because the overall direction is obviously right to me, but the overloading of unsafe feels awfully subtle overall. I feel like we keep doubling down on the original decision to have unsafe play two roles (unsafe and trusted) and I am concerned that it is getting more and more confusing. I would feel much better if we proposed to move to a trusted keyword -- which is obviously a distinct RFC -- in conjunction with this.

@nikomatsakis
Copy link
Contributor

One specific concern I have:

  • What is the proportion of external functions that can correctly be safe?

i.e., we have to weigh the risk of people accidentally writing unsafe extern "C" versus how often they actually want those semantics. It's possible to use proc macros to derive around this.

@Lokathor
Copy link
Contributor Author

Lokathor commented Oct 4, 2023

Hey I'm all for a trusted keyword but gosh I don't think you can sell it at this point.

@pnkfelix
Copy link
Member

pnkfelix commented Oct 11, 2023

The T-lang team met to discuss this RFC today.

We bikeshed'ed a bit and probably covered the same ground that was already covered in this comment thread above (e.g. the suggestion above of a weak safe keyword).

In the end, we have the following proposal for how you might adjust this RFC to meet some broader goals that we identified.

Proposal:

(In this text, let the term "unadorned extern" mean "an extern that is not prefixed by unsafe", i.e. not unsafe extern { ... })

  • On all Rust editions, you can now write unsafe extern { ... }
  • On ≤2021 editions, we lint against unadorned extern { ... }, and emit a suggestion that one write unsafe extern { ... } instead; in ≥2024 editions, unsafe extern { ... } is required.
  • On all Rust editions, unsafe extern { ... } is extended to allow writing safe and unsafe in front of any static or fn item. (This is introducing a new contextual keyword: safe; the T-lang members present at the meeting supported this in part because we think such a keyword may have utility in other contexts in the future, and this is a natural place to employ it.)
    • We also suggest that on all Rust editions, the grammar of unadorned extern { ... } be extended to allow writing safe and unsafe (so that we can unify the parsing code for unsafe extern { ...} and unadorned extern { ... }), but it is nonetheless an error in the static semantics to have a safe or unsafe item within an unadorned extern { ... } block.
  • An item declared safe within an unsafe extern block is allowed to be used from safe code without an unsafe block surrounding the use of that item.
  • An item with declared unsafe within an unsafe extern block is only allowed to be used within an unsafe block (i.e. this re-implements the current Rust semantics, but now allowing for someone to include the redundant declaration so that it looks like other unsafe fn that occur outside of extern blocks).
  • An item without a safe nor unsafe marker within an extern block is implicitly unsafe (i.e. this is the current Rust 2015 semantics).

Here are the various motivations/assumptions that led us to the proposal above:

  • We agreed that unsafe extern { ... } itself is motivated. There is a proof obligation to check that the signature you use for foreign items actually matches the foreign items that end up being pulled in at link time.
  • We were concerned about people accidentally making items in the extern block safe to call by leaving out some kind of opt-in marking: this motivates the contextual safe keyword.
  • Once we had had the safe keyword as the way to opt into safety, the unsafe keyword on each unsafe-to-use item was no longer strictly necessary.
    • We agreed that it was useful to allow it, so that e.g. people can write code where it is apparent at the line of the declaration site that this is an unsafe fn. (But we want to encourage people to migrate to unsafe extern { ... }, even on old editions, so use of the new safe/unsafe declarations requires unsafe extern and is not legal within an unadorned extern.)
  • Allowing the unsafe keyword to be omitted from each item means that the migration path to 2024 edition is now truly trivial, even for developers migrating by hand: just replace extern { ... } with unsafe extern { ... }. Any other change beyond that is a matter of personal style.
  • We believe the proposal above gives us room to be stricter in future editions, after we have more experience with the feature.
    • For example, we might start requiring unsafe fn rather than just fn for an unsafe-to-call function in an unsafe extern { ... } block.

`extern` blocks are `unsafe` because if the declaration doesn't match the actual external function, or the actual external data, then it causes compile time Undefined Behavior (UB).

Once they are unsafely declared, a `safe` item can be used outside the `extern` block as if it were any other safe function or static value declared within rust.
The unsafe obligation of ensuring that the correct items are being linked to is performed by the crate making the declaration, not the crate using of that declaration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The unsafe obligation of ensuring that the correct items are being linked to is performed by the crate making the declaration, not the crate using of that declaration.
The unsafe obligation of ensuring that the correct items are being linked to is performed by the crate making the declaration, not the crate using that declaration.

@traviscross
Copy link
Contributor

traviscross commented Mar 25, 2024

@rustbot labels +I-lang-nominated +A-edition-2024

We met back on 2023-10-11 to discuss this RFC, and in that meeting, we hammered out a consensus for how to move forward on this as articulated by pnkfelix above. We've just been waiting for the RFC to be updated according to that consensus, and it now has been (with one caveat that I've noted in review -- that caveat has now been addressed).

Let's nominate to discuss potentially moving forward on this for Rust 2024.

@rustbot rustbot added A-edition-2024 Area: The 2024 edition I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. labels Mar 25, 2024
@scottmcm
Copy link
Member

Updates here look good! I this this is a good path forward and this version addresses the previous worries about changing something to be the opposite over an edition boundary.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 27, 2024

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Mar 27, 2024

* On editions >= 2024, you *must* write all `extern` blocks as `unsafe extern`.
* On editions < 2024, you *may* write `unsafe extern`, or you can write an `extern` block without the `unsafe` keyword. Writing an `extern` block without the `unsafe` keyword is provided for compatibility only, and will generate a warning.
* `unsafe extern` interacts with the `unsafe_code` lint, and a `deny` or `forbid` with that lint will deny or forbid the unsafe external block.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will an extern (no unsafe, edition < 2024) block trip the lint? I'd argue it's as unsafe (if not more dangerous) with unsafe token...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for compatibility reasons I'd expect to warn at most, even if you use deny/forbid unsafe_code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intent is that you get a forward compatibility warning if you write extern without unsafe.

# Drawbacks
[drawbacks]: #drawbacks

* It is very unfortunate to have to essentially reverse the status quo.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to mention a drawback of a previous version? (where no qualifier in unsafe extern {} meant "safe")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, yeah, maybe, i dunno, i mean we're still swapping around how things work, maybe not "reversing" maybe some other verb.

I don't know how precisely to describe the motion happening, but also I don't wanna say that "there's no drawbacks at all!". it's definitely some motion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. How about something like this?

  • It is very unfortunate to change the syntax of extern blocks and require changes to all of them.
    • Hopefully, allowing people to safely call some foreign functions will make up for the churn caused by this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree "reverse" is confusing, I don't know what is being reverse here. 👍 for something like Waffle's proposal. Basically the drawback is "churn".

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Apr 3, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 3, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Apr 3, 2024
@tmandry
Copy link
Member

tmandry commented Apr 3, 2024

Excited about this RFC, particularly the ability to declare extern fn's and static's as safe, but also because it puts the "burden of proof" where it belongs, on the declaration of the extern.

@rfcbot reviewed

@WaffleLapkin
Copy link
Member

I think the consensus in the T-lang meeting today in response to @RalfJung's concern (hm, re-reading the comment now I'm not sure it's actually a concern, it actually looks more just a response to the other comment...) was that even if the "merge definitions" is an LLVM/rustc bug we still want to make extern {} unsafe to define. At the very least to push the burden of cheking the signature against the definition onto the one defining the foreign function. It doesn't seem reasonable to ask callers of foreign functions to prove that the functions are defined correctly -- callers should only need to check that they satisfy the function's safety requirements.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this on 2024-04-03 and people were happy to see this happen. Thanks to @Lokathor for updating the RFC so this can be included in the edition. It's now in FCP, so let's unnominate.

@traviscross traviscross removed the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Apr 5, 2024
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

I don't love the safe keyword, but I concur with the reasoning @pnkfelix laid out here that got us there. I think we should consider a trusted keyword for Rust 2027.

@nikomatsakis
Copy link
Contributor

@rfcbot concern choice-of-keyword

I guess, to @rust-lang/lang, I'd like to ask:

Should we opt for trusted instead as the contextual keyword, for better future compat? Or do we think safe { ... } is the right word? I tend to think "trusted" is better at capturing the meaning. Safe implies "compiler proved it", to me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Apr 5, 2024
@RalfJung
Copy link
Member

RalfJung commented Apr 5, 2024

Both the unsafe in unsafe extern and the safe in safe fn are places that discharge safety obligations, right? The first requires proving that the signature is correct (i.e. there is no UB due to wrong ABI, but there may still be UB due to other issues), the second requires proving that the code on the other side is actually sound for the given Rust types.

In contrast, the unsafe fn is declaring a safety obligation.

It seems unfortunate that we are introducing a new keyword to distinguish "create obligation" from "discharge obligation" but the same RFC also uses unsafe to mean both "create obligation" and "discharge obligation".

@traviscross
Copy link
Contributor

@rustbot labels +I-lang-nominated

Let's nominate this to discuss the concern that nikomatsakis has raised above.

@rustbot rustbot added the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Apr 10, 2024
@Lokathor
Copy link
Contributor Author

I'm not against a discussion, but I'll note that "a safe function" is already what the Rust documentation/community calls a function that doesn't require an unsafe block to call, and that has absolutely nothing to do with the compiler "proving" anything. Countless functions in Rust are safe functions that do unsafe things internally using unsafe blocks, because they know more than the compiler.

@kennytm
Copy link
Member

kennytm commented Apr 10, 2024

OK sorry maybe I'm confused about the current proposal. In the RFC's original version there was no safe contextual keyword so adding unsafe to the extern block makes sense. But now,

unsafe extern "C" {
    unsafe fn a(); // yes it's unsafe
    fn b(); // unsafe by default, same as current situation, maybe linted to require unsafe/safe annotation
    bikeshed_safe fn c(); // explicitly marked safe

    unsafe static X: [u8; 1024]; // unsafe
    static Y: [u8; 1024]; //unsafe by default
    bikeshed_trusted static Z: [u8; 1024]; // explicitly safe
}

That unsafe keyword before extern seems totally unnecessary??? I think you could even introduce the safe contextual keyword without needing a new edition

extern "C" {  // <-- no unsafe keyword, still triggers unsafe_code lint
    unsafe fn a();
    fn b();
    bikeshed_harmless fn c();

    unsafe static X: [u8; 1024];
    static Y: [u8; 1024];
    bikeshed_good_boi static Z: [u8; 1024];
}

@Lokathor
Copy link
Contributor Author

Lokathor commented Apr 10, 2024

My understanding is that the difference is primarily because of the history. I absolutely don't think it's good design on its own.

EDIT: and some people just want the ascii string unsafe to just literally appear in a source code file near unsafe stuff, even if it doesn't actually improve safety at all, which is why we separately have the unsafe attributes proposal.

@WaffleLapkin
Copy link
Member

@kennytm I think the point of the unsafe there is two fold:

  1. The RFC as-is actually says that unsafe extern { fn implicit(); } is an error, the unsafe|safe qualifier on functions is required with the new style unsafe extern
  2. unsafe highlights that it's the responsibility of the definition to make sure the functions have correct ABI; even for unsafe functions it's unreasonable to expect the caller to check that

@kennytm
Copy link
Member

kennytm commented Apr 10, 2024

@WaffleLapkin

  1. OK. But that does not preclude just extern { fn something(); } being an error in 2024 and requires extern { unsafe fn something(); }
  2. Well I certainly don't get this intention from just requiring unsafe to prefix every extern block. All I feel was:
extern "C" {
    unsafe type FILE; // <-- btw how does this RFC work with rust-lang/rust#43467
    unsafe static stderr: *mut FILE;
    unsafe fn fopen(name: *const c_char, mode: *const c_char) -> *mut FILE;
    unsafe fn fclose(stream: *mut FILE) -> c_int;
    // SAFETY: just a pure math function
    safe fn sinf(x: f32) -> f32;
    // SAFETY: just a pure math function
    safe fn atan2f(y: f32, x: f32) -> f32;
}
I can understand that
unsafe extern "C" {
    type FILE;
    static stderr: *mut FILE;
    fn fopen(name: *const c_char, mode: *const c_char) -> *mut FILE;
    fn fclose(stream: *mut FILE) -> c_int;
}
// SAFETY: just pure math functions
safe extern "C" {
    fn sinf(x: f32) -> f32;
    fn atan2f(y: f32, x: f32) -> f32;
}
I can understand that
unsafe extern "C" {
    unsafe type FILE;
    unsafe static stderr: *mut FILE;
    unsafe fn fopen(name: *const c_char, mode: *const c_char) -> *mut FILE;
    unsafe fn fclose(stream: *mut FILE) -> c_int;
    // SAFETY: just a pure math function
    safe fn sinf(x: f32) -> f32;
    // SAFETY: just a pure math function
    safe fn atan2f(y: f32, x: f32) -> f32;
}
The Rust language is just trying to semantic satiate the word "unsafe" 😅

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

The grammar of the langauge is updated so that:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The grammar of the langauge is updated so that:
The grammar of the language is updated so that:

@nikomatsakis
Copy link
Contributor

@rfcbot resolve choice-of-keyword

We discussed this in the @rust-lang/lang meeting and @pnkfelix convinced me that the contextual keyword safe is a reasonable choice here -- in particular, the point where the proof obligation is incurred is the extern block (which is, effectively, a trusted extern { ... }).

The question I was posing, effectively, is "if we DID distinguish trusted vs unsafe, wouldn't we be using the keyword trusted here? should we just do that?" But what @pnkfelix convinced me of is that, in the fullness of time, what we would really do is...

trusted extern {
    /*safe*/ fn foo(); 
}

but we are including an explicit safe keyword because flipping the default in one step is too strong.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Apr 24, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 24, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@madsmtm
Copy link

madsmtm commented Apr 24, 2024

trusted extern {
    /*safe*/ fn foo(); 
}

I agree that that's where we want to end up.

But I'll still argue that you do not need to worry about the safe keyword, because you do not need to introduce safe external functions at all! Simply prohibit them to begin with, and once we deem that the potential for error isn't that great any more because most people have migrated, loosen the restriction.

// Edition 2015-2021
extern "C" {
    fn a_safe_function(); // `unsafe`, even though that function itself is safe
    fn an_unsafe_function();
}

// Edition 2024
unsafe extern "C" {
    unsafe fn a_safe_function(); // Still forced to be `unsafe`, if we try to remove `unsafe` we get a compilation error
    unsafe fn an_unsafe_function();
}

// Edition 2024+N, when enough people have migrated to `unsafe extern "C" { ... }`
unsafe extern "C" {
    fn a_safe_function(); // Allow marking as safe by just not including `unsafe`
    unsafe fn an_unsafe_function();
}

@kennytm
Copy link
Member

kennytm commented Apr 24, 2024

sorry absolutely -1 if you want us to pepper unsafe in every FFI declaration/extern block, and sleep on it for N (≥3 presumably) years to finally see the benefit (ability to declare a safe FFI item).


what we would really do is...

trusted extern {
    /*safe*/ fn foo(); 
}

so this RFC should be changed to say trusted extern rather than unsafe extern? It makes no sense to have an intermediate edition making it a 2-step evolution externunsafe externtrusted extern.

(and I'm still not convinced that the keyword extern alone is insufficient)

@pnkfelix
Copy link
Member

@kennytm wrote:

so this RFC should be changed to say trusted extern rather than unsafe extern?

no, that is not our intention for this RFC.

I took it as more of a general statement about where the language might try to go, in the very long term, but only as part of a more holistic revisiting of our story regarding of how one writes introduces and discharges proof obligations.

@nikomatsakis
Copy link
Contributor

Correct @pnkfelix --

I took it as more of a general statement about where the language might try to go, in the very long term, but only as part of a more holistic revisiting of our story regarding of how one writes introduces and discharges proof obligations.

...I was not attempting to suggest that we adopt the trusted keyword in that position (or any position), but simply safe... if we did (in some future RFC) choose to adopt trusted for uses of unsafe that introduce safety conditions, this is what it would look like.

@traviscross traviscross removed the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
Status: Needs help: Design
Development

Successfully merging this pull request may close these issues.

None yet