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

Merged
merged 29 commits into from
May 20, 2024
Merged

Conversation

Lokathor
Copy link
Contributor

@Lokathor Lokathor commented Sep 7, 2023

Rendered

Tracking:


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.

@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
@RalfJung
Copy link
Member

RalfJung commented May 7, 2024

but because unsafe unsafe and unsafe safe is even sillier it becomes

I don't agree with that framing.
I would say: because trusted(extern_signature) must appear on every single item, we can optimize the bureaucratic ideal world to reduce redundancy:

trusted(extern_signature) extern {
    unsafe(deref_pointer) fn fopen(pathname: *const c_str, mode: *const c_str) -> *mut FILE;
    safe fn malloc(size: usize) -> *mut c_void;
}

Then we do the "remove the reasons" and "conflate trusted and unsafe" transformations and voila -- we arrived at the RFC proposal.

You proposal removes every trace of trusted(extern_signature), which is not how we treat unsafe anywhere else in the language. For instance, one could argue that get_mut_unchecked already clearly indicates that something unsafe is going on, so surely we can absorb the trusted(index_in_bounds) into the call? But no, that is emphatically not how Rust works.

@tmccombs
Copy link

tmccombs commented May 7, 2024

So in the 2024 edition from what I can tell, you wouldn't be able to use extern without unsafe. So this is efectively changing the extern keyword to a new unsafe extern keyword that, unusually, contains whitespace.

one could argue that get_mut_unchecked already clearly indicates that something unsafe is going on, so surely we can absorb the trusted(index_in_bounds) into the call?

There is a pretty big difference between a language keyword, and a substring of a function name.

@kennytm
Copy link
Member

kennytm commented May 7, 2024

@tmccombs I'm pretty sure extern crate and extern fn (function definitions) are unaffected. unsafe extern are still 2 keywords.

@programmerjake
Copy link
Member

programmerjake commented May 7, 2024

I think unsafe should only be required on extern if it contains any fn/static declarations (or any other declarations that link to symbols in an unchecked manner, if we get any more kinds).
I know I used:

#[doc = include_str!("../README.md")]
extern {} // there's no reason this should be unsafe

and I expect extern type declarations to also not need unsafe.

@kennytm
Copy link
Member

kennytm commented May 7, 2024

@RalfJung #3484 (comment)

You proposal removes every trace of trusted(extern_signature),

I'm saying extern { implied trusted(extern_signature)

which is not how we treat unsafe anywhere else in the language.

#![warn(unsafe_code)]
std::arch::global_asm! { "" } 
// ^ using this macro is unsafe even though it does not need an `unsafe` block

For instance, one could argue that get_mut_unchecked already clearly indicates that something unsafe is going on, so surely we can absorb the trusted(index_in_bounds) into the call? But no, that is emphatically not how Rust works.

Strawman. For get_mut_unchecked I may want to propagate the unsafe effect rather than discharging it. There is no such option for an extern block.

Additionally, existing (≤2021) extern {} block did not require unsafe, unlike get_mut_unchecked here which always required unsafe {}. Migration matters.

@RalfJung
Copy link
Member

RalfJung commented May 8, 2024

I'm saying extern { implied trusted(extern_signature)

I understand. And I'm saying that's bad as one now has to know a list of things in Rust that implicitly introduce unsafety, as opposed to the simple rule that grep unsafe is sufficient.

I don't think my get_mut_unchecked example is a strawman. For me, that feels like the same thing -- implicit vs explicit unsafety. You seem to make a qualitative difference there, but to me the difference is just quantitative.

std::arch::global_asm! { "" }

That should be rectified, like we did with unsafe attributes.

No reason to repeat past mistakes.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in the lang triage meeting today, and we confirmed that:

  1. Our key motivation for this change is to make clear that the obligation for proving the correctness of the signatures within an extern block falls on the author of the extern block, not on the caller (or, in general, the user) of an item within an extern block.
  2. We're happy with this motivation even if Rust could be fixed such that writing an extern block with incorrect signatures would not potentially cause the resulting program to exhibit undefined behavior even if those items were never used by Rust code.

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

nikomatsakis commented May 10, 2024 via email

The FCP for RFC 3484 has completed with a disposition to merge.  Let's
prepare to merge it.
We had earlier made clarifying edits to many sections, but not to all
of them.  Let's clarify some text in these remaining sections.
@traviscross traviscross merged commit c81432d into rust-lang:master May 20, 2024
@traviscross
Copy link
Contributor

The lang team has accepted this RFC, and we've now merged it.

Thanks to @Lokathor for pushing this forward, and thanks to all those who contributed helpful feedback.

For further updates on this work, please follow the tracking issue:

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2024
…li-obk

Unsafe extern blocks

This implements RFC 3484.

Tracking issue rust-lang#123743 and RFC rust-lang/rfcs#3484

This is better reviewed commit by commit.
github-actions bot pushed a commit to ytmimi/rustfmt that referenced this pull request Jun 7, 2024
Unsafe extern blocks

This implements RFC 3484.

Tracking issue #123743 and RFC rust-lang/rfcs#3484

This is better reviewed commit by commit.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 7, 2024
Unsafe extern blocks

This implements RFC 3484.

Tracking issue #123743 and RFC rust-lang/rfcs#3484

This is better reviewed commit by commit.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 13, 2024
Unsafe extern blocks

This implements RFC 3484.

Tracking issue #123743 and RFC rust-lang/rfcs#3484

This is better reviewed commit by commit.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
Unsafe extern blocks

This implements RFC 3484.

Tracking issue #123743 and RFC rust-lang/rfcs#3484

This is better reviewed commit by commit.
tgross35 added a commit to tgross35/rust that referenced this pull request Aug 3, 2024
…-blocks, r=compiler-errors

Stabilize unsafe extern blocks (RFC 3484)

# Stabilization report

## Summary

This is a tracking issue for the RFC 3484: Unsafe Extern Blocks

