Bug #19529: [BUG] ObjectSpace::WeakMap can segfault after compaction

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 3.2.1, 2.7.5p203
• Backport: 2.7: WONTFIX, 3.0: REQUIRED, 3.1: DONE, 3.2: DONE

[byroot (Jean Boussier)]

Reproduction script:

wm = ObjectSpace::WeakMap.new
obj = Object.new
100.times do |i|
  GC.compact
  wm[i] = obj #  [BUG] Segmentation fault at 0x0000000000000001
end

Crash report:

/tmp/weakmap.rb:5: [BUG] Segmentation fault at 0x0000000000000003
ruby 3.2.1 (2023-02-08 revision 31819e82c8) [arm64-darwin22]

-- Crash Report log information --------------------------------------------
   See Crash Report log file in one of the following locations:             
     * ~/Library/Logs/DiagnosticReports                                     
     * /Library/Logs/DiagnosticReports                                      
   for more details.                                                        
Don't forget to include the above Crash Report log file in bug reports.     

-- Control frame information -----------------------------------------------
c:0005 p:---- s:0023 e:000022 CFUNC  :[]=
c:0004 p:0016 s:0017 e:000015 BLOCK  /tmp/weakmap.rb:5 [FINISH]
c:0003 p:---- s:0012 e:000011 CFUNC  :times
c:0002 p:0017 s:0008 E:0025b0 EVAL   /tmp/weakmap.rb:3 [FINISH]
c:0001 p:0000 s:0003 E:0010b0 DUMMY  [FINISH]

-- Ruby level backtrace information ----------------------------------------
/tmp/weakmap.rb:3:in `<main>'
/tmp/weakmap.rb:3:in `times'
/tmp/weakmap.rb:5:in `block in <main>'
/tmp/weakmap.rb:5:in `[]='

-- Machine register context ------------------------------------------------
  x0: 0x000000016da8a6b0  x1: 0x000000016da8a6a8  x2: 0x000000000000003d
  x3: 0x0000000000000001  x4: 0x0000000000000000  x5: 0x0000000000000b00
  x6: 0x0000600002db4b00  x7: 0x0000000000000000 x18: 0x0000000000000000
 x19: 0x000000016da8a6a8 x20: 0x000000000000003d x21: 0x0000000000000003
 x22: 0x0000000000000001 x23: 0x00000001023df7b8 x24: 0x0000000000000001
 x25: 0x0000000000000000 x26: 0x00006000027bdf80 x27: 0xffffffffffffffff
 x28: 0xffffffffffffffff  lr: 0x0000000102ca28f0  fp: 0x000000016da8a680
  sp: 0x000000016da8a640

-- C level backtrace information -------------------------------------------
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_vm_bugreport+0x9a0) [0x102d3ba98]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_bug_for_fatal_signal+0x160) [0x102b5fe14]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(sig_do_nothing+0x0) [0x102c9a354]
/usr/lib/system/libsystem_platform.dylib(_sigtramp+0x38) [0x19b63c2a4]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_st_update+0x328) [0x102ca28f0]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_st_update+0x328) [0x102ca28f0]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(wmap_aset+0x90) [0x102b88e9c]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(vm_call_cfunc_with_frame+0xe8) [0x102d2fc7c]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(vm_sendish+0x4cc) [0x102d31fdc]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(vm_exec_core+0x239c) [0x102d136c8]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_vm_exec+0xad4) [0x102d26bf0]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(invoke_block_from_c_bh+0x398) [0x102d36438]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_yield_1+0x7c) [0x102d1f208]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(int_dotimes+0x148) [0x102bff3ec]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(vm_call_cfunc_with_frame+0xe8) [0x102d2fc7c]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(vm_sendish+0x4cc) [0x102d31fdc]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(vm_exec_core+0x2350) [0x102d1367c]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_vm_exec+0xad4) [0x102d26bf0]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(rb_ec_exec_node+0x12c) [0x102b6b4ac]
/opt/rubies/3.2.1/lib/libruby.3.2.dylib(ruby_run_node+0x60) [0x102b6b318]
/opt/rubies/3.2.1/bin/ruby(main+0x68) [0x102377f34]

Seems to happens since 2.7.

[byroot (Jean Boussier)]

Looking at the code, the dcompact function is wrong:

static void
wmap_compact(void *ptr)
{
    struct weakmap *w = ptr;
    if (w->wmap2obj) rb_gc_update_tbl_refs(w->wmap2obj);
    if (w->obj2wmap) rb_gc_update_tbl_refs(w->obj2wmap);
    w->final = rb_gc_location(w->final);
}

w->obj2wmap is not a VALUE -> VALUE st_table, it's a VALUE -> VALUE[] where the first element is the size:

static int
wmap_aset_update(st_data_t *key, st_data_t *val, st_data_t arg, int existing)
{
    VALUE size, *ptr, *optr;
    if (existing) {
        size = (ptr = optr = (VALUE *)*val)[0];
        ++size;
        SIZED_REALLOC_N(ptr, VALUE, size + 1, size);
    }
    else {
        optr = 0;
        size = 1;
        ptr = ruby_xmalloc(2 * sizeof(VALUE));
    }
    ptr[0] = size;
    ptr[size] = (VALUE)arg;
    if (ptr == optr) return ST_STOP;
    *val = (st_data_t)ptr;
    return ST_CONTINUE;
}

[byroot (Jean Boussier)]

Proposed patch: https://github.com/ruby/ruby/pull/7518

[byroot (Jean Boussier)]

• Status changed from Open to Closed

Applied in changeset git|548086b34e3dd125edabf5dc1e46b891fad3ea9c.

---

ObjectSpace::WeakMap: fix compaction support

[Bug #19529]

rb_gc_update_tbl_refs can't be used on w->obj2wmap because it's
not a VALUE -> VALUE table, but a VALUE -> VALUE * table, so
we need some dedicated iterator.

[peterzhu2118 (Peter Zhu)]

To the backport manager: please ensure to also backport commit 3dc8cde70078ccb38f5f4b0818ad5eecded01bd5 because the fix above has a bug.

[peterzhu2118 (Peter Zhu)]

To the backport manager: in addition to the two commits above, please also ensure to backport commit e0cf80d666d4b5df3229f030a16d10d21323508e because there could be crashes in compaction.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.7: WONTFIX, 3.0: REQUIRED, 3.1: REQUIRED, 3.2: REQUIRED to 2.7: WONTFIX, 3.0: REQUIRED, 3.1: DONE, 3.2: REQUIRED

ruby_3_1 cd4a997e53a02e5a8b24e38a10ba5525c176980e merged revision(s) 548086b34e3dd125edabf5dc1e46b891fad3ea9c,3dc8cde70078ccb38f5f4b0818ad5eecded01bd5,e0cf80d666d4b5df3229f030a16d10d21323508e.

[jprokop (Jarek Prokop)]

Please consider adding a guard to the test that comes with https://github.com/ruby/ruby/commit/3dc8cde70078ccb38f5f4b0818ad5eecded01bd5 .

There can be a case of not compiling with compaction, which we do with Ruby 3.1 on Fedora for ppc64le due to another bug.
In the case of unsupported compaction, the test failure looks like so:

1) Error:
TestWeakMap#test_compaction_bug_19529:
NotImplementedError: compact() function is unimplemented on this machine

Since we patch [0] Ruby to not implement the methods unless a compilation-time flag is defined, we see NotImplementedError and therefore can do a simple

  def test_compaction_bug_19529
    skip('Compaction not supported') unless GC.respond_to?(:compact)

in the offending test.

But maybe something similar to the CompactionSupportInspector in test/ruby/test_gc_compact.rb is better for the overall consistency in tests: https://github.com/ruby/ruby/blob/f269fae07e68117cd4ab42d889136ea37e6d3913/test/ruby/test_gc_compact.rb#L12

[0] https://src.fedoraproject.org/rpms/ruby/blob/86c35ad5cfca4e73134e6438295c68beeb1ef2cf/f/ruby-3.2.0-define-unsupported-gc-compaction-methods-as-rb_f_notimplement.patch

[peterzhu2118 (Peter Zhu)]

@jprokop (Jarek Prokop) Thanks for letting us know. I fixed it in bffadcd6d46ccfccade79ce0efb60ced8eac4483.

[byroot (Jean Boussier)]

Hum, this raise the question of wether GC.compact should actually be undef when it's not supported.

I think in general you don't care much if it's supported or not, you can call it and worst case it's a noop. Very few code out there expect it not to be there.

[Eregon (Benoit Daloze)]

byroot (Jean Boussier) wrote in #note-9:

Hum, this raise the question of wether GC.compact should actually be undef when it's not supported.

That's already the case, right? (undef if unsupported)

I think in general you don't care much if it's supported or not, you can call it and worst case it's a noop. Very few code out there expect it not to be there.

The docs say:

To test whether GC compaction is supported, use the idiom:

  GC.respond_to?(:compact)

So it doesn't seem good to break that.
Also old Rubies and TruffleRuby don't define it for instance, and it doesn't seem entirely right to make it a noop as it would ignore the user intent.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.7: WONTFIX, 3.0: REQUIRED, 3.1: DONE, 3.2: REQUIRED to 2.7: WONTFIX, 3.0: REQUIRED, 3.1: DONE, 3.2: DONE

ruby_3_2 3ebcbb537d7ae37e49c49e05b28d2cc5b324f151 merged revision(s) 548086b34e3dd125edabf5dc1e46b891fad3ea9c,3dc8cde70078ccb38f5f4b0818ad5eecded01bd5,e0cf80d666d4b5df3229f030a16d10d21323508e.

[nagachika (Tomoyuki Chikanaga)]

ruby_3_2 e3d10dedf106357d2b17fc8786d35035e0b366cf merged revision(s) bffadcd6d46ccfccade79ce0efb60ced8eac4483.