mruby-regexp: snapshot the subject string in create_matchdata() - #7053

MatchData stores the subject string by reference and computes #[], #pre_match, #post_match and friends lazily from it, so mutating that string afterwards retroactively changes what an already built MatchData reports.

s = "hello"
s =~ /l/
s.upcase!
$~[0]        # CRuby: "l",     mruby: "L"
$~.string    # CRuby: "hello", mruby: "HELLO"

CRuby snapshots the subject with rb_str_new_frozen(), so $~ always describes the string as it was at match time, and $~.string is frozen.

Every accessor that reads the subject diverges, and on an MRB_UTF8_STRING build #begin and #end go with them, since they read it to convert a byte offset to a character offset.

$&, $`, $' and $+ survive that particular sequence, because the path that sets them computes them through re_byte_substr(), which copies the bytes at match time. They are not immune in general: matchdata_set_globals() recomputes them from md->source, and the String#gsub loop in mrbgems/mruby-regexp/mrblib/string_regexp.rb calls it after the user block has run.

t = "hello"
n = 0
t.gsub(/l/) { n += 1; t.upcase! if n == 2; "X" }
$&           # CRuby: nil, mruby: "L", where the match was "l"

It is also a trap for the destructive string methods. One that matches self and then overwrites it with replace would leave behind a $~ describing the replaced string. Not reproducible today, since sub! and gsub! still raise TypeError for a Regexp pattern, but any implementation of them would have to know about it.

Cause

create_matchdata() assigns md->source = str without copying, and sets the source instance variable to the same string. Nothing else in the gem copies it, so every accessor reads whatever the caller's string holds at the time it is called rather than at match time.

Fix

Rebind str to mrb_str_dup_frozen(mrb, str) at the top of create_matchdata().

That function is the sole constructor of a MatchData: mrb_data_object_alloc(..., &matchdata_type) appears once, and the four call sites (exec_match(), regexp_gsub_str(), regexp_sub_str(), regexp_scan()) all pass the subject as an argument. Rebinding the parameter covers all four, and covers the three reads inside the function that have to agree with each other: md->source, the source instance variable and the set_match_globals() call. Snapshotting at the call sites instead would be four edits and would leave the invariant unenforced for the fifth.

No accessor changes. They are wrong because of what md->source holds, and the single assignment fixes all of them at once, matchdata_set_globals() and with it the gsub case above included. The comment on matchdata_string() already claimed the method returns "the original string (frozen copy)"; this is what makes it true.

set_match_globals() needs no edit either. Called from create_matchdata() it receives the snapshot, and called from matchdata_set_globals() it receives md->source, which the snapshot has already made immutable.

Cost

mrb_str_dup_frozen() returns an already frozen string untouched, and mrb_str_dup() shares the buffer of a heap string, so only short embedded strings are really copied. The duplicate is held in a C local until mrb_iv_set() roots it, protected in that window by the GC arena that mrb_obj_alloc() pushed it onto.

Best of five, two separate batches per column:

Every row swings by more between batches than between the columns, so the duplication does not show above the noise.

The last row is the case worth naming, because the snapshot pins the buffer and so forces every subsequent in-place write to the subject through mrb_str_modify(). That used to make the usual scanner loop quadratic. It no longer does, because mrb_str_cat() appends into a shared buffer in place instead of copying it as of 34e208b.

Testing

mrbgems/mruby-regexp/test/regexp.rb gains two cases: the upcase! sequence against md[0], #string, #pre_match, #post_match and #string.frozen? through both Regexp#match and =~, and the gsub block above against $&, $` and $'. Both fail on master and pass here.

All green, KO: 0 and Crash: 0 everywhere:

• rake test with build_config/default.rb
• MRUBY_CONFIG=asan rake test (address,undefined)
• MRUBY_CONFIG=ci/gcc-clang rake test, which covers MRB_GC_STRESS with MRB_USE_DEBUG_HOOK, MRB_GC_FIXED_ARENA, and the C++ ABI build
• MRB_UTF8_STRING
• MRB_INT32 with MRB_WORD_BOXING and MRB_UTF8_STRING
• MRB_NAN_BOXING

The MRB_NAN_BOXING build fails three mruby-string-bitops assertions, and fails the same three with this branch reverted, so they are not from this change.

Summary by CodeRabbit

• Bug Fixes

  • Match results now preserve the original subject text captured at match time, even if the source string is later modified.
  • Regular expression match data consistently retains the matched text, surrounding text, source string, and global match variables.
  • Match snapshots are protected from modification.

• Tests

  • Added regression coverage for mutated subjects and regular expression substitutions.