WIP: memory_view: finalizer GC safety issue #5012

XrXr · 2021-10-22T22:42:50Z

The finalizer of Fiddle::memory_view uses rb_memory_view_release(),
which traverses the super chain to find the memory view entry. Trouble
is, while finalizing object x, it's unsafe to access any object other
than x. Depending on finalization order, other objects might have been
finalized and released already.

This memory safety issue manifests as a crash on my machine reliably
when I run the following:

make test-all TESTS='test/fiddle/test_memory_view.rb -n/test_memory_view_from_pointer/'

On CI, there are many instances of this crash. For example:

This change checks for object liveness before access, however, I don't
think this fix is completely correct while it does avoid the crash.

In the event that the klass which stores the memory view entry is
finalized before rb_memory_view_release is called, the memory view entry
will be leaked. Also, it's possible that during finalization, the
address of the super class has been repurposed for a different object.

It seems that memory view needs to handle double-free coming from the
above situation. One free comes the class storing the entry going away,
and another when the Fiddle object finalizes.

cc: @ko1 @mrkn

The finalizer of Fiddle::memory_view uses rb_memory_view_release(), which traverses the super chain to find the memory view entry. Trouble is, while finalizing object x, it's unsafe to access any object other than x. Depending on finalization order, other objects might have been finalized and released already. This memory safety issue manifests as a crash on my machine reliably when I run the following: make test-all TESTS='test/fiddle/test_memory_view.rb -n/test_memory_view_from_pointer/' On CI, there are many instances of this crash. For example: - http://rubyci.s3.amazonaws.com/osx1015/ruby-master/log/20211022T114502Z.fail.html.gz - http://rubyci.s3.amazonaws.com/solaris10-sunc/ruby-master/log/20211022T090002Z.fail.html.gz This change checks for object liveness before access, however, I don't think this fix is completely correct while it does avoid the crash. In the event that the klass which stores the memory view entry is finalized before rb_memory_view_release is called, the memory view entry will be leaked. Also, it's possible that during finalization, the address of the super class has been repurposed for a different object. It seems that memory view needs to handle double-free coming from the above situation. One free comes the class storing the entry going away, and another when the Fiddle object finalizes.

XrXr · 2021-10-22T22:57:39Z

memory_view.c

