diff options
author | Samuel Williams <samuel.williams@oriontransfer.co.nz> | 2022-05-25 23:12:54 +1200 |
---|---|---|
committer | Samuel Williams <samuel.williams@oriontransfer.co.nz> | 2022-05-26 00:17:30 +1200 |
commit | d875445e8a8b073298e8b3db177d1a5e78c92893 (patch) | |
tree | 0f670ebc108fd139684b4c592ebf87876bfd9274 | |
parent | cd6f87eefc35922d21a1889e038c9f50f229004f (diff) |
Fix GC race condition in autoload.
Notes
Notes:
Merged: https://github.com/ruby/ruby/pull/5941
-rw-r--r-- | test/ruby/test_autoload.rb | 1 | ||||
-rw-r--r-- | variable.c | 33 |
2 files changed, 19 insertions, 15 deletions
diff --git a/test/ruby/test_autoload.rb b/test/ruby/test_autoload.rb index f6183f5ee2..bac6337eb9 100644 --- a/test/ruby/test_autoload.rb +++ b/test/ruby/test_autoload.rb @@ -529,6 +529,7 @@ p Foo::Bar t2 = Thread.new {Bar} t1.join + GC.start # force GC. t2.join Object.send(:remove_const, :Foo) diff --git a/variable.c b/variable.c index 5c041e92ac..78eecad2c2 100644 --- a/variable.c +++ b/variable.c @@ -2669,12 +2669,6 @@ autoload_try_load(VALUE _arguments) { struct autoload_load_arguments *arguments = (struct autoload_load_arguments*)_arguments; - // We have tried to require the autoload feature, so we shouldn't bother trying again in any - // other threads. More specifically, `arguments->result` starts of as nil, but then contains the - // result of `require` which is either true or false. Provided it's not nil, it means some other - // thread has got as far as evaluating the require statement completely. - if (arguments->result != Qnil) return arguments->result; - // Try to require the autoload feature: rb_ensure(autoload_feature_require, _arguments, autoload_feature_require_ensure, _arguments); @@ -2722,7 +2716,15 @@ rb_autoload_load(VALUE module, ID name) arguments.flag = ce->flag & (CONST_DEPRECATED | CONST_VISIBILITY_MASK); // Only one thread will enter here at a time: - return rb_mutex_synchronize(arguments.mutex, autoload_try_load, (VALUE)&arguments); + VALUE result = rb_mutex_synchronize(arguments.mutex, autoload_try_load, (VALUE)&arguments); + + // If you don't guard this value, it's possible for the autoload constant to + // be freed by another thread which loads multiple constants, one of which + // resolves to the constant this thread is trying to load, so proteect this + // so that it is not freed until we are done with it in `autoload_try_load`: + RB_GC_GUARD(autoload_const_value); + + return result; } VALUE @@ -3312,18 +3314,19 @@ rb_const_set(VALUE klass, ID id, VALUE val) } static struct autoload_data * -autoload_data_for_named_constant(VALUE mod, ID id, struct autoload_const **acp) +autoload_data_for_named_constant(VALUE module, ID name, struct autoload_const **autoload_const_pointer) { - struct autoload_data *ele; - VALUE load = autoload_data(mod, id); - if (!load) return 0; - ele = get_autoload_data(load, acp); - if (!ele) return 0; + VALUE autoload_data_value = autoload_data(module, name); + if (!autoload_data_value) return 0; + + struct autoload_data *autoload_data = get_autoload_data(autoload_data_value, autoload_const_pointer); + if (!autoload_data) return 0; /* for autoloading thread, keep the defined value to autoloading storage */ - if (autoload_by_current(ele)) { - return ele; + if (autoload_by_current(autoload_data)) { + return autoload_data; } + return 0; } |