Skip to content

Commit 938a94c

Browse files
luke-gruberluke-gruber
committed
Make sure we never context switch while holding VM lock.
We were seeing errors in our application that looked like: ``` [BUG] unexpected situation - recordd:1 current:0 /error.c:1097 rb_bug_without_die_internal /vm_sync.c:275 disallow_reentry /eval_intern.h:136 rb_ec_vm_lock_rec_check /eval_intern.h:147 rb_ec_tag_state /vm.c:2619 rb_vm_exec /vm.c:1702 rb_yield /eval.c:1173 rb_ensure ``` We concluded that there was context switching going on while a thread held the VM lock. During the investigation into the issue, we added assertions that we never yield to another thread with the VM lock held. We enabled these VM lock assertions even in single ractor mode. These assertions were failing in a few places, but most notably in finalizers. We were running finalizers with the VM lock held, and they were context switching and causing this issue. These rules must be held going forward to ensure we don't context switch unexpectedly: If you have the VM lock held, * Don't enter the interpreter loop. * Don't yield to ruby code. * Don't call rb_nogvl (it will context switch you and will not unlock the VM lock). * You can still check interrupts, but we won't allow context switching If you don't have the GVL: * Don't call rb_ensure/rb_protect, etc (these are old rules but good to have assertions for). Rework global variables, don't lock when calling getter or setter Add a test that fails without these lock_rec changes. Add ASSERT_vm_unlocking() to vm_call0_body This uncovered many more test failures. Revert changes introduced in 2f6c694 This doesn't appear to be a correct fix. We should allow raising NoMemoryError even if we're under the VM lock. another commit
1 parent 8f040a5 commit 938a94c

File tree

22 files changed

+333
-100
lines changed

22 files changed

+333
-100
lines changed

‎depend‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4199,7 +4199,9 @@ encoding.$(OBJEXT): $(top_srcdir)/internal/gc.h
41994199
encoding.$(OBJEXT): $(top_srcdir)/internal/imemo.h
42004200
encoding.$(OBJEXT): $(top_srcdir)/internal/inits.h
42014201
encoding.$(OBJEXT): $(top_srcdir)/internal/load.h
4202+
encoding.$(OBJEXT): $(top_srcdir)/internal/namespace.h
42024203
encoding.$(OBJEXT): $(top_srcdir)/internal/object.h
4204+
encoding.$(OBJEXT): $(top_srcdir)/internal/sanitizers.h
42034205
encoding.$(OBJEXT): $(top_srcdir)/internal/serial.h
42044206
encoding.$(OBJEXT): $(top_srcdir)/internal/set_table.h
42054207
encoding.$(OBJEXT): $(top_srcdir)/internal/static_assert.h
@@ -13362,6 +13364,7 @@ re.$(OBJEXT): {$(VPATH)}backward/2/stdalign.h
1336213364
re.$(OBJEXT): {$(VPATH)}backward/2/stdarg.h
1336313365
re.$(OBJEXT): {$(VPATH)}config.h
1336413366
re.$(OBJEXT): {$(VPATH)}constant.h
13367+
re.$(OBJEXT): {$(VPATH)}debug_counter.h
1336513368
re.$(OBJEXT): {$(VPATH)}defines.h
1336613369
re.$(OBJEXT): {$(VPATH)}encindex.h
1336713370
re.$(OBJEXT): {$(VPATH)}encoding.h
@@ -13545,6 +13548,7 @@ re.$(OBJEXT): {$(VPATH)}util.h
1354513548
re.$(OBJEXT): {$(VPATH)}vm_core.h
1354613549
re.$(OBJEXT): {$(VPATH)}vm_debug.h
1354713550
re.$(OBJEXT): {$(VPATH)}vm_opts.h
13551+
re.$(OBJEXT): {$(VPATH)}vm_sync.h
1354813552
regcomp.$(OBJEXT): $(hdrdir)/ruby.h
1354913553
regcomp.$(OBJEXT): $(hdrdir)/ruby/ruby.h
1355013554
regcomp.$(OBJEXT): {$(VPATH)}assert.h
@@ -14791,7 +14795,9 @@ ruby.$(OBJEXT): {$(VPATH)}thread_$(THREAD_MODEL).h
1479114795
ruby.$(OBJEXT): {$(VPATH)}thread_native.h
1479214796
ruby.$(OBJEXT): {$(VPATH)}util.h
1479314797
ruby.$(OBJEXT): {$(VPATH)}vm_core.h
14798+
ruby.$(OBJEXT): {$(VPATH)}vm_debug.h
1479414799
ruby.$(OBJEXT): {$(VPATH)}vm_opts.h
14800+
ruby.$(OBJEXT): {$(VPATH)}vm_sync.h
1479514801
ruby.$(OBJEXT): {$(VPATH)}yjit.h
1479614802
ruby_parser.$(OBJEXT): $(hdrdir)/ruby/ruby.h
1479714803
ruby_parser.$(OBJEXT): $(top_srcdir)/internal/array.h
@@ -17722,21 +17728,32 @@ time.$(OBJEXT): {$(VPATH)}timev.rbinc
1772217728
time.$(OBJEXT): {$(VPATH)}util.h
1772317729
time.$(OBJEXT): {$(VPATH)}vm_core.h
1772417730
time.$(OBJEXT): {$(VPATH)}vm_opts.h
17731+
transcode.$(OBJEXT): $(CCAN_DIR)/check_type/check_type.h
17732+
transcode.$(OBJEXT): $(CCAN_DIR)/container_of/container_of.h
17733+
transcode.$(OBJEXT): $(CCAN_DIR)/list/list.h
17734+
transcode.$(OBJEXT): $(CCAN_DIR)/str/str.h
1772517735
transcode.$(OBJEXT): $(hdrdir)/ruby/ruby.h
1772617736
transcode.$(OBJEXT): $(top_srcdir)/internal/array.h
17737+
transcode.$(OBJEXT): $(top_srcdir)/internal/basic_operators.h
1772717738
transcode.$(OBJEXT): $(top_srcdir)/internal/class.h
1772817739
transcode.$(OBJEXT): $(top_srcdir)/internal/compilers.h
1772917740
transcode.$(OBJEXT): $(top_srcdir)/internal/encoding.h
1773017741
transcode.$(OBJEXT): $(top_srcdir)/internal/gc.h
17742+
transcode.$(OBJEXT): $(top_srcdir)/internal/imemo.h
1773117743
transcode.$(OBJEXT): $(top_srcdir)/internal/inits.h
17744+
transcode.$(OBJEXT): $(top_srcdir)/internal/namespace.h
1773217745
transcode.$(OBJEXT): $(top_srcdir)/internal/object.h
17746+
transcode.$(OBJEXT): $(top_srcdir)/internal/sanitizers.h
1773317747
transcode.$(OBJEXT): $(top_srcdir)/internal/serial.h
17748+
transcode.$(OBJEXT): $(top_srcdir)/internal/set_table.h
1773417749
transcode.$(OBJEXT): $(top_srcdir)/internal/static_assert.h
1773517750
transcode.$(OBJEXT): $(top_srcdir)/internal/string.h
1773617751
transcode.$(OBJEXT): $(top_srcdir)/internal/transcode.h
1773717752
transcode.$(OBJEXT): $(top_srcdir)/internal/variable.h
17753+
transcode.$(OBJEXT): $(top_srcdir)/internal/vm.h
1773817754
transcode.$(OBJEXT): $(top_srcdir)/internal/warnings.h
1773917755
transcode.$(OBJEXT): {$(VPATH)}assert.h
17756+
transcode.$(OBJEXT): {$(VPATH)}atomic.h
1774017757
transcode.$(OBJEXT): {$(VPATH)}backward/2/assume.h
1774117758
transcode.$(OBJEXT): {$(VPATH)}backward/2/attributes.h
1774217759
transcode.$(OBJEXT): {$(VPATH)}backward/2/bool.h
@@ -17905,15 +17922,24 @@ transcode.$(OBJEXT): {$(VPATH)}internal/value_type.h
1790517922
transcode.$(OBJEXT): {$(VPATH)}internal/variable.h
1790617923
transcode.$(OBJEXT): {$(VPATH)}internal/warning_push.h
1790717924
transcode.$(OBJEXT): {$(VPATH)}internal/xmalloc.h
17925+
transcode.$(OBJEXT): {$(VPATH)}method.h
1790817926
transcode.$(OBJEXT): {$(VPATH)}missing.h
17927+
transcode.$(OBJEXT): {$(VPATH)}node.h
1790917928
transcode.$(OBJEXT): {$(VPATH)}onigmo.h
1791017929
transcode.$(OBJEXT): {$(VPATH)}oniguruma.h
17930+
transcode.$(OBJEXT): {$(VPATH)}ruby_assert.h
17931+
transcode.$(OBJEXT): {$(VPATH)}ruby_atomic.h
17932+
transcode.$(OBJEXT): {$(VPATH)}rubyparser.h
1791117933
transcode.$(OBJEXT): {$(VPATH)}shape.h
1791217934
transcode.$(OBJEXT): {$(VPATH)}st.h
1791317935
transcode.$(OBJEXT): {$(VPATH)}subst.h
17936+
transcode.$(OBJEXT): {$(VPATH)}thread_$(THREAD_MODEL).h
17937+
transcode.$(OBJEXT): {$(VPATH)}thread_native.h
1791417938
transcode.$(OBJEXT): {$(VPATH)}transcode.c
1791517939
transcode.$(OBJEXT): {$(VPATH)}transcode_data.h
17940+
transcode.$(OBJEXT): {$(VPATH)}vm_core.h
1791617941
transcode.$(OBJEXT): {$(VPATH)}vm_debug.h
17942+
transcode.$(OBJEXT): {$(VPATH)}vm_opts.h
1791717943
transcode.$(OBJEXT): {$(VPATH)}vm_sync.h
1791817944
util.$(OBJEXT): $(hdrdir)/ruby/ruby.h
1791917945
util.$(OBJEXT): $(top_srcdir)/internal/array.h

‎encoding.c‎

Lines changed: 66 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "ruby/encoding.h"
2929
#include "ruby/util.h"
3030
#include "ruby_assert.h"
31+
#include "vm_core.h"
3132
#include "vm_sync.h"
3233
#include "ruby_atomic.h"
3334

@@ -344,6 +345,7 @@ static int
344345
enc_table_expand(struct enc_table *enc_table, int newsize)
345346
{
346347
if (newsize > ENCODING_LIST_CAPA) {
348+
RB_VM_UNLOCK();
347349
rb_raise(rb_eEncodingError, "too many encoding (> %d)", ENCODING_LIST_CAPA);
348350
}
349351
return newsize;
@@ -421,6 +423,7 @@ int
421423
rb_enc_register(const char *name, rb_encoding *encoding)
422424
{
423425
int index;
426+
bool set_const = false;
424427

425428
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
426429
index = enc_registered(enc_table, name);
@@ -439,9 +442,12 @@ rb_enc_register(const char *name, rb_encoding *encoding)
439442
}
440443
else {
441444
index = enc_register(enc_table, name, encoding);
442-
set_encoding_const(name, rb_enc_from_index(index));
445+
set_const = true;
443446
}
444447
}
448+
if (set_const) {
449+
set_encoding_const(name, rb_enc_from_index(index));
450+
}
445451
return index;
446452
}
447453

@@ -472,22 +478,25 @@ rb_enc_registered(const char *name)
472478
void
473479
rb_encdb_declare(const char *name)
474480
{
481+
int idx;
475482
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
476-
int idx = enc_registered(enc_table, name);
483+
idx = enc_registered(enc_table, name);
477484
if (idx < 0) {
478485
idx = enc_register(enc_table, name, 0);
479486
}
480-
set_encoding_const(name, rb_enc_from_index(idx));
481487
}
488+
set_encoding_const(name, rb_enc_from_index(idx));
482489
}
483490

