Skip to content

Commit

Permalink
Don't allow re-embedding frozen arrays
Browse files Browse the repository at this point in the history
Frozen arrays should not move from heap allocated to embedded because
frozen arrays could be shared roots for other (shared) arrays. If the
frozen array moves from heap allocated to embedded it would cause issues
since the shared array would no longer know where to set the pointer
in the shared root.
  • Loading branch information
peterzhu2118 committed Dec 23, 2022
1 parent d61a4ce commit 7891f94
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 2 deletions.
11 changes: 9 additions & 2 deletions array.c
Expand Up @@ -226,8 +226,15 @@ ary_embeddable_p(long capa)
bool
rb_ary_embeddable_p(VALUE ary)
{
// if the array is shared or a shared root then it's not moveable
return !(ARY_SHARED_P(ary) || ARY_SHARED_ROOT_P(ary));
/* An array cannot be turned embeddable when the array is:
* - Shared root: other objects may point to the buffer of this array
* so we cannot make it embedded.
* - Frozen: this array may also be a shared root without the shared root
* flag.
* - Shared: we don't want to re-embed an array that points to a shared
* root (to save memory).
*/
return !(ARY_SHARED_ROOT_P(ary) || OBJ_FROZEN(ary) || ARY_SHARED_P(ary));
}

size_t
Expand Down
26 changes: 26 additions & 0 deletions test/ruby/test_gc_compact.rb
Expand Up @@ -209,6 +209,32 @@ def obj.tracee
assert_equal([:call, :line], results)
end

def test_updating_references_for_heap_allocated_frozen_shared_arrays
assert_separately(%w[-robjspace], "#{<<~"begin;"}\n#{<<~"end;"}", timeout: 10, signal: :SEGV)
begin;
ary = []
50.times { |i| ary << i }
# Frozen arrays can become shared root without RARRAY_SHARED_ROOT_FLAG
ary.freeze
slice = ary[10..40]
# Check that slice is pointing to buffer of ary
assert_include(ObjectSpace.dump(slice), '"shared":true')
# Run compaction to re-embed ary
GC.verify_compaction_references(expand_heap: true, toward: :empty)
# Assert that slice is pointer to updated buffer in ary
assert_equal(10, slice[0])
# Check that slice is still pointing to buffer of ary
assert_include(ObjectSpace.dump(slice), '"shared":true')
end;
end

end;
end

def test_moving_arrays_down_size_pools
omit if GC::INTERNAL_CONSTANTS[:SIZE_POOL_COUNT] == 1
assert_separately([], "#{<<~"begin;"}\n#{<<~"end;"}", timeout: 10, signal: :SEGV)
Expand Down

0 comments on commit 7891f94

Please sign in to comment.