Repository navigation
Conversation
|
cc @rust-lang/miri |
|
r? @ChrisDenton rustbot has assigned @ChrisDenton. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This was discussed in the past and the conclusion was that adding a lock on our side is good enough for now; musl and glibc have also added locks on their side.
It should generally work like "return from main", I just recently finally fixed that in Miri.^^ (Some of those tests I added will probably fail now.) |
|
The one alternative I can think of to fix the soundness problem is to mark TLS variables where a |
|
On every platform but macOS we can also fix this by using the pthread-TLS-key-based destructor registration – pthread TLS destructors aren't run on As an aside, I'm actually very surprised that C++ doesn't have the same problem. I don't really know the language, but I'd imagine there are a ton of move constructors (or is it assignment operators?) that are unsound for a similar reason. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Does this also change the behavior of
I think the C++ approach is to say "if you call |
| } | ||
| } | ||
| } | ||
| target_os = "windows" => unsafe { crate::sys::pal::c::ExitProcess(code as u32) }, |
There was a problem hiding this comment.
AFAIK this still calls destructors on Windows
There was a problem hiding this comment.
Please make the test call libc::exit directly so that we keep testing the same codepath as before here (ensuring that atexit handlers get invoked on exit).
There was a problem hiding this comment.
Seems worth a comment in the test that exit does not invoke the destructors so we will not get output in that case.
| )?; | ||
| } else { | ||
| // Stop interpreter loop. | ||
| throw_machine_stop!(TerminationInfo::Exit { |
There was a problem hiding this comment.
We should still do the "yield 100 times" thing here that exit does, by invoking replace_active_thread_stack. Looks like ExitType::ExitCalled needs a field saying whether atexit handlers should be invoked.
I can handle that if you prefer.
That's the same fix as what your PR does, just different in implementation details. I was thinking about a fix that actually behaves differently. |
|
☔ The latest upstream changes (presumably #165375) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
|
Per today's libs meeting, we'd like to look into the option of using pthread TLS - there's probably a good reason not to use it, but it's worth double-checking. Failing that, we're in favour of merging this PR and possibly exposing an unsafe exit function that also calls atexit handlers. Some other options were raised (notably, registering an |
This comment was marked as outdated.
This comment was marked as outdated.
|
I looked into this, and per #10312 at least, it looks like we switched away from pthread TLS for multiple reasons: performance (pthread TLS is slower, especially for repeated accesses) and limited slots, at the very least. So, I don't think we should switch back to pthread TLS. Proposal: let's merge this PR, and (per suggestion from @clarfonthey) consider adding an unstable exit function that does call |
|
fair enough. with that in mind: @rfcbot fcp merge libs |
|
@nia-e has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/libs-ping: FCP proposed for libs, please feel free to register concerns. |
|
Since it was brought up in the meeting I'll also add that Windows doesn't have the Unix unsafety issue (#126600, #164243) because it it is thread safe but it can still deadlock (#164273) due to Windows having a lock and immediately terminating other threads. And of course every platform has the issue that locals aren't dropped (which can be surprising if they have intentional side-effects). |
|
This is still unsound on Windows though as the destructor gets |
|
Oh hm, yeah, that is still an issue. |
View all comments
Fixes #126600
Fixes #164243
Fixes #164273
This fixes three issues related to
process::exitat once: C'sexitis not MT-safe, and there is no entirely sound way to make it so, and it calls destructors for TLS variables (and C++ static variables) that may be currently in use, leading to double-frees and similar undefined behaviour. The POSIX-defined_exitfunction on the other hand immediately terminates the process without running any handlers and hence does not have any of these problems, so this PR switchesprocess::exitto use it instead ofexit.While skipping
atexithandlers is a bit unfortunate, it shouldn't come as that big a surprise to users ofprocess::exit; that function is already documented not to perform any stack cleanup. It's a change in behaviour, though, so this will need FCP.