484491
static void
485492
enc_check_addable(struct enc_table *enc_table, const char *name)
486493
{
487494
if (enc_registered(enc_table, name) >= 0) {
495+
RB_VM_UNLOCK();
488496
rb_raise(rb_eArgError, "encoding %s is already registered", name);
489497
}
490498
else if (!valid_encoding_name_p(name)) {
499+
RB_VM_UNLOCK();
491500
rb_raise(rb_eArgError, "invalid encoding name: %s", name);
492501
}
493502
}
@@ -535,9 +544,11 @@ enc_replicate(struct enc_table *enc_table, const char *name, rb_encoding *encodi
535544

536545
enc_check_addable(enc_table, name);
537546
idx = enc_register(enc_table, name, encoding);
538-
if (idx < 0) rb_raise(rb_eArgError, "invalid encoding name: %s", name);
547+
if (idx < 0) {
548+
RB_VM_UNLOCK();
549+
rb_raise(rb_eArgError, "invalid encoding name: %s", name);
550+
}
539551
set_base_encoding(enc_table, idx, encoding);
540-
set_encoding_const(name, rb_enc_from_index(idx));
541552
return idx;
542553
}
543554

@@ -552,9 +563,9 @@ enc_replicate_with_index(struct enc_table *enc_table, const char *name, rb_encod
552563
}
553564
if (idx >= 0) {
554565
set_base_encoding(enc_table, idx, origenc);
555-
set_encoding_const(name, rb_enc_from_index(idx));
556566
}
557567
else {
568+
RB_VM_UNLOCK();
558569
rb_raise(rb_eArgError, "failed to replicate encoding");
559570
}
560571
return idx;
@@ -575,6 +586,8 @@ rb_encdb_replicate(const char *name, const char *orig)
575586
r = enc_replicate_with_index(enc_table, name, rb_enc_from_index(origidx), idx);
576587
}
577588

589+
set_encoding_const(name, rb_enc_from_index(r));
590+
578591
return r;
579592
}
580593

@@ -588,6 +601,7 @@ rb_define_dummy_encoding(const char *name)
588601
rb_encoding *enc = enc_table->list[index].enc;
589602
ENC_SET_DUMMY((rb_raw_encoding *)enc);
590603
}
604+
set_encoding_const(name, rb_enc_from_index(index));
591605

592606
return index;
593607
}
@@ -605,6 +619,8 @@ rb_encdb_dummy(const char *name)
605619
ENC_SET_DUMMY((rb_raw_encoding *)enc);
606620
}
607621

622+
set_encoding_const(name, rb_enc_from_index(index));
623+
608624
return index;
609625
}
610626

@@ -671,18 +687,20 @@ enc_alias_internal(struct enc_table *enc_table, const char *alias, int idx)
671687
}
672688

673689
static int
674-
enc_alias(struct enc_table *enc_table, const char *alias, int idx)
690+
enc_alias(struct enc_table *enc_table, const char *alias, int idx, bool *set_const)
675691
{
676692
if (!valid_encoding_name_p(alias)) return -1;
677-
if (!enc_alias_internal(enc_table, alias, idx))
678-
set_encoding_const(alias, enc_from_index(enc_table, idx));
693+
if (!enc_alias_internal(enc_table, alias, idx)) {
694+
*set_const = true;
695+
}
679696
return idx;
680697
}
681698

682699
int
683700
rb_enc_alias(const char *alias, const char *orig)
684701
{
685702
int idx, r;
703+
bool set_const = false;
686704
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
687705
enc_check_addable(enc_table, alias); // can raise
688706
}
@@ -691,7 +709,11 @@ rb_enc_alias(const char *alias, const char *orig)
691709
if (idx < 0) return -1;
692710

