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

#[no_mangle] is unsafe #28179

Open
geofft opened this Issue Sep 2, 2015 · 16 comments

Comments

Projects
None yet
@geofft
Copy link
Contributor

geofft commented Sep 2, 2015

On some platforms (at least GNU/Linux, but I hear Windows and several others too), if you link together two static libraries that both export a symbol of the same name, it's undefined which symbol actually gets linked. In practice on my machine, the first library seems to win. This lets you defeat type/memory safety without the unsafe keyword, by having two crates export a #[no_mangle] pub fn with different signatures but compatible calling conventions:

// one.rs
#![crate_type = "lib"]

#[no_mangle]
pub fn convert(x: &'static i32) -> Result<i32, f32> {
    Ok(*x)
}
// two.rs
#![crate_type = "lib"]

#[no_mangle]
pub fn convert(x: &'static f32) -> Result<i32, f32> {
    Err(*x)
}
// transmute.rs
extern crate one;
extern crate two;             

fn main() {
    static X: f32 = 3.14;
    let y: i32 = two::convert(&X).unwrap();
    println!("{}", y);
}
geofft@titan:/tmp/transmute$ rustc one.rs
geofft@titan:/tmp/transmute$ rustc two.rs
geofft@titan:/tmp/transmute$ rustc transmute.rs -L .
geofft@titan:/tmp/transmute$ ./transmute 
1078523331

Despite the stated call to two::convert, it's actually one::convert that gets called, which interprets the argument as a &'static i32. (It may be clearer to understand with this simpler example, which doesn't break type safety.)

On at least GNU/Linux but not other platforms like Windows or Darwin, dynamically-linked symbols have the same ambiguity.

I don't know what the right response is here. The following options all seem pretty reasonable:

  1. Acknowledge it and ignore it. Maybe document it as a possible source of unsafety, despite not using the unsafe keyword.
  2. Have #[no_mangle] export both a mangled and un-mangled name, and have Rust crates call each other via mangled names only, on the grounds that #[no_mangle] is for external interfaces, not for crates linking to crates. ("External interfaces" includes other Rust code using FFI, but FFI is unsafe.) This is analogous to how extern "C" fns export both a Rust-ABI symbol as well as a C-ABI shim, and a direct, safe Rust call to those function happens through the Rust ABI, not through the C ABI. I'm pretty sure that all production uses of #[no_mangle] are extern "C", anyway (see e.g. #10025).
  3. Deprecate #[no_mangle] on safe functions and data, and introduce a new #[unsafe_no_mangle], so it substring-matches unsafe. (#[no_mangle] on unsafe functions or mutable statics is fine, since you need the unsafe keyword to get at them.)

All of these are, I think, backwards-compatible.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 3, 2015

I think this even leads to a linker error on some platforms (depending on the situation), so in that sense I'd also be fine just having a set of all #[no_mangle] symbols for each crate and if they overlap we generate a hard compile time error. While strictly not backwards compatible I believe it's practically fine and it's basically what most crates what anyway

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented Sep 3, 2015

You can #[no_mangle] override an existing mangled function too:

fn main() {
  println!("ok")
}

#[no_mangle]
#[allow(non_snake_case)]
pub fn _ZN2io5stdio6_print20h94cd0587c9a534faX3gE() {
    unreachable!()
}

This makes the playpen timeout on stable 1.2: http://is.gd/sxdUl3

@geofft

This comment has been minimized.

Copy link
Contributor Author

geofft commented Sep 3, 2015

While strictly not backwards compatible I believe it's practically fine

Yeah, I'd argue that having multiply-defined #[no_mangle] symbols is, in essence, undefined behavior (in the sense that nobody defined it), so making it a hard error seems fine.

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Sep 3, 2015

I doubt there’s anything sensible (other than writing our own linker) that can be done here to truly fix the issue.

I’m not against the idea of making less of these cases possible, though. e.g.

#[no_mangle]
fn main(){}

doesn’t work already. But most of the proposed solutions (exception being keeping more symbols in memory) sound like a non-option to me.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Sep 8, 2015

@nagisa

#[no_mangle] will always be always unsafe as you can use it to hijack C library functions, .e.g make malloc do something evil.

@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Sep 8, 2015

@arielb1 i didn’t say anything about safety; only about our capability to do anything about this issue.

Either way, if you have to be wary of code you control unknowingly hijacking other code or doing evil things, something went very wrong where Rust (or any language, really) can’t help.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Sep 15, 2015

@nagisa

JS is designed for that use-case. Rust is theoretically supposed to fill that role too, but both rustc and the type-system are way not sufficiently trustworthy for that.

@bstrie

This comment has been minimized.

Copy link
Contributor

bstrie commented Oct 19, 2016

I agree that unmangled symbol collisions should be a compilation error. Breakage here is extraordinarily unlikely, and the only code that it could break is code that won't be working as intended anyway, which is exactly the sort of the code that we ought to break.

@nikomatsakis nikomatsakis added T-lang and removed T-compiler labels Oct 20, 2016

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Oct 20, 2016

Reassigning to lang team. What a pain.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Nov 3, 2016

triage: P-low

Seems like a real problem, if not that concerning. @rust-lang/lang feels that the fix proposed by @alexcrichton in this comment is the correct one. Check at link time for duplicate #[no_mangle] symbols in rust code; don't try to solve similar problems that might occur with C code and other stuff out of our purview.

@vi

This comment has been minimized.

Copy link
Contributor

vi commented Jan 27, 2018

Why is calling #[no_mangle] functions safe in the first place?

Also #[no_mangle] feels like a first step towards #[naked], which should also require unsafe to be called.

@geofft

This comment has been minimized.

Copy link
Contributor Author

geofft commented Jan 27, 2018

Why is calling #[no_mangle] functions safe in the first place?

They can't do anything that normal functions can't do. And there isn't in theory any way to use them to violate safety (this issue is a bug; it would be totally fine in theory for rustc to fail to compile code with any symbol collisions in any objects, whether they come from Rust or from another language).

#[naked], which should also require unsafe to be called.

As far as I can tell, this is basically because naked functions have no guaranteed ret opcode, so #[naked] fn foo() -> {println!("hi!")} will just let the instruction pointer run past the end of a function, which is rather memory-unsafe. If they did (and if there were a more coherent story about accessing arguments and returning values from pure Rust code), I'm having trouble seeing what makes them unsafe.

That is, the reason #[naked] functions should require unsafe is that the compiler expects a particular thing to be done by the implementation in order for them to be safely callable, and it has no way to check that. (I sort of think that, like unsafe traits, it should be possible for the implementor to say "Trust me on this" and others to be able to call the function from safe code, but maybe this is a discussion for the naked function RFC, not here.) There's nothing special that #[no_mangle] functions need to take care to do; they're just the same as normal functions except for their linkage name.

@vi

This comment has been minimized.

Copy link
Contributor

vi commented Jan 27, 2018

They can't do anything that normal functions can't do.

It's not the function itself that is unsafe but the call. The call to something unmangled is FFI-ish, therefore looks (and is) unsafe.

#[naked] functions are unsafe per se, so calls to them should be unsafe not directly, but as a consequence.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 29, 2018

There are some other examples of abusing #[no_mangle] that may be worth mentioning.

For example, this hack overrides main with custom assembly:

#![no_main]

#[link_section=".text"]
#[no_mangle]
pub static main: [u32; 9] = [
    3237986353,
    3355442993,
    120950088,
    822083584,
    252621522,
    1699267333,
    745499756,
    1919899424,
    169960556,
];

Sigh.

@vi

This comment has been minimized.

Copy link
Contributor

vi commented Jan 29, 2018

#![forbid(unsafe)] code should be assumed free of such hacks...

@Centril Centril added the A-linkage label Nov 20, 2018

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Dec 3, 2018

#![forbid(unsafe)] code should be assumed free of such hacks...

Yeah, my thinking exactly. Maybe that should forbid no_mangle as well, or so?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment