mattn / mattn/mruby-onig-regexp
`String#split(/regex/)` leaks `@set_global_variables=false` on mid-loop raise → all subsequent `$~`/`$1`-`$9` silently nil
Nobody has claimed this yet.
- Dominant language
- C
- Stars
- 31
- Forks
- 40
- PR merge metrics
- No merged PRs in 30d
Description
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.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in src/mruby_onig_regexp.c at string_split and compare the post-0126b46 code with bcfa173. Trace the disable/restore handling around onig_match_common, then reproduce the leaked state with a split followed by a regex match that reads $1. Done means an exception during split cannot leave @set_global_variables disabled and later matches still populate the globals.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c, ruby
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100