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

Draft: fix ub, shrink unsafe blocks #1435

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

antonilol
Copy link
Contributor

Breaking changes:

  • mixer::Chunk fields were made private because the struct relied on these pointer being valid, but users could set them to any value
  • Timer callback must live for 'static because the callback can be called after the scope ends (by using std::mem::forget).

timer ub reproduction (ran from a #[test]):

let sdl_context = crate::sdl::init().unwrap();
let timer_subsystem = sdl_context.timer().unwrap();

let mut v = Box::new(123);

std::mem::forget(timer_subsystem.add_timer(
    20,
    Box::new(|| {
        dbg!(std::mem::take(&mut v));
        0
    }),
));

drop(v);

// v is used after dropping (from the timer callback)

std::thread::sleep(Duration::from_millis(30));

Comment on lines -124 to -127
// I tried using `std::panic::catch_unwind()` here and it compiled but
// would not catch. Maybe wait for `c_unwind` to stabilize? Then the behavior
// will automatically abort the process when panicking over an `extern "C"`
// function.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why that did not work, it should work.

Since Rust 1.81, unwinding in an extern "C" function will abort the process, I added the catch_unwind to not have this make undefined behavior on "older" (1.81 is almost 2 weeks old at the time of writing this comment) versions and have the same behavior independent of the Rust version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant