Skip to content

Rust 1.83.0 Sync with dyn Traits Regression #134014

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

Closed
IGN-Styly opened this issue Dec 7, 2024 · 13 comments
Closed

Rust 1.83.0 Sync with dyn Traits Regression #134014

IGN-Styly opened this issue Dec 7, 2024 · 13 comments
Labels
C-gub Category: the reverse of a compiler bug is generally UB

Comments

@IGN-Styly
Copy link

IGN-Styly commented Dec 7, 2024

Code

Problematic Version: rustc 1.83.0 (90b35a6 2024-11-26)
Works on rust rustc 1.82.0 (f6e511e 2024-10-15)

Using my lib, here's a short example causing the segfault.
Issue Demo Requires Submodule!
SegFault

Thread 1 "server" received signal SIGSEGV, Segmentation fault.
alloc::sync::{impl#29}::deref<dyn enginelib::event::EventHandler, alloc::alloc::Global> (self=0x555555d77db0)
    at /home/styly/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/sync.rs:2179
2179	        &self.inner().data

Rust 1.83 seems just leak. #128778 may be the cause but i can't seem to compile rust at all.

@IGN-Styly IGN-Styly added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Dec 7, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 7, 2024
@compiler-errors
Copy link
Member

What is the regression here? I don't think you mentioned what the problem actually is.

@IGN-Styly
Copy link
Author

IGN-Styly commented Dec 7, 2024

Compilation happens fine but on execution a segfault happens, which didn't happen on previous versions. My bets are on: #128778 i am currently testing.

@IGN-Styly
Copy link
Author

@compiler-errors I Just made my lib oss, and updated issue to have code to recreate the bug.

@apiraino
Copy link
Contributor

apiraino commented Dec 9, 2024

@IGN-Styly can you please update your report adding how do you build your library. That would help in reproducing more quickly the issue. Thank you.

@jieyouxu
Copy link
Member

jieyouxu commented Dec 9, 2024

More specifically, can you also make your example self-contained, e.g. in a repo, so that we can confirm what we try to run is the same as what you try to run?

@jieyouxu jieyouxu added S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. S-needs-info Status: The issue lacks details necessary to triage or act on it. labels Dec 9, 2024
@IGN-Styly
Copy link
Author

yes will do asap

@IGN-Styly
Copy link
Author

Have Updated to include a repo to easily recreate the bug.

@cyrgani
Copy link
Contributor

cyrgani commented Dec 9, 2024

Can reproduce, working on minimizing this.

@rustbot label: -S-needs-repro -S-needs-info

@rustbot rustbot removed S-needs-repro Status: This issue has no reproduction and needs a reproduction to make progress. S-needs-info Status: The issue lacks details necessary to triage or act on it. labels Dec 9, 2024
@apiraino apiraino added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Dec 9, 2024
@aDotInTheVoid aDotInTheVoid added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Dec 9, 2024
@cyrgani
Copy link
Contributor

cyrgani commented Dec 9, 2024

https://github.com/cyrgani/issue-134014 is a reduction that still keeps the original behavior of not segfaulting on 1.82 but on 1.83 and needs no external crates. However, removing the simple println!("cursed"); line in the repro causes it to segfault on 1.82 as well.

Additionally interesting is that the original unmodified code segfaults on 1.80 and below too, but not on 1.81.

@IGN-Styly
Copy link
Author

IGN-Styly commented Dec 10, 2024

is it haunted?( i know my code is shitty but i haven't found a better solution) might be lifetime related, and if you dont mind ill add the minimized repro into the inial repo.

@workingjubilee workingjubilee added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Dec 10, 2024
@workingjubilee
Copy link
Member

I am removing the regression status because based on @cyrgani's report it seems very likely that what happened here is difficult to classify as a regression.

@workingjubilee workingjubilee removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-untriaged Untriaged performance or correctness regression. labels Dec 10, 2024
@lukas-code
Copy link
Member

lukas-code commented Dec 10, 2024

@cyrgani's minimal repro from #134014 (comment) has UB by reading from a vtable that is located in a dynamic library that has been dynamically unloaded.

The reduced example looks roughly like this:

      enginelib
      /       \
 engine       engine_core

where enginelib is a library that has a trait impl

pub trait Task {}
pub struct FibTask {}
impl Task for FibTask {}

engine_core is a dynamic library that exports a function like this:

#[no_mangle]
pub fn run(api: &mut Option<Box<dyn Task>>) {
    // here, the vtable is set to a pointer to a global variable in the `engine_core.so` file
    *api = Some(Box::new(FibTask {}) as Box<dyn Task>);
}

and engine is an executable that dynamically loads (and unloads!) engine_core.so:

fn main() {
    let mut api: Option<Box<dyn Task> = None;

    unsafe {
        let lib = Library::open(); // load `engine_core.so`
        let run: Symbol<unsafe extern "Rust" fn(reg: &mut EngineAPI)> = lib.get();
        run(&mut api); // sets vtable pointer to a vtable located in `engine_core.so`
        // `lib` goes out of scope, `engine_core.so` is unloaded
    }

    // `api` goes out of scope and the destructor of `dyn Task` is executed, which will attempt to load the
    // `drop_in_place` function from the vtable, but here the vtable is a danging pointer, because the global
    // variable that it points to has been freed by unloading `engine_core`! 💥  
}

This works in 1.82, because for some reason the dlclose doesn't actually cause the dynamic library to be unloaded on that version. I haven't investigated why this happens.

Anyway, I'm inclined to say that this is probably not a compiler bug: When dynamic libraries are dynamically unloaded, then global variables (including vtables!) are suddenly not so global anymore, and it is the responsibility of the programmer to ensure that they aren't used after they have been freed.

@jieyouxu
Copy link
Member

jieyouxu commented Dec 10, 2024

Thanks for the analysis @lukas-code and the reduction @cyrgani! I'm going to close this as not-a-compiler-bug.

@jieyouxu jieyouxu closed this as not planned Won't fix, can't repro, duplicate, stale Dec 10, 2024
@jieyouxu jieyouxu removed C-bug Category: This is a bug. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc labels Dec 10, 2024
@jieyouxu jieyouxu added C-gub Category: the reverse of a compiler bug is generally UB and removed E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-gub Category: the reverse of a compiler bug is generally UB
Projects
None yet
Development

No branches or pull requests

9 participants