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

Tracking issue for safe_extern_statics compatibility lint #36247

Closed
2 of 3 tasks
petrochenkov opened this issue Sep 3, 2016 · 14 comments · Fixed by #65785
Closed
2 of 3 tasks

Tracking issue for safe_extern_statics compatibility lint #36247

petrochenkov opened this issue Sep 3, 2016 · 14 comments · Fixed by #65785
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-future-incompatibility Category: Future-incompatibility lints T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 3, 2016

What is this lint about

Consider the following code:

extern {
    static S: T;
}

S here is a static variable accessible to Rust but defined and initialized elsewhere.
External static variables cannot be accessed safely from Rust code for several reasons:

  • S can contain an arbitrary bit pattern not necessarily being a valid value of type T.
  • even if all bit patterns are valid values of T, S is still not controlled by Rust ownership system and can be accessed from other threads potentially creating data races.

See #35112 for more details as well as examples of how this can lead to crashes.

Previously safe accesses to extern statics were erroneously permitted outside of unsafe blocks. #36173 fixed this oversight.

How to fix this warning/error

Surround accesses to extern statics with unsafe blocks and make sure these accesses are actually valid.

Current status

@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Sep 8, 2016
bors added a commit that referenced this issue Sep 8, 2016
Issue deprecation warnings for safe accesses to extern statics

Fixes #35112
cc #36247
@cbiffle
Copy link
Contributor

cbiffle commented Sep 19, 2016

In the case where I'm using an extern struct to model memory-mapped peripheral registers, and can design the API carefully to be safe, is there any way to disable this warning on a per-item basis? It seems that even naming the extern triggers the warning, so I cannot carefully implement methods on it to do safe things.

Example:

// GPIOD is an extern struct so that I can place it on memory-mapped
// registers and keep Rust from trying to initialize it.  Its only public
// operations are set_pin and clear_pin, both of which use unsafe code
// but are intended as safe APIs.  But safe code cannot do this:
stm32f4::gpio::GPIOD.set_pin(12);

One option I've considered is to always allocate two statics, one extern, and one a simple proxy containing unsafe-marked calls to its methods. I don't like this approach, but it will unstick me for now.

@petrochenkov
Copy link
Contributor Author

@cbiffle
I think the proper solution is ability to mark unsafe-by-default items as safe, similarly to how safe-by-default pure Rust functions can be marked as unsafe.
In addition to foreign statics this can be applied to foreign functions and union fields.

@cbiffle
Copy link
Contributor

cbiffle commented Sep 19, 2016

@petrochenkov, yes, that is basically what I'm hoping for. Just as I can use unsafe {} to tell the compiler "I know this is composed of unsafe operations, but the result is safe, really," I'd like a way of indicating something similar at an item or type level.

Does it already exist? (I'm trying to get code working today.)

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Sep 19, 2016

Does it already exist? (I'm trying to get code working today.)

No. Even if someone submits an RFC right now, it'll be months until it's accepted (if it's accepted), implemented and hits stable.
The best advice I can give is to write a convenience macro for the proxy workaround you are using now.

A worse advice is to use #![allow(safe_extern_statics)].
Given that safe access to foreign statics was permitted for 1.5 years since 1.0, it won't probably become an error in the next 1-2 years and until some replacement is available.

Ms2ger added a commit to servo/rust-mozjs that referenced this issue Sep 22, 2016
This fixes safe_extern_statics warnings; see
<rust-lang/rust#36247>.
bors-servo pushed a commit to servo/rust-mozjs that referenced this issue Sep 22, 2016
Wrap uses of extern statics in unsafe blocks.

This fixes safe_extern_statics warnings; see
<rust-lang/rust#36247>.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/307)
<!-- Reviewable:end -->
SSheldon added a commit to SSheldon/rust-dispatch that referenced this issue Jan 8, 2017
@nikomatsakis
Copy link
Contributor

cc @rust-lang/lang

So I had a long conversation with @wycats about this. He convinced me that simply declaring all accesses to statics as unsafe is not an optimal solution. Specifically, it is common in C code to have statics that are constant (never mutated), and right now there is no way to "wrap" those statics with a safe interface.

To see what I mean, consider wrapping a C function fn noop() that just does nothing. Clearly, this function is always safe. But of course rust doesn't know what it does, so we make it unsafe to call. But as the wrapper of this library, I can write a Rust function that is entirely equivalent:

mod sys {
    extern { fn noop(); }
}

fn noop() {
    // I assert that this is safe to call at any time
    unsafe { noop(); }
}

This is annoying, but mostly works. On the other hand, if I have a static constant, I am stuck. An example is that Ruby has a constant for a nil value, called Q_nil or something like that. The only wrapper I can write is not a constant, but rather a function that returns the value of the constant (or an &'static pointer to it). This seems distinctly more painful to use, which works against the ergonomic wrapping of C libraries.

I see two possible solutions here:

Define some way for FFI declarations in general to declare that the value is, in fact, safe. this could also be used to wrap noop. I'm not sure what this syntax is. Usually we use an unsafe block for this, but here we have no "code". It's too bad we never adopted the trust keyword, perhaps, but oh well.

Permit const in FFI blocks. This would be a way to assert that the static is indeed a constant (e.g., extern { const Q_nil: u32; }. This seems like a nice thing to make easy. My one hesitation is that constants, in Rust, have some kind of funny-ish semantics, e.g., they don't have a defined address, that is probably not what we want here. We might also write extern { static const Q_nil: u32 }, but that is kind of confusing too.

Regardles, we should probably handle this case more nicely I think!

@nikomatsakis
Copy link
Contributor

Er, I'm actually just repeating what @cbiffle wrote, I suppose. =) Sorry, missed those comments.

@petrochenkov
Copy link
Contributor Author

@nikomatsakis
You'll be surprised, I submitted RFC 1901 "Reverting default unsafety" inspired by @cbiffle's comments just two days ago.
(People don't seem to like it so far 😄 )

@eddyb
Copy link
Member

eddyb commented Feb 17, 2017

Are we disallowing const FOO: &'static Foo = unsafe { &C_FOO }; right now?
We could allow it and then disallow any operations which may cause a read (which I think we do correctly right now?) and also disallow passing it to const fn (which I believe was the big issue with it).
Or we could "just" evaluate these with miri and allow reads of statics, which would work fine for Rust ones (unless there's a cycle which on-demand would catch), while FFI ones would error on read/write.
cc @solson

@nikomatsakis
Copy link
Contributor

@petrochenkov I am indeed =) I'm always like 2 steps behind on the RFC repo...working on catching up though!

@nikomatsakis
Copy link
Contributor

@eddyb indeed that's a nifty thought, although it means users will have to write *FOO

@joshtriplett
Copy link
Member

@nikomatsakis While const in Rust does allow Rust to omit any corresponding runtime storage or symbol, we could define extern const (or for that matter extern "C" const) as always referencing a symbol, FFI-style.

@retep998
Copy link
Member

Const has some additional semantics over static, such as the ability to use it in patterns. Would extern consts also gain these semantics? (I'm opposed either way)

@petrochenkov petrochenkov added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Feb 19, 2017
@nikomatsakis
Copy link
Contributor

@retep998

I think extern const is the wrong syntax, since this is not a "Rust const", but rather a static variable that the FFI author is promising will not be re-assigned. I do feel this is a very useful thing to be able to express, but i'm not sure what's the best way to do it.

fitzgen pushed a commit to fitzgen/mozjs that referenced this issue May 11, 2017
This fixes safe_extern_statics warnings; see
<rust-lang/rust#36247>.
@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 22, 2017
ids1024 added a commit to ids1024/ralloc that referenced this issue Nov 8, 2017
- New allocator API
- Remove the "allocator" feature, which should be unnecessary due to how
the new allocator API works
- NonZero no longer implements Deref (rust-lang/rust#41064)
- NonZero::new() returns an Option; use NonZero::new_unchecked()
- Thread locals are no longer 'static (rust-lang/rust#43746)
- Changes to feature flags
- Use unsafe to access extern static (rust-lang/rust#36247)
jdm pushed a commit to servo/mozjs that referenced this issue Dec 18, 2017
This fixes safe_extern_statics warnings; see
<rust-lang/rust#36247>.
@Centril
Copy link
Contributor

Centril commented Nov 11, 2018

@joshtriplett what's to be done here?

@Centril Centril added C-future-incompatibility Category: Future-incompatibility lints and removed C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Jan 13, 2019
bors added a commit that referenced this issue Aug 3, 2019
Transition some C-future-compatibility lints to {ERROR, DENY}

Closes #40107 (ERROR).
Closes #39207 (ERROR).
Closes #37872 (ERROR).
Closes #36887 (ERROR).
Closes #36247 (ERROR.
Closes #42238 (ERROR).
Transitions #59014 (DENY).
Transitions #57571 (DENY).
Closes #60210 (ERROR).
Transitions #35203 (DENY).

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this issue Nov 8, 2019
Transition future compat lints to {ERROR, DENY} - Take 2

Follow up to rust-lang#63247 implementing rust-lang#63247 (comment).

- `legacy_ctor_visibility` (ERROR) -- closes rust-lang#39207
- `legacy_directory_ownership` (ERROR) -- closes rust-lang#37872
- `safe_extern_static` (ERROR) -- closes rust-lang#36247
- `parenthesized_params_in_types_and_modules` (ERROR) -- closes rust-lang#42238
- `duplicate_macro_exports` (ERROR)
- `nested_impl_trait` (ERROR) -- closes rust-lang#59014
- `ill_formed_attribute_input` (DENY) -- transitions rust-lang#57571
- `patterns_in_fns_without_body` (DENY) -- transitions rust-lang#35203

r? @varkor
cc @petrochenkov
@bors bors closed this as completed in 7ab50e4 Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-future-incompatibility Category: Future-incompatibility lints T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
8 participants