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. |
|
I feel like this is missing a pretty big breaking change documentation... While one could argue that programs should have proper signaling schemes for exit so all cleanup happens at the right time and under controlled conditions, being able to register handlers for (not memory related) cleanups like unlinking temporary files (UNIX sockets, FIFOs, shared buffers) is useful, and libraries not under direct control might depend on this as well. This seems to me like it would be especially relevant for external dependencies (e.g. C libraries). At the very least a heads up for such users would be nice. (I'll be honest though, calling |
|
Specifically we'd want to ban |
|
hmm, well maybe we could do something simpler with a global atomic that effectively turns |
|
That doesn't solve the problem of calling |
|
Here's an example demonstrating UB without |
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.