We are stabilizing `#![feature(unsafe_extern_blocks)]`, as described in [Unsafe Extern Blocks RFC 3484](rust-lang/rfcs#3484). This feature makes explicit that declaring an extern block is unsafe. Starting in Rust 2024, all extern blocks must be marked as unsafe. In all editions, items within unsafe extern blocks may be marked as safe to use.

RFC: rust-lang/rfcs#3484
Tracking issue: rust-lang#123743

## What is stabilized

### Summary of stabilization

We now need extern blocks to be marked as unsafe and items inside can also have safety modifiers (unsafe or safe), by default items with no modifiers are unsafe to offer easy migration without surprising results.

```rust
unsafe extern {
    // sqrt (from libm) may be called with any `f64`
    pub safe fn sqrt(x: f64) -> f64;

    // strlen (from libc) requires a valid pointer,
    // so we mark it as being an unsafe fn
    pub unsafe fn strlen(p: *const c_char) -> usize;

    // this function doesn't say safe or unsafe, so it defaults to unsafe
    pub fn free(p: *mut core::ffi::c_void);

    pub safe static IMPORTANT_BYTES: [u8; 256];

    pub safe static LINES: SyncUnsafeCell<i32>;
}
```

## Tests

The relevant tests are in `tests/ui/rust-2024/unsafe-extern-blocks`.

## History

- rust-lang#124482
- rust-lang#124455
- rust-lang#125077
- rust-lang#125522
- rust-lang#126738
- rust-lang#126749
- rust-lang#126755
- rust-lang#126757
- rust-lang#126758
- rust-lang#126756
- rust-lang#126973
- rust-lang#127535
- rust-lang/rustfmt#6204

## Unresolved questions

I am not aware of any unresolved questions.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 3, 2024
…-blocks, r=compiler-errors

Stabilize unsafe extern blocks (RFC 3484)

# Stabilization report

## Summary

This is a tracking issue for the RFC 3484: Unsafe Extern Blocks

We are stabilizing `#![feature(unsafe_extern_blocks)]`, as described in [Unsafe Extern Blocks RFC 3484](rust-lang/rfcs#3484). This feature makes explicit that declaring an extern block is unsafe. Starting in Rust 2024, all extern blocks must be marked as unsafe. In all editions, items within unsafe extern blocks may be marked as safe to use.

RFC: rust-lang/rfcs#3484
Tracking issue: rust-lang#123743

## What is stabilized

### Summary of stabilization

We now need extern blocks to be marked as unsafe and items inside can also have safety modifiers (unsafe or safe), by default items with no modifiers are unsafe to offer easy migration without surprising results.

```rust
unsafe extern {
    // sqrt (from libm) may be called with any `f64`
    pub safe fn sqrt(x: f64) -> f64;

    // strlen (from libc) requires a valid pointer,
    // so we mark it as being an unsafe fn
    pub unsafe fn strlen(p: *const c_char) -> usize;

    // this function doesn't say safe or unsafe, so it defaults to unsafe
    pub fn free(p: *mut core::ffi::c_void);

    pub safe static IMPORTANT_BYTES: [u8; 256];

    pub safe static LINES: SyncUnsafeCell<i32>;
}
```

## Tests

The relevant tests are in `tests/ui/rust-2024/unsafe-extern-blocks`.

## History

- rust-lang#124482
- rust-lang#124455
- rust-lang#125077
- rust-lang#125522
- rust-lang#126738
- rust-lang#126749
- rust-lang#126755
- rust-lang#126757
- rust-lang#126758
- rust-lang#126756
- rust-lang#126973
- rust-lang#127535
- rust-lang/rustfmt#6204

## Unresolved questions

I am not aware of any unresolved questions.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2024
Rollup merge of rust-lang#127921 - spastorino:stabilize-unsafe-extern-blocks, r=compiler-errors

Stabilize unsafe extern blocks (RFC 3484)

# Stabilization report

## Summary

This is a tracking issue for the RFC 3484: Unsafe Extern Blocks

We are stabilizing `#![feature(unsafe_extern_blocks)]`, as described in [Unsafe Extern Blocks RFC 3484](rust-lang/rfcs#3484). This feature makes explicit that declaring an extern block is unsafe. Starting in Rust 2024, all extern blocks must be marked as unsafe. In all editions, items within unsafe extern blocks may be marked as safe to use.

RFC: rust-lang/rfcs#3484
Tracking issue: rust-lang#123743

## What is stabilized

### Summary of stabilization

We now need extern blocks to be marked as unsafe and items inside can also have safety modifiers (unsafe or safe), by default items with no modifiers are unsafe to offer easy migration without surprising results.

```rust
unsafe extern {
    // sqrt (from libm) may be called with any `f64`
    pub safe fn sqrt(x: f64) -> f64;

    // strlen (from libc) requires a valid pointer,
    // so we mark it as being an unsafe fn
    pub unsafe fn strlen(p: *const c_char) -> usize;

    // this function doesn't say safe or unsafe, so it defaults to unsafe
    pub fn free(p: *mut core::ffi::c_void);

    pub safe static IMPORTANT_BYTES: [u8; 256];

    pub safe static LINES: SyncUnsafeCell<i32>;
}
```

## Tests

The relevant tests are in `tests/ui/rust-2024/unsafe-extern-blocks`.

## History

- rust-lang#124482
- rust-lang#124455
- rust-lang#125077
- rust-lang#125522
- rust-lang#126738
- rust-lang#126749
- rust-lang#126755
- rust-lang#126757
- rust-lang#126758
- rust-lang#126756
- rust-lang#126973
- rust-lang#127535
- rust-lang/rustfmt#6204

## Unresolved questions

I am not aware of any unresolved questions.
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. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.