693711
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
694-
r = enc_alias(enc_table, alias, idx);
712+
r = enc_alias(enc_table, alias, idx, &set_const);
713+
}
714+
715+
if (set_const) {
716+
set_encoding_const(alias, rb_enc_from_index(idx));
695717
}
696718

697719
return r;
@@ -701,14 +723,18 @@ int
701723
rb_encdb_alias(const char *alias, const char *orig)
702724
{
703725
int r;
726+
bool set_const = false;
704727

705728
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
706729
int idx = enc_registered(enc_table, orig);
707730

708731
if (idx < 0) {
709732
idx = enc_register(enc_table, orig, 0);
710733
}
711-
r = enc_alias(enc_table, alias, idx);
734+
r = enc_alias(enc_table, alias, idx, &set_const);
735+
}
736+
if (set_const) {
737+
set_encoding_const(alias, rb_enc_from_index(r));
712738
}
713739

714740
return r;
@@ -717,34 +743,35 @@ rb_encdb_alias(const char *alias, const char *orig)
717743
static void
718744
rb_enc_init(struct enc_table *enc_table)
719745
{
720-
ASSERT_vm_locking();
721-
enc_table_expand(enc_table, ENCODING_COUNT + 1);
722-
if (!enc_table->names) {
723-
enc_table->names = st_init_strcasetable_with_size(ENCODING_LIST_CAPA);
724-
}
746+
RB_VM_LOCKING() {
747+
enc_table_expand(enc_table, ENCODING_COUNT + 1);
748+
if (!enc_table->names) {
749+
enc_table->names = st_init_strcasetable_with_size(ENCODING_LIST_CAPA);
750+
}
725751
#define OnigEncodingASCII_8BIT OnigEncodingASCII
726752
#define ENC_REGISTER(enc) enc_register_at(enc_table, ENCINDEX_##enc, rb_enc_name(&OnigEncoding##enc), &OnigEncoding##enc)
727-
ENC_REGISTER(ASCII_8BIT);
728-
ENC_REGISTER(UTF_8);
729-
ENC_REGISTER(US_ASCII);
730-
global_enc_ascii = enc_table->list[ENCINDEX_ASCII_8BIT].enc;
731-
global_enc_utf_8 = enc_table->list[ENCINDEX_UTF_8].enc;
732-
global_enc_us_ascii = enc_table->list[ENCINDEX_US_ASCII].enc;
753+
ENC_REGISTER(ASCII_8BIT);
754+
ENC_REGISTER(UTF_8);
755+
ENC_REGISTER(US_ASCII);
756+
global_enc_ascii = enc_table->list[ENCINDEX_ASCII_8BIT].enc;
757+
global_enc_utf_8 = enc_table->list[ENCINDEX_UTF_8].enc;
758+
global_enc_us_ascii = enc_table->list[ENCINDEX_US_ASCII].enc;
733759
#undef ENC_REGISTER
734760
#undef OnigEncodingASCII_8BIT
735761
#define ENCDB_REGISTER(name, enc) enc_register_at(enc_table, ENCINDEX_##enc, name, NULL)
736-
ENCDB_REGISTER("UTF-16BE", UTF_16BE);
737-
ENCDB_REGISTER("UTF-16LE", UTF_16LE);
738-
ENCDB_REGISTER("UTF-32BE", UTF_32BE);
739-
ENCDB_REGISTER("UTF-32LE", UTF_32LE);
740-
ENCDB_REGISTER("UTF-16", UTF_16);
741-
ENCDB_REGISTER("UTF-32", UTF_32);
742-
ENCDB_REGISTER("UTF8-MAC", UTF8_MAC);
743-
744-
ENCDB_REGISTER("EUC-JP", EUC_JP);
745-
ENCDB_REGISTER("Windows-31J", Windows_31J);
762+
ENCDB_REGISTER("UTF-16BE", UTF_16BE);
763+
ENCDB_REGISTER("UTF-16LE", UTF_16LE);
764+
ENCDB_REGISTER("UTF-32BE", UTF_32BE);
765+
ENCDB_REGISTER("UTF-32LE", UTF_32LE);
766+
ENCDB_REGISTER("UTF-16", UTF_16);
767+
ENCDB_REGISTER("UTF-32", UTF_32);
768+
ENCDB_REGISTER("UTF8-MAC", UTF8_MAC);
769+
770+
ENCDB_REGISTER("EUC-JP", EUC_JP);
771+
ENCDB_REGISTER("Windows-31J", Windows_31J);
746772
#undef ENCDB_REGISTER
747-
enc_table->count = ENCINDEX_BUILTIN_MAX;
773+
enc_table->count = ENCINDEX_BUILTIN_MAX;
774+
}
748775
}
749776

750777
rb_encoding *
@@ -865,7 +892,11 @@ int
865892
rb_enc_find_index(const char *name)
866893
{
867894
int i;
868-
ASSERT_vm_unlocking(); // it needs to be unlocked so it can call `load_encoding` if necessary
895+
#if RUBY_DEBUG
896+
if (rb_multi_ractor_p() || !rb_enc_registered(name)) {
897+
ASSERT_vm_unlocking(); // it needs to be unlocked so it can call `load_encoding` if necessary
898+
}
899+
#endif
869900
GLOBAL_ENC_TABLE_LOCKING(enc_table) {
870901
i = enc_registered(enc_table, name);
871902
}
@@ -1812,6 +1843,7 @@ set_default_internal(VALUE klass, VALUE encoding)
18121843
static void
18131844
set_encoding_const(const char *name, rb_encoding *enc)
18141845
{
1846+
ASSERT_vm_unlocking();
18151847
VALUE encoding = rb_enc_from_encoding(enc);
18161848
char *s = (char *)name;
18171849
int haslower = 0, hasupper = 0, valid = 0;

‎eval_intern.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,10 @@ extern int select_large_fdset(int, fd_set *, fd_set *, fd_set *, struct timeval
9595

9696
#include <sys/stat.h>
9797

98+
int ruby_thread_has_gvl_p(void);
99+
98100
#define EC_PUSH_TAG(ec) do { \
101+
VM_ASSERT(ruby_thread_has_gvl_p()); \
99102
rb_execution_context_t * const _ec = (ec); \
100103
struct rb_vm_tag _tag; \
101104
_tag.state = TAG_NONE; \

‎gc.c‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5081,20 +5081,16 @@ rb_memerror(void)
50815081
rb_execution_context_t *ec = GET_EC();
50825082
VALUE exc = GET_VM()->special_exceptions[ruby_error_nomemory];
50835083

5084-
if (!exc ||
5085-
rb_ec_raised_p(ec, RAISED_NOMEMORY) ||
5086-
rb_ec_vm_lock_rec(ec) != ec->tag->lock_rec) {
5084+
if (!exc || rb_ec_raised_p(ec, RAISED_NOMEMORY)) {
50875085
fprintf(stderr, "[FATAL] failed to allocate memory\n");
50885086
exit(EXIT_FAILURE);
50895087
}
5090-
if (rb_ec_raised_p(ec, RAISED_NOMEMORY)) {
5091-
rb_ec_raised_clear(ec);
5092-
}
5093-
else {
5094-
rb_ec_raised_set(ec, RAISED_NOMEMORY);
5095-
exc = ruby_vm_special_exception_copy(exc);
5096-
}
5088+
rb_ec_raised_set(ec, RAISED_NOMEMORY);
5089+
exc = ruby_vm_special_exception_copy(exc);
50975090
ec->errinfo = exc;
5091+
// Jumping with the VM lock held is dangerous, as it can cause ruby frames to pop
5092+
// and that checks interrupts which can context switch.
5093+
RB_VM_UNLOCK_ALL();
50985094
EC_JUMP_TAG(ec, TAG_RAISE);
50995095
}
51005096

0 commit comments

Comments
 (0)