@@ -781,8 +782,16 @@ rb_memory_view_get_item(rb_memory_view_t *view, const ssize_t *indices)
 static const rb_memory_view_entry_t *
 lookup_memory_view_entry(VALUE klass)
 {
+    RUBY_ASSERT(rb_objspace_markable_object_p(klass));


Even this assert fails. The "touch other objects during finalization" issue extends one level up to fiddle_memview_release(). When the wrapper object is finalizing, view->obj is already gone and T_NONE.

kou · 2021-10-23T02:59:47Z

ruby/fiddle#79 may be related.

nobu · 2021-10-25T08:46:21Z

memory_view.c

@@ -781,8 +782,16 @@ rb_memory_view_get_item(rb_memory_view_t *view, const ssize_t *indices)
 static const rb_memory_view_entry_t *
 lookup_memory_view_entry(VALUE klass)
 {
+    RUBY_ASSERT(rb_objspace_markable_object_p(klass));


In that case, we can and should do nothing?

Suggested change

RUBY_ASSERT(rb_objspace_markable_object_p(klass));

if (!rb_objspace_markable_object_p(klass)) return NULL;

We could, but I don't think it covers all the cases. Since it's a stale pointer, it could be a live object and a completely unrelated class. We could leak or even end up getting an unrelated rb_memory_view_entry_t and use a bad release function.

Do we need to free zombies after finalization calls?

diff --git a/gc.c b/gc.c index 0c739ba709b..47f5cfc5b2a 100644 --- a/gc.c +++ b/gc.c @@ -4069,12 +4069,10 @@ run_final(rb_objspace_t *objspace, VALUE zombie) static void finalize_list(rb_objspace_t *objspace, VALUE zombie) { + VALUE next_zombie; while (zombie) { - VALUE next_zombie; - struct heap_page *page; asan_unpoison_object(zombie, false); next_zombie = RZOMBIE(zombie)->next; - page = GET_HEAP_PAGE(zombie); run_final(objspace, zombie); @@ -4084,20 +4082,28 @@ finalize_list(rb_objspace_t *objspace, VALUE zombie) if (FL_TEST(zombie, FL_SEEN_OBJ_ID)) { obj_free_object_id(objspace, zombie); } - - GC_ASSERT(heap_pages_final_slots > 0); - GC_ASSERT(page->final_slots > 0); - - heap_pages_final_slots--; - page->final_slots--; - page->free_slots++; - heap_page_add_freeobj(objspace, page, zombie); - objspace->profile.total_freed_objects++; } RB_VM_LOCK_LEAVE(); zombie = next_zombie; } + + RB_VM_LOCK_ENTER(); + while (zombie) { + struct heap_page *page; + next_zombie = RZOMBIE(zombie)->next; + page = GET_HEAP_PAGE(zombie); + GC_ASSERT(heap_pages_final_slots > 0); + GC_ASSERT(page->final_slots > 0); + + heap_pages_final_slots--; + page->final_slots--; + page->free_slots++; + heap_page_add_freeobj(objspace, page, zombie); + objspace->profile.total_freed_objects++; + zombie = next_zombie; + } + RB_VM_LOCK_LEAVE(); } static void

Ah, that fixes the crash on my machine. I was thinking of adding a rb_memory_view_entry_t * to rb_memory_view_t to avoid the lookup in rb_memory_view_release() but maybe with this change that won't be necessary.

zombie must be reset before the second loop.

diff --git a/gc.c b/gc.c index bd3915fb470..c0bcbfb2a5b 100644 --- a/gc.c +++ b/gc.c @@ -4068,12 +4068,10 @@ run_final(rb_objspace_t *objspace, VALUE zombie) static void finalize_list(rb_objspace_t *objspace, VALUE zombie) { + VALUE next_zombie, first_zombie = zombie; while (zombie) { - VALUE next_zombie; - struct heap_page *page; asan_unpoison_object(zombie, false); next_zombie = RZOMBIE(zombie)->next; - page = GET_HEAP_PAGE(zombie); run_final(objspace, zombie); @@ -4083,20 +4081,29 @@ finalize_list(rb_objspace_t *objspace, VALUE zombie) if (FL_TEST(zombie, FL_SEEN_OBJ_ID)) { obj_free_object_id(objspace, zombie); } - - GC_ASSERT(heap_pages_final_slots > 0); - GC_ASSERT(page->final_slots > 0); - - heap_pages_final_slots--; - page->final_slots--; - page->free_slots++; - heap_page_add_freeobj(objspace, page, zombie); - objspace->profile.total_freed_objects++; } RB_VM_LOCK_LEAVE(); zombie = next_zombie; } + + zombie = first_zombie; + RB_VM_LOCK_ENTER(); + while (zombie) { + struct heap_page *page; + next_zombie = RZOMBIE(zombie)->next; + page = GET_HEAP_PAGE(zombie); + GC_ASSERT(heap_pages_final_slots > 0); + GC_ASSERT(page->final_slots > 0); + + heap_pages_final_slots--; + page->final_slots--; + page->free_slots++; + heap_page_add_freeobj(objspace, page, zombie); + objspace->profile.total_freed_objects++; + zombie = next_zombie; + } + RB_VM_LOCK_LEAVE(); } static void

zombie must be reset before the second loop.

I tried the new patch and it also fixes the crash.

eregon · 2021-10-25T09:42:26Z

The finalizer of Fiddle::memory_view uses rb_memory_view_release(),
which traverses the super chain to find the memory view entry. Trouble
is, while finalizing object x, it's unsafe to access any object other
than x. Depending on finalization order, other objects might have been
finalized and released already.

Shouldn't this work fine if the GC is correct and consider references that the finalizer can access?
I think that's how it'd work e.g. on JVM.

Also accessing x while it's being finalized is a problem in itself (it would never be finalized, AFAIK also an issue on CRuby). So finalizers always need to capture just what they need, and not the object itself.

