symbol.c: copy the name of a symbol that symbol GC can free - #7020

Symbol#to_s and Symbol#name return a string that shares the symbol's name buffer,
which symbol GC can free.

For a dynamic symbol whose name is longer than RSTRING_EMBED_LEN_MAX, Symbol#to_s
returns a RSTR_NOFREE string pointing straight into the symbol table's name buffer.
Symbol GC does not treat such a string as a reference to the symbol, so it can sweep the
symbol and mrb_free() the buffer while the string is still alive. Every later read of
that string is a use after free.

str = ("q" * 40).to_sym.to_s
6000.times { |i| "junk-symbol-with-a-long-name-#{i}".to_sym }
GC.start
a = []; 3000.times { |i| a << "Z" * 60 + i.to_s }
p str

The exact garbage varies by run: it is whatever allocation happened to reuse the block.
Here the freed buffer was taken by a later symbol's packed name, so the string reports
that symbol's name plus the packed length byte.

Symbol#name has the same problem, and so does anything that keeps the result of
mrb_obj_as_string() for a symbol. MatchData makes it easy to hit by accident, because
it keeps the subject string for the lifetime of $~:

md = ("target-" + "b" * 30).to_sym.to_s.match(/target/)
# ... same symbol churn ...
p md.string       #=> "junk-symbol-with-a-long-name-4097\x00bbb"
p md.post_match   #=> "ymbol-with-a-long-name-4097\x00bbb"

Cause

mrb_sym_str() hands the raw name pointer to mrb_str_new_static(), and
str_new_static() copies only when the name fits inline:

if (RSTR_EMBEDDABLE_P(len)) {
  return str_init_embed(mrb_obj_alloc_string(mrb), p, len);
}
return str_init_nofree(mrb_obj_alloc_string(mrb), p, len);

That was safe while symbol names lived forever. It no longer is: a dynamic symbol gets an
individual mrb_malloc() and symbol GC frees it in phase 3 of mrb_symbol_gc():

mrb_free(mrb, (void*)mrb->symtbl[i]);
mrb->symtbl[i] = NULL;  /* tombstone */

Nothing connects the two. sym_gc_mark_object() walks classes, ivars, arrays, hashes, env
and the stack looking for mrb_symbol_p() values, and MRB_TT_STRING falls into its
default: break;. A string is the one kind of object that can keep a symbol's name alive
without holding the symbol, and it is exactly the case that is not scanned.

The name length is what decides between safe and corrupt, at RSTRING_EMBED_LEN_MAX
(27 on a 64-bit build):

MRB_SYMBOL_MAX defaults to 4096, so a default build is affected; only MRB_SYMBOL_MAX 0
is immune. Presym and literal symbols are safe, because their names are either static data
or pool allocated and are never individually freed.

Fix

Copy the name in mrb_sym_str() and sym_name() when the symbol is one symbol GC can
reclaim, and keep the zero copy path for everything else. Only a dynamic symbol gets an
individual allocation: a presym name is static data, a literal name comes from the symbol
pool and an inline symbol carries its name in the value, so none of those is ever freed.

The cost is one allocation per to_s of a long dynamic symbol. Short names were already
copied by str_init_embed(), and symbols from source literals, which is nearly all of
them in practice, keep sharing the buffer.

Alternatives considered

Marking from the string side would mean recognising, during symbol GC, that a RSTR_NOFREE
string's pointer lands inside the symbol table, then mapping it back to a symbol index.
That is a linear scan per string over mrb->symtbl for a case that copying avoids
outright, and it does not compose with str_init_nofree() strings that legitimately point
at static data.

Freezing the returned string does not help either: the problem is the lifetime of the
buffer, not mutation of the string.

Where the fix belongs

This came up twice in review, on #6993 and on #6995, both times as "a MatchData holding a
symbol derived string can outlive the symbol". Neither PR was the right place. The same use
after free reproduces with Symbol#to_s alone and no regexp anywhere, and patching
match_operand() in mruby-regexp would close the Regexp#match(:sym) route while leaving
Symbol#to_s, Symbol#name, "#{sym}" and every other mrb_obj_as_string() caller open.

Verification

test/t/symbol.rb gains a test that fails on master and passes with the patch:

Fail: Symbol#to_s and Symbol#name outlive symbol GC (core)
 - Assertion[1]
    Expected: "gc-target-symbol-aaaaaaaaaaaaaaaaaaaaaaaa"
      Actual: "gc-filler-symbol-name-4959\x00aaaaaaaaaaaaaa"

rake test passes on a plain host build (1943 assertions, 0 KO, 0 crash) and on
build_config/clang-asan.rb (2120 assertions, 0 KO, 0 crash).

Under ASan the use after free is reported directly. The shortest repro needs a small
MRB_SYMBOL_MAX so the sweep runs early, and in that shape the tombstone guard from #7018
has to be in place first, otherwise the null deref in migrate_to_hash_table() lands
before this bug can be read back. With MRB_SYMBOL_MAX=16 and that guard applied:

s = ("dynamic-target-name-" + "a" * 12).to_sym.to_s
40.times { |i| "filler-symbol-name-#{i}".to_sym }
p s

==236534==ERROR: AddressSanitizer: heap-use-after-free on address 0x7c0f05fe05d1
READ of size 1 at 0x7c0f05fe05d1 thread T0
    #0 str_escape        src/string.c:1586
    #1 mrb_str_inspect   src/string.c:3240
    #2 mrb_vm_exec       src/vm.c:2947
    ...
freed by thread T0 here:
    #2 mrb_free          src/gc.c:387
    #3 mrb_symbol_gc     src/symbol.c:838
    #4 sym_intern        src/symbol.c:428
    #5 mrb_intern        src/symbol.c:465
    #6 mrb_intern_str    src/symbol.c:512

With the patch the same build prints "dynamic-target-name-aaaaaaaaaaaa" and ASan is
silent. The two changes are independent; only that shortcut repro is shared.

Summary by CodeRabbit

• Bug Fixes

  • Fixed Symbol#to_s and Symbol#name so returned strings remain valid after dynamic symbol garbage collection and memory reuse.
  • Improved handling of symbol names to prevent returned strings from referencing reclaimable storage.

• Tests

  • Added regression coverage for symbol string validity after garbage collection.