From 674eb7df7f409099f33da77293d9658e09b470d6 Mon Sep 17 00:00:00 2001 From: Peter Zhu Date: Mon, 4 Dec 2023 14:02:56 -0500 Subject: [PATCH] Make env_copy compaction safe The original order of events is: 1. Allocate env_body. 2. Fill env_body using elements in src_env, and it performs operations that can trigger a GC. 3. Create the copied_env using vm_env_new. However, if GC compaction runs during step 2, then copied_env would not have yet been created and objects on env_body could move but it would not be reference updated. This commit changes the the order to be (1), (3), (2). --- vm.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/vm.c b/vm.c index 74f6cc2971..4cc19f7c23 100644 --- a/vm.c +++ b/vm.c @@ -1218,6 +1218,13 @@ env_copy(const VALUE *src_ep, VALUE read_only_variables) VALUE *env_body = ZALLOC_N(VALUE, src_env->env_size); // fill with Qfalse VALUE *ep = &env_body[src_env->env_size - 2]; + ep[VM_ENV_DATA_INDEX_ME_CREF] = src_ep[VM_ENV_DATA_INDEX_ME_CREF]; + ep[VM_ENV_DATA_INDEX_FLAGS] = src_ep[VM_ENV_DATA_INDEX_FLAGS] | VM_ENV_FLAG_ISOLATED; + if (!VM_ENV_LOCAL_P(src_ep)) { + VM_ENV_FLAGS_SET(ep, VM_ENV_FLAG_LOCAL); + } + + const rb_env_t *copied_env = vm_env_new(ep, env_body, src_env->env_size, src_env->iseq); volatile VALUE prev_env = Qnil; if (read_only_variables) { @@ -1237,7 +1244,7 @@ env_copy(const VALUE *src_ep, VALUE read_only_variables) rb_str_cat_cstr(msg, "a hidden variable"); rb_exc_raise(rb_exc_new_str(rb_eRactorIsolationError, msg)); } - env_body[j] = v; + RB_OBJ_WRITE((VALUE)copied_env, &env_body[j], v); rb_ary_delete_at(read_only_variables, i); break; } @@ -1245,20 +1252,17 @@ env_copy(const VALUE *src_ep, VALUE read_only_variables) } } - ep[VM_ENV_DATA_INDEX_ME_CREF] = src_ep[VM_ENV_DATA_INDEX_ME_CREF]; - ep[VM_ENV_DATA_INDEX_FLAGS] = src_ep[VM_ENV_DATA_INDEX_FLAGS] | VM_ENV_FLAG_ISOLATED; - if (!VM_ENV_LOCAL_P(src_ep)) { const VALUE *prev_ep = VM_ENV_PREV_EP(src_env->ep); const rb_env_t *new_prev_env = env_copy(prev_ep, read_only_variables); prev_env = (VALUE)new_prev_env; ep[VM_ENV_DATA_INDEX_SPECVAL] = VM_GUARDED_PREV_EP(new_prev_env->ep); + VM_ENV_FLAGS_UNSET(ep, VM_ENV_FLAG_LOCAL); } else { ep[VM_ENV_DATA_INDEX_SPECVAL] = VM_BLOCK_HANDLER_NONE; } - const rb_env_t *copied_env = vm_env_new(ep, env_body, src_env->env_size, src_env->iseq); RB_GC_GUARD(prev_env); return copied_env; }