mruby-regexp: always allocate the capture name arena - #7166

mrb_re_compile() finishes by copying the parsed capture names into an arena that the compiled regexp owns. It has to: until that point named_captures[i].name borrows either the caller's pattern bytes or the c.stripped preprocessing buffer, and c.stripped is freed by mrb_re_compile() itself a few lines further down.

That copy was guarded by if (total > 0), where total is the sum of every name_len. On the other branch no arena was allocated and the borrowed pointers were simply left in the compiled pattern. This makes the copy unconditional, allocating a single byte when total is zero, so the postcondition the comment above the loop already states, that the regexp owns its names, holds on every path.

Let me be precise about what this is and is not, because both matter for judging whether it is worth taking.

It is not a fix for a reachable defect. The sole place a named capture is registered is the (?<name> parse, which rejects an empty name outright, so every registered entry has name_len >= 1 and total is non-zero whenever num_named is. The zero-length branch cannot be reached by any pattern that compiles. I confirmed this two ways: instrumenting the branch with abort() and running the full suite, and fuzzing roughly 169k patterns across the various named-group opener spellings and name payloads, including ones forced through preprocess_pattern. Zero hits.

Nor was the old code unsafe on its own. total is an unsigned sum of the individual lengths, so a zero total forces every name_len to zero, and all three readers of the field pass that length alongside the pointer. memcpy(dst, p, 0) and memcmp(p, q, 0) perform no access, so the stale pointer was inert rather than dangerous. No crash motivated this patch and none is claimed.

What it buys is that the invariant stops depending on a coincidence. The guard is written in terms of total while the fact that keeps it safe is a parser rejection several hundred lines away, and nothing at the copy site connects the two. A future change to the accepted group-name syntax, supporting another opener or relaxing the empty-name check, would leave borrowed pointers in a structure that outlives the buffer they point into, silently. Allocating unconditionally removes that path entirely rather than relying on the parser to keep it unreachable.

One alternative is worth naming, because it looks cheaper and is in fact worse: setting name = NULL on the zero-length path. matchdata_name_to_group(), behind MatchData#[], #begin and #end, compares names with memcmp(), whose first parameter is declared nonnull, and it tests name_len for equality before the comparison rather than after. A stored zero-length name and a request of md[""] satisfy 0 == 0 and reach memcmp(NULL, name, 0), which is undefined behaviour even at length 0.

That is not a hypothetical. Building this gem with the empty-name rejection removed, to stand in for exactly the future parser change the hardening is meant to survive, and then running

r = Regexp.new("(?<>\\d+)")
m = r.match("42")
m[""]; m.begin(""); m.end("")

under clang -fsanitize=address,undefined gives, with the nulling variant:

mrbgems/mruby-regexp/src/regexp.c:847:18: runtime error: null pointer passed as argument 1,
                                          which is declared to never be null
/usr/include/string.h:65:33: note: nonnull attribute specified here

and with the variant in this PR, nothing. Nulling would trade an inert pointer for a reportable one in precisely the scenario the hardening exists to cover. Keeping name a valid owned pointer avoids the question.

Patterns that compile today are unaffected: when total is non-zero the allocation size and the copy loop are byte for byte what they were. rake -m test on the default build is unchanged at mrbtest Total: 2075, OK: 2046, KO: 0, Crash: 0 and bintest Total: 105, OK: 105, KO: 0, and the full-core build under -fsanitize=address,undefined passes with Total: 2291, OK: 2289, KO: 0, Crash: 0 and no sanitizer output.

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of named regular-expression captures when capture names have zero-length calculated storage.
  • Ensured capture names remain available and correctly associated during regular-expression processing.