RUBY_FREE_AT_EXIT fixes #9324
Merged
RUBY_FREE_AT_EXIT fixes #9324
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Nice find! The description makes a ton of sense to me! 👍 |
This argument doesn't seem used anymore. Since we want to free these objects during VM destruction when RUBY_FREE_AT_EXIT is set they must work without an EC. This avoids a use-after-free running `RUBY_FREE_AT_EXIT=1 ./miniruby -e ''`
Ractor's free iterates through its TLS keys so we need to keep this
memory available until after Ractors are freed.
Minimal reproduction:
RUBY_FREE_AT_EXIT=1 ./miniruby -e rand
When a forked process was started in a thread, this would result in a
double-free during the child process exit.
RUBY_FREE_AT_EXIT=1 ./miniruby -e 'Thread.new { fork { } }.join; Process.waitpid'
This is because the main thread in the forked process was not the
initial VM thread, and the new thread's stack was freed as part of
objectspace iteration.
This change also allows rb_threadptr_root_fiber_release to run without
EC being available.
1a3f29c to
9c65ccb
Compare
peterzhu2118
approved these changes
Dec 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a couple errors which could occur when RUBY_FREE_AT_EXIT=1 is enabled. These fixes aren't perfect, but I think fixes which avoided more leaks and never caused errors would be more complex and would be dangerous to merge so close to 3.3's release. (I'd like to investigate this more next year and maybe we can backport the fixes For 3.3.1)
First, during VM destruction EC seems to be NULL, this avoids two cases we were always calling
GET_EC()at that time (one we just didn't need and the other I added an if case for it being NULL). This fixes an error which always occurred when Ruby was built in debug.Fixes
RUBY_FREE_AT_EXIT=1 ./miniruby -e ''Next, we need to move the freeing of
default_rand_keyafter the freeing of Ractors inrb_objspace_free_objects, as that will iterate over over the local storage keys so the memory needs to be freed later to avoid a use-after-free.Fixes
RUBY_FREE_AT_EXIT=1 ./miniruby -e randLastly when we forked for a Thread we would end up double-freeing the main thread's stack, because the main thread was no longer the initial thread from
Init_BareVM. The new Thread's stack was freed naturally inrb_objspace_free_objectsFixes
RUBY_FREE_AT_EXIT=1 ./miniruby -e 'Thread.new { fork { } }.join; Process.waitpid'With these changes
RUBY_FREE_AT_EXIT=1 make btestworks with RUBY_DEBUG and ASAN enabled (other than a few Ractor tests which detect false positives).cc @HParker @peterzhu2118