`String#split(/regex/)` leaks `@set_global_variables=false` on mid-loop raise → all subsequent `$~`/`$1`-`$9` silently nil

[vobloeb]

Affected commit / version

Introduced in 0126b46 ("Reduce mrb_gv_set and mrb_gv_remove calls in string_split for performance", 2025-12-21). Present in HEAD (3389bae at time of report).

Last good commit: bcfa173 (parent of the offending commit; pre-optimization).

Symptom

After any String#split(/regex/) call that raises an exception inside its inner loop (and the exception is caught somewhere by the caller, e.g. via rescue), every subsequent regex match in the same mrb_state silently fails to populate $~, $&, $`, $', $+ and $1–$9.

User-level symptom — typical case statement breaks silently:

"x=42".split(/something_that_raises/) rescue nil   # ← exception caught here
# ... arbitrary further code ...
case "uname=Linux"
when /^uname=(.+)$/
  puts $1   # → nil   (expected: "Linux")
end

The opacity is the worst part: there is no warning, no log line; downstream code that relied on $1 simply receives nil and fails later in unrelated places with NoMethodError: undefined method 'X' for nil:NilClass.

Root cause

src/mruby_onig_regexp.c string_split (post-0126b46):

mrb_value last_set_global_variables = mrb_obj_iv_get(mrb, (struct RObject*)cls_onig_regexp, MRB_IVSYM(set_global_variables));
mrb_obj_iv_set(mrb, (struct RObject*)cls_onig_regexp, MRB_IVSYM(set_global_variables), mrb_false_value());  // ← disable
while ((end = onig_match_common(mrb, reg, match_value, self, start)) >= 0) {
  // ... mrb_ary_push, onig_str_substr, mrb_str_new_lit, ... — any of these can raise on OOM/GC
  // onig_match_common itself can `mrb_raise(mrb, E_REGEXP_ERROR, ...)` on onig_search error
}
mrb_obj_iv_set(mrb, (struct RObject*)cls_onig_regexp, MRB_IVSYM(set_global_variables), last_set_global_variables);  // ← restore (skipped on raise)
onig_gv_set(mrb, match_value);

The optimization caches one final $~/$1-$9 write at the end of split, instead of N writes per iteration. To do so it disables the class-level IV @set_global_variables for the duration of the loop and restores it after.

The flaw: the restore line is reached only on normal control flow. Any exception (E_REGEXP_ERROR from onig_search; arena/OOM mrb_raise from mrb_ary_push / onig_str_substr / mrb_str_new_lit) longjmps directly out of string_split without restoring the IV. From that point on, for the entire lifetime of the mrb_state, the gate in onig_match_common:

if (mrb_class_get_id(mrb, MRB_SYM(Regexp)) == cls_onig_regexp &&
    mrb_bool(mrb_obj_iv_get(mrb, (struct RObject*)cls_onig_regexp, MRB_IVSYM(set_global_variables)))) {
  onig_gv_set(mrb, MISMATCH_NIL_OR(match_value));
}

evaluates to false and $~ / $1-$9 updates are silently skipped — even outside any split.

The pre-0126b46 code did not manipulate @set_global_variables at all — each inner onig_match_common call set globals normally, last match's globals persisted (same observable semantic, just N-1 extra writes per split — the perf cost the optimization tried to avoid).

Reproducer (no exception needed to demonstrate the gate)

Simulating the leaked state by setting the IV directly is enough to show the gate:

OnigRegexp.set_global_variables = false   # ← what a raised split mid-loop leaves behind

case "x=42"
when /^x=(.+)$/
  p $1   # → nil  (expected: "42")
end

A natural trigger via split is harder to demonstrate portably (depends on memory pressure or a malformed regex hitting onig_search's error path mid-iteration), but the C-level mechanism is unambiguous: any C-level raise between disable and restore produces the same leaked state.

Proposed fix — options

Option 1 (minimal): wrap with mrb_protect_error / explicit RAII pattern

Use mrb_protect_error (or equivalent mrb_protect / try-finally pattern) around the split loop so the IV is always restored, even on exception:

mrb_value last_sgv = mrb_obj_iv_get(mrb, ..., MRB_IVSYM(set_global_variables));
mrb_obj_iv_set(mrb, ..., MRB_IVSYM(set_global_variables), mrb_false_value());

mrb_bool err = FALSE;
mrb_value loop_result = mrb_protect_error(mrb, &split_loop_body, &state, &err);

mrb_obj_iv_set(mrb, ..., MRB_IVSYM(set_global_variables), last_sgv);  // ← always restored

if (err) mrb_exc_raise(mrb, loop_result);
onig_gv_set(mrb, match_value);

Option 2 (simplest): function-local flag instead of class-level IV

The IV exists for a process-global toggle (OnigRegexp.set_global_variables = ...). Repurposing it as a transient "disable during split" toggle is what creates the cross-call leak. Pass a bool skip_globals parameter into onig_match_common instead — pure stack-local state, can't leak across exceptions.

Option 3 (revert): drop the optimization

Revert 0126b46. The observable semantic ("$~ reflects the last match of split") is identical without the optimization — each inner onig_match_common simply writes globals normally, last-write-wins. The cost is N inner writes per split instead of 1.

How we encountered this

We bumped mruby-onig-regexp from bcfa173 to HEAD. Immediately after, our program started failing with cryptic NoMethodError: undefined method 'split' and 'downcase' at case-arm bodies that referenced $1. The bug surfaced in two completely unrelated callers— both as $1.X on a nil $1. Bisect pointed at our gem bump; the offending commit narrowed via inspection to 0126b46.

Locally working around by reverting the four IV-manipulation lines (mrb_value last_set_global_variables = ...; / mrb_obj_iv_set(... mrb_false_value()); / mrb_obj_iv_set(... last_set_global_variables); / the final onig_gv_set(mrb, match_value);) restores correct behavior.

Happy to send a PR for whichever of the three options above you prefer — please advise.