XrXr · 2021-10-25T16:22:29Z

Shouldn't this work fine if the GC is correct and consider references that the finalizer can access?

Sorry, I'm using the terminology loosely here. I think the normal Ruby finalizers does allow for accessing other objects, but the callbacks I'm talking about in the ticket are the ones for doing deallocation exposed in the C API. These callbacks have different constraints from Ruby level finalizers.

eregon · 2021-10-27T18:06:40Z

Rereading a bit more about the issue, maybe Fiddle::MemoryView shouldn't try to autorelease (or if it does use a Ruby-level finalizer)?
I think autorelease is anyway a bad pattern, efficient resource handling requires explicitly releasing (e.g., at the end of a block) and not waiting for the GC to do it much later if ever.

kou · 2021-10-28T01:27:37Z

I think autorelease is anyway a bad pattern, efficient resource handling requires explicitly releasing (e.g., at the end of a block) and not waiting for the GC to do it much later if ever.

ruby/fiddle#80 is for it.

mrkn · 2021-11-06T07:03:10Z

I consulted with @ko1 about how to fix this issue yesterday.
As the consequence, we found that we can stop looking up memory view entry in class at rb_memory_view_release.
I'm trying this approach in #5088.

XrXr · 2021-11-06T16:20:53Z

Avoiding the lookup altogether sounds good to me since I also had the same idea: #5012 (comment) :). Closing this in favor of #5088.

To stop looking up the memory_view_entry from the class of the original object of a memory_view in rb_memory_view_release, a pointer of rb_memory_view_entry_t is added as a member of rb_memory_view_t. Fix rubyGH-5012

XrXr commented Oct 22, 2021

View reviewed changes

nobu reviewed Oct 25, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

mrkn mentioned this pull request Nov 6, 2021

memory_view.c: Add _memory_view_entry member in rb_memory_view_t #5088

Merged

XrXr closed this Nov 6, 2021

XrXr deleted the memview-bug branch November 6, 2021 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: memory_view: finalizer GC safety issue #5012

WIP: memory_view: finalizer GC safety issue #5012

XrXr commented Oct 22, 2021 •

edited

Loading

XrXr Oct 22, 2021 •

edited

Loading

kou commented Oct 23, 2021

nobu Oct 25, 2021

XrXr Oct 25, 2021

nobu Oct 26, 2021

XrXr Oct 26, 2021 •

edited

Loading

nobu Oct 27, 2021

XrXr Oct 27, 2021

eregon commented Oct 25, 2021

XrXr commented Oct 25, 2021

eregon commented Oct 27, 2021 •

edited

Loading

kou commented Oct 28, 2021

This comment has been minimized.

This comment has been minimized.

mrkn commented Nov 6, 2021

XrXr commented Nov 6, 2021

	RUBY_ASSERT(rb_objspace_markable_object_p(klass));
	if (!rb_objspace_markable_object_p(klass)) return NULL;

WIP: memory_view: finalizer GC safety issue #5012

WIP: memory_view: finalizer GC safety issue #5012

Conversation

XrXr commented Oct 22, 2021 • edited Loading

XrXr Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

kou commented Oct 23, 2021

nobu Oct 25, 2021

Choose a reason for hiding this comment

XrXr Oct 25, 2021

Choose a reason for hiding this comment

nobu Oct 26, 2021

Choose a reason for hiding this comment

XrXr Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

nobu Oct 27, 2021

Choose a reason for hiding this comment

XrXr Oct 27, 2021

Choose a reason for hiding this comment

eregon commented Oct 25, 2021

XrXr commented Oct 25, 2021

eregon commented Oct 27, 2021 • edited Loading

kou commented Oct 28, 2021

This comment has been minimized.

This comment has been minimized.

mrkn commented Nov 6, 2021

XrXr commented Nov 6, 2021

XrXr commented Oct 22, 2021 •

edited

Loading

XrXr Oct 22, 2021 •

edited

Loading

XrXr Oct 26, 2021 •

edited

Loading

eregon commented Oct 27, 2021 •

edited

Loading