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

Extern statics can be accessed in safe Rust #35112

Closed
bjorn3 opened this issue Jul 29, 2016 · 11 comments
Closed

Extern statics can be accessed in safe Rust #35112

bjorn3 opened this issue Jul 29, 2016 · 11 comments
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@bjorn3
Copy link
Member

bjorn3 commented Jul 29, 2016

This code crashes:

#[no_mangle] pub static X: usize = 0;
mod test {
    extern {
        pub static X: &'static u32;
    }
}
fn main() {
     println!("{}", X);
     println!("{}", *test::X);
}

@ubsan wrote this code on irc.
https://botbot.me/mozilla/rust-internals/2016-07-28/?msg=70411962&page=5

@strega-nil
Copy link
Contributor

strega-nil commented Jul 29, 2016

I believe the solution is to require unsafe when accessing statics that are extern, or that we do unsafe extern when writing an extern block that includes statics.

@Stebalien
Copy link
Contributor

Stebalien commented Jul 29, 2016

Why do we even allow extern references? Shouldn't this be pub static X: *const u32? Nevermind, it's useful to be able to talk about external values of arbitrary types.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 29, 2016

It's pretty horrifying that unsafe isn't required to access foreign statics.
Given that you can write a foreign static with any Rust type with any invariants (improper_ctypes is not a hard rule)

extern "C" {
    static STATIC: Vec<u8>;
}

fn main() {
    println!("{}", STATIC[0]);
}

and the C side can contain arbitrary bytes under the name STATIC.

@steveklabnik steveklabnik added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 29, 2016
@steveklabnik
Copy link
Member

nominating due to unsoundness

@strega-nil
Copy link
Contributor

strega-nil commented Jul 29, 2016

@petrochenkov technically, the Rust side can also contain arbitrary bytes under the name STATIC. Or the C++ side. Or really any language.

@retep998
Copy link
Member

I'd be okay with extern statics requiring unsafe to access. Would probably need a very long deprecation/warning period however.

@strega-nil
Copy link
Contributor

@retep998 it's unsound, therefore, it doesn't need that long of a deprecation period, in my opinion. Maybe two cycles.

@hanna-kruppe
Copy link
Contributor

Since this would presumably go through a forward compatibility lint, it could be made deny-by-default soon-ish. A hard error should perhaps take longer — just because it's unsound doesn't mean it won't affect many crates.

@strega-nil
Copy link
Contributor

@rkruppe makes sense.

@retep998
Copy link
Member

Probably one Rust release warn by default, and then at least one deny by default and keeping it deny by default for a while until crater says it is mostly okay.

@nikomatsakis
Copy link
Contributor

Agreed, it seems very clear that accessing an extern static should be unsafe -- just as calling an extern fn is unsafe. Just a plain oversight. There are many ways for this to go wrong:

  • the C code could mutate the static
  • the type might not match what the C type is
  • etc

So we should add a forwards-compatibility lint and make it unsafe to access an extern static. I agree we'll want to have a (very) long deprecation period.

@arielb1 arielb1 changed the title Crash in safe rust Extern statics can be accessed in safe Rust Aug 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
tilde-engineering pushed a commit to tildeio/helix that referenced this issue Feb 16, 2017
tilde-engineering pushed a commit to tildeio/helix that referenced this issue Feb 16, 2017
Fix extern static warning

See rust-lang/rust#35112
tilde-engineering pushed a commit to tildeio/helix that referenced this issue Feb 17, 2017
Fix extern static warning

See rust-lang/rust#35112
wagenet pushed a commit to tildeio/helix that referenced this issue Feb 25, 2017
EQt added a commit to EQt/hdf5-rs that referenced this issue Aug 20, 2018
Since last commit, Rust changed the behavior of `extern static`; see
rust-lang/rust#35112

In the future, this should be change to an unsafe block, but now it suffices to turn off the error by the `#[allow(...)]` switch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants