Skip to content

Conversation

@peterzhu2118
Copy link
Member

We cannot free the weakmap_entry before the ST_DELETE because it could hash the key which would read the weakmap_entry and would cause a use-after-free. Instead, we store the entry and free it on the next iteration.

For example, the following script triggers a use-after-free in Valgrind:

weakmap = ObjectSpace::WeakMap.new
10_000.times { weakmap[Object.new] = Object.new }
==25795== Invalid read of size 8
==25795==    at 0x462297: wmap_cmp (weakmap.c:165)
==25795==    by 0x3A2B1C: find_table_bin_ind (st.c:930)
==25795==    by 0x3A5EAA: st_general_foreach (st.c:1599)
==25795==    by 0x3A5EAA: rb_st_foreach (st.c:1640)
==25795==    by 0x25C991: gc_mark_children (default.c:4870)
==25795==    by 0x25C991: gc_marks_wb_unprotected_objects_plane (default.c:5565)
==25795==    by 0x25C991: rgengc_rememberset_mark_plane (default.c:5557)
==25795==    by 0x25C991: rgengc_rememberset_mark (default.c:6233)
==25795==    by 0x25C991: gc_marks_start (default.c:6057)
==25795==    by 0x25C991: gc_marks (default.c:6077)
==25795==    by 0x25C991: gc_start (default.c:6723)
==25795==    by 0x260F96: heap_prepare (default.c:2282)
==25795==    by 0x260F96: heap_next_free_page (default.c:2489)
==25795==    by 0x260F96: newobj_cache_miss (default.c:2598)
==25795==    by 0x26197F: newobj_alloc (default.c:2622)
==25795==    by 0x26197F: rb_gc_impl_new_obj (default.c:2701)
==25795==    by 0x26197F: newobj_of (gc.c:890)
==25795==    by 0x26197F: rb_wb_protected_newobj_of (gc.c:917)
==25795==    by 0x2DEA88: rb_class_allocate_instance (object.c:131)
==25795==    by 0x2E3B18: class_call_alloc_func (object.c:2141)
==25795==    by 0x2E3B18: rb_class_alloc (object.c:2113)
==25795==    by 0x2E3B18: rb_class_new_instance_pass_kw (object.c:2172)
==25795==    by 0x429DDC: vm_call_cfunc_with_frame_ (vm_insnhelper.c:3786)
==25795==    by 0x44B08D: vm_sendish (vm_insnhelper.c:5953)
==25795==    by 0x44B08D: vm_exec_core (insns.def:898)
==25795==    by 0x43A7A4: rb_vm_exec (vm.c:2564)
==25795==    by 0x234914: rb_ec_exec_node (eval.c:281)
==25795==  Address 0x21603710 is 0 bytes inside a block of size 16 free'd
==25795==    at 0x4849B2C: free (vg_replace_malloc.c:989)
==25795==    by 0x249651: rb_gc_impl_free (default.c:8527)
==25795==    by 0x249651: rb_gc_impl_free (default.c:8508)
==25795==    by 0x249651: ruby_sized_xfree.constprop.0 (gc.c:4178)
==25795==    by 0x4626EC: ruby_sized_xfree_inlined (gc.h:277)
==25795==    by 0x4626EC: wmap_free_entry (weakmap.c:45)
==25795==    by 0x4626EC: wmap_mark_weak_table_i (weakmap.c:61)
==25795==    by 0x3A5CEF: apply_functor (st.c:1633)
==25795==    by 0x3A5CEF: st_general_foreach (st.c:1543)
==25795==    by 0x3A5CEF: rb_st_foreach (st.c:1640)
==25795==    by 0x25C991: gc_mark_children (default.c:4870)
==25795==    by 0x25C991: gc_marks_wb_unprotected_objects_plane (default.c:5565)
==25795==    by 0x25C991: rgengc_rememberset_mark_plane (default.c:5557)
==25795==    by 0x25C991: rgengc_rememberset_mark (default.c:6233)
==25795==    by 0x25C991: gc_marks_start (default.c:6057)
==25795==    by 0x25C991: gc_marks (default.c:6077)
==25795==    by 0x25C991: gc_start (default.c:6723)
==25795==    by 0x260F96: heap_prepare (default.c:2282)
==25795==    by 0x260F96: heap_next_free_page (default.c:2489)
==25795==    by 0x260F96: newobj_cache_miss (default.c:2598)
==25795==    by 0x26197F: newobj_alloc (default.c:2622)
==25795==    by 0x26197F: rb_gc_impl_new_obj (default.c:2701)
==25795==    by 0x26197F: newobj_of (gc.c:890)
==25795==    by 0x26197F: rb_wb_protected_newobj_of (gc.c:917)
==25795==    by 0x2DEA88: rb_class_allocate_instance (object.c:131)
==25795==    by 0x2E3B18: class_call_alloc_func (object.c:2141)
==25795==    by 0x2E3B18: rb_class_alloc (object.c:2113)
==25795==    by 0x2E3B18: rb_class_new_instance_pass_kw (object.c:2172)
==25795==    by 0x429DDC: vm_call_cfunc_with_frame_ (vm_insnhelper.c:3786)
==25795==    by 0x44B08D: vm_sendish (vm_insnhelper.c:5953)
==25795==    by 0x44B08D: vm_exec_core (insns.def:898)
==25795==    by 0x43A7A4: rb_vm_exec (vm.c:2564)
==25795==  Block was alloc'd at
==25795==    at 0x484680F: malloc (vg_replace_malloc.c:446)
==25795==    by 0x25CE9E: rb_gc_impl_malloc (default.c:8542)
==25795==    by 0x462A39: wmap_aset_replace (weakmap.c:423)
==25795==    by 0x3A5542: rb_st_update (st.c:1487)
==25795==    by 0x462B8E: wmap_aset (weakmap.c:452)
==25795==    by 0x429DDC: vm_call_cfunc_with_frame_ (vm_insnhelper.c:3786)
==25795==    by 0x44B08D: vm_sendish (vm_insnhelper.c:5953)
==25795==    by 0x44B08D: vm_exec_core (insns.def:898)
==25795==    by 0x43A7A4: rb_vm_exec (vm.c:2564)
==25795==    by 0x234914: rb_ec_exec_node (eval.c:281)
==25795==    by 0x2369B8: ruby_run_node (eval.c:319)
==25795==    by 0x15D675: rb_main (main.c:43)
==25795==    by 0x15D675: main (main.c:62)

[Bug #20688]

We cannot free the weakmap_entry before the ST_DELETE because it could
hash the key which would read the weakmap_entry and would cause a
use-after-free. Instead, we store the entry and free it on the next
iteration.

For example, the following script triggers a use-after-free in Valgrind:

    weakmap = ObjectSpace::WeakMap.new
    10_000.times { weakmap[Object.new] = Object.new }

    ==25795== Invalid read of size 8
    ==25795==    at 0x462297: wmap_cmp (weakmap.c:165)
    ==25795==    by 0x3A2B1C: find_table_bin_ind (st.c:930)
    ==25795==    by 0x3A5EAA: st_general_foreach (st.c:1599)
    ==25795==    by 0x3A5EAA: rb_st_foreach (st.c:1640)
    ==25795==    by 0x25C991: gc_mark_children (default.c:4870)
    ==25795==    by 0x25C991: gc_marks_wb_unprotected_objects_plane (default.c:5565)
    ==25795==    by 0x25C991: rgengc_rememberset_mark_plane (default.c:5557)
    ==25795==    by 0x25C991: rgengc_rememberset_mark (default.c:6233)
    ==25795==    by 0x25C991: gc_marks_start (default.c:6057)
    ==25795==    by 0x25C991: gc_marks (default.c:6077)
    ==25795==    by 0x25C991: gc_start (default.c:6723)
    ==25795==    by 0x260F96: heap_prepare (default.c:2282)
    ==25795==    by 0x260F96: heap_next_free_page (default.c:2489)
    ==25795==    by 0x260F96: newobj_cache_miss (default.c:2598)
    ==25795==    by 0x26197F: newobj_alloc (default.c:2622)
    ==25795==    by 0x26197F: rb_gc_impl_new_obj (default.c:2701)
    ==25795==    by 0x26197F: newobj_of (gc.c:890)
    ==25795==    by 0x26197F: rb_wb_protected_newobj_of (gc.c:917)
    ==25795==    by 0x2DEA88: rb_class_allocate_instance (object.c:131)
    ==25795==    by 0x2E3B18: class_call_alloc_func (object.c:2141)
    ==25795==    by 0x2E3B18: rb_class_alloc (object.c:2113)
    ==25795==    by 0x2E3B18: rb_class_new_instance_pass_kw (object.c:2172)
    ==25795==    by 0x429DDC: vm_call_cfunc_with_frame_ (vm_insnhelper.c:3786)
    ==25795==    by 0x44B08D: vm_sendish (vm_insnhelper.c:5953)
    ==25795==    by 0x44B08D: vm_exec_core (insns.def:898)
    ==25795==    by 0x43A7A4: rb_vm_exec (vm.c:2564)
    ==25795==    by 0x234914: rb_ec_exec_node (eval.c:281)
    ==25795==  Address 0x21603710 is 0 bytes inside a block of size 16 free'd
    ==25795==    at 0x4849B2C: free (vg_replace_malloc.c:989)
    ==25795==    by 0x249651: rb_gc_impl_free (default.c:8527)
    ==25795==    by 0x249651: rb_gc_impl_free (default.c:8508)
    ==25795==    by 0x249651: ruby_sized_xfree.constprop.0 (gc.c:4178)
    ==25795==    by 0x4626EC: ruby_sized_xfree_inlined (gc.h:277)
    ==25795==    by 0x4626EC: wmap_free_entry (weakmap.c:45)
    ==25795==    by 0x4626EC: wmap_mark_weak_table_i (weakmap.c:61)
    ==25795==    by 0x3A5CEF: apply_functor (st.c:1633)
    ==25795==    by 0x3A5CEF: st_general_foreach (st.c:1543)
    ==25795==    by 0x3A5CEF: rb_st_foreach (st.c:1640)
    ==25795==    by 0x25C991: gc_mark_children (default.c:4870)
    ==25795==    by 0x25C991: gc_marks_wb_unprotected_objects_plane (default.c:5565)
    ==25795==    by 0x25C991: rgengc_rememberset_mark_plane (default.c:5557)
    ==25795==    by 0x25C991: rgengc_rememberset_mark (default.c:6233)
    ==25795==    by 0x25C991: gc_marks_start (default.c:6057)
    ==25795==    by 0x25C991: gc_marks (default.c:6077)
    ==25795==    by 0x25C991: gc_start (default.c:6723)
    ==25795==    by 0x260F96: heap_prepare (default.c:2282)
    ==25795==    by 0x260F96: heap_next_free_page (default.c:2489)
    ==25795==    by 0x260F96: newobj_cache_miss (default.c:2598)
    ==25795==    by 0x26197F: newobj_alloc (default.c:2622)
    ==25795==    by 0x26197F: rb_gc_impl_new_obj (default.c:2701)
    ==25795==    by 0x26197F: newobj_of (gc.c:890)
    ==25795==    by 0x26197F: rb_wb_protected_newobj_of (gc.c:917)
    ==25795==    by 0x2DEA88: rb_class_allocate_instance (object.c:131)
    ==25795==    by 0x2E3B18: class_call_alloc_func (object.c:2141)
    ==25795==    by 0x2E3B18: rb_class_alloc (object.c:2113)
    ==25795==    by 0x2E3B18: rb_class_new_instance_pass_kw (object.c:2172)
    ==25795==    by 0x429DDC: vm_call_cfunc_with_frame_ (vm_insnhelper.c:3786)
    ==25795==    by 0x44B08D: vm_sendish (vm_insnhelper.c:5953)
    ==25795==    by 0x44B08D: vm_exec_core (insns.def:898)
    ==25795==    by 0x43A7A4: rb_vm_exec (vm.c:2564)
    ==25795==  Block was alloc'd at
    ==25795==    at 0x484680F: malloc (vg_replace_malloc.c:446)
    ==25795==    by 0x25CE9E: rb_gc_impl_malloc (default.c:8542)
    ==25795==    by 0x462A39: wmap_aset_replace (weakmap.c:423)
    ==25795==    by 0x3A5542: rb_st_update (st.c:1487)
    ==25795==    by 0x462B8E: wmap_aset (weakmap.c:452)
    ==25795==    by 0x429DDC: vm_call_cfunc_with_frame_ (vm_insnhelper.c:3786)
    ==25795==    by 0x44B08D: vm_sendish (vm_insnhelper.c:5953)
    ==25795==    by 0x44B08D: vm_exec_core (insns.def:898)
    ==25795==    by 0x43A7A4: rb_vm_exec (vm.c:2564)
    ==25795==    by 0x234914: rb_ec_exec_node (eval.c:281)
    ==25795==    by 0x2369B8: ruby_run_node (eval.c:319)
    ==25795==    by 0x15D675: rb_main (main.c:43)
    ==25795==    by 0x15D675: main (main.c:62)
[Bug #20688]

We cannot free the key before the ST_DELETE because it could hash the
key which would read the key and would cause a use-after-free. Instead,
we store the key and free it on the next iteration.
@k0kubun k0kubun merged commit 8657de7 into ruby:ruby_3_3 Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants