rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
95.46k stars 12.29k forks source link

Regression in global_allocator when using prefer-dynamic on 1.71.0 and above #114518

Open alexkornitzer opened 11 months ago

alexkornitzer commented 11 months ago

Since 1.71.0 a regression has been added into the compiler causing segmentation faults in applications compiled using prefer-dynamic, this issue is not present in 1.70.0. It appears that the system allocator is still being used in some cases causing segmentation faults.

Code

https://github.com/alexkornitzer/dylib-errors/tree/error/jemalloc

use jemallocator::Jemalloc;

#[global_allocator]
static GLOBAL: Jemalloc = Jemalloc;

fn main() {
    let a = std::thread::spawn(move || {});
    a.join().unwrap();
    println!("didn''t crash");
}

Version it worked on

It most recently worked on: 1.70.0

Version with regression

1.71.0

Have run git bisect and the regression was introduced with the following commit: a2b1646c597329d0a25efa3889b66650f65de1de

lqd commented 11 months ago

cc @bjorn3 for that bisection to #86844

bjorn3 commented 11 months ago

As far as I understand this shouldn't work on Windows ever before my PR as on Windows symbol preemption across dll's doesn't happen, so inside std.dll it would resolve __rust_alloc to the system allocator unconditionally. cc https://github.com/rust-lang/rust/issues/100781#issuecomment-1636773228

alexkornitzer commented 11 months ago

This is happening for me on macOS and Linux. What is odd if that cc above is accurate is that global_allocator and -C prefer-dynamic has worked for years for me, its only with 1.71.0 and above that its now broken.

bjorn3 commented 11 months ago

I mean it should always have been broken on Windows as far as I understand and now is broken on other platforms too.

alexkornitzer commented 11 months ago

Ah right gotcha, just did not get why Windows was being brought in scope but then again I was not clear that linux was my deployment target and macos my development target. Is there anything I can do to help you fix the regression?

bjorn3 commented 11 months ago

When I wrote https://github.com/rust-lang/rust/pull/86844 I expected rustc to error on #[global_allocator] when a dylib with the allocator shim (like libstd.so) is linked. Instead it turns out that rustc accepts it just fine, but ignores #[global_allocator] entirely. For example the following doesn't panic when compiled with -Cprefer-dynamic with rustc 1.70:

use std::alloc::*;

struct MyAlloc;

unsafe impl GlobalAlloc for MyAlloc {
    unsafe fn alloc(&self, _: Layout) -> *mut u8 { todo!() }
    unsafe fn dealloc(&self, _: *mut u8, _: Layout) { todo!() }
}

#[global_allocator]
static ALLOC: MyAlloc = MyAlloc;

fn main() {
    Box::new(0);
}

and __rust_alloc comes from libstd.so. On ELF based systems it would be possible for rustc to generate the allocator shim and then thus have #[global_allocator] apply, but for Windows and likely macOS it would result in the exact kind of crash you are observing here as preempting symbols from dynamic libraries is not possible and thus the main executable and dynamic libraries will observe a different allocator. The least confusing fix is likely to give an error instead.

alexkornitzer commented 11 months ago

So basically on 1.70.0 and below, when -C prefer-dynamic was set with a #[global_allocator] it was actually just ignoring it and using the default system allocator? If this is the case then yep raising an error makes total sense cause having silent segmentation faults is definitely not good and difficult to debug as the root cause.

bjorn3 commented 11 months ago

So basically on 1.70.0 and below, when -C prefer-dynamic was set with a #[global_allocator] it was actually just ignoring it and using the default system allocator?

Yes

apiraino commented 11 months ago

Removing the prioritization label as the comments seem to point out that now this is the intended behaviour. Though probably can be surprising to some users, so I wonder about the actionable here.

@rustbot label -I-prioritize

alexkornitzer commented 11 months ago

@apiraino surely its important to get the compiler to throw an error here or people are going to get unexpected segfaults. This is probably not that apparent due to the rare use of both features, but rustc is now creating invalid binaries.

apiraino commented 11 months ago

ok @alexkornitzer so what would be the desired behaviour in 1.71.0 instead of crashing? Some kind of safeguard preventing it (and maybe giving useful info to the user)? Thanks for helping me figuring the context :-)

alexkornitzer commented 11 months ago

Yeah that would be ideal, which I think is what @bjorn3 suggested but never actioned? (https://github.com/rust-lang/rust/issues/100781#issuecomment-1636783226)

apiraino commented 11 months ago

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium