mruby-regexp: give the pattern its buffers before the compile starts - #7232

Replaces #7229, which closed one of the exits below.

mrb_re_compile() grew the pattern in a re_compiler on its own stack and copied the result into a mrb_regexp_pattern once the parse was through:

  re_compiler c;
  memset(&c, 0, sizeof(c));
  ...
  mrb_regexp_pattern *pat = (mrb_regexp_pattern*)mrb_malloc(mrb, sizeof(mrb_regexp_pattern));
  pat->code = c.code;

Nothing outside that frame could reach c.code, c.classes, c.named_captures or c.stripped while the compile ran, and a compile leaves in two ways that never arrive at the copy: compile_error() raises RegexpError for a pattern it cannot parse, and mrb_realloc() raises NoMemoryError for an allocation it cannot make. Both longjmp past the frame, and nothing unwinds it.

compile_error() answered its own exit with a block of mrb_free() calls. A refused allocation had no answer at all, and neither did the allocations made after the parse: pat itself, named_arena, prefix, cached_visited and the two cached_threads are asked for one at a time, and a failure among them loses the pattern along with everything already handed to it. On a MRB_GC_FIXED_ARENA build, which build_config/ci/gcc-clang.rb and build_config/ci/msvc.rb both are, an arena overflow opens the same exits.

The fix

regexp_init() allocates the pattern zero filled and hands it to the Regexp before asking for a compile:

  pat = (mrb_regexp_pattern*)mrb_calloc(mrb, 1, sizeof(mrb_regexp_pattern));
  DATA_TYPE(self) = &regexp_type;
  DATA_PTR(self) = pat;

  mrb_re_compile(mrb, pat, RSTRING_PTR(pattern), RSTRING_LEN(pattern), flags);

Every buffer the compile allocates hangs off an object the GC reaches, from the moment it is allocated, so regexp_free() collects them however the compile ends. re_compiler keeps what belongs to the parse: the cursor, the capacities the pattern does not record, and the counts that reach it at the end.

A pattern that can be read before it is finished has to answer for that, in three places:

• add_class() clears a class before counting it. num_classes is what mrb_re_free() reads ranges pointers out of, so the count grows last rather than first.
• pat->code_len is written last, after the final allocation. No pattern compiles to no instructions, so a zero there marks a compile that did not finish, and re_uninitialized_p() reads it. It takes over from the !pat tests already standing at those call sites: the object is reachable in that state, since the exception can be rescued while ObjectSpace still hands it out.
• A second Regexp#initialize is refused with TypeError: already initialized regexp, which is CRuby's answer to it. Compiling in place would drop the pattern the object already owns with nothing left to free it, which the old code did.

That leaves compile_error() with nothing to free. The preprocessed copy of the pattern is not the pattern's to own, so it comes from mrb_temp_alloc() and the GC arena holds it for as long as the parse reads it. mark_empty_loops() takes its two arrays from one allocation for the same reason: asking twice put a raising call between the first block and anything that could free it.

Measurement

A driver that redefines mrb_basic_alloc_func to fail every allocation from the Nth on, counting only the allocations made inside a Regexp.new call, under ASan and LeakSanitizer. The leaked columns are what LeakSanitizer reports at exit; the size columns are the allocation that was refused, and they differ between the two builds because the order changes, the pattern struct now being the third rather than the twelfth.

A pattern that compiles through to the end, Regexp.new("[a] (?<n>b) " + "z"*2000, Regexp::EXTENDED). Its compile asks 20 times on master and 19 times here, the two arrays mark_empty_loops() wants now coming from one allocation:

The same pattern with a ( appended, so that compile_error() raises. Its compile asks 14 times on master and 15 times here, the extra one being the pattern struct, which master does not reach on this path:

The rows master answers with a leak are the two kinds this changes. #4 to #11, in both tables, are a refused allocation in the middle of the parse, where compile_error() is never reached and nothing frees anything. #12 onward are a refused allocation after the parse: on the failing pattern those are the two mrb_format() makes for the message, and on the other they are the pattern struct and the arrays that follow it, where what is lost is the pattern together with everything already handed to it. On this branch every N reports nothing leaked, in both tables.

Nothing observable changes when the allocator answers, apart from the refusal a second initialize now gets, which mrbgems/mruby-regexp/test/regexp.rb covers. rake -m test on the default configuration and on ci/gcc-clang, whose six test runs report no failure and no warning, says so; the whole suite under the sanitizer build below is green as well, at 2340 assertions, with LeakSanitizer reporting nothing at exit.

Size

.text over every object of a build, on a clean build directory, ci/gcc-clang:

The growth is one indirection: a buffer the parser reached through c it now reaches through c->pat, and c->pat has to be loaded again after every call the parser makes. compile_seq, where the parser is inlined, takes +288 of the bintest figure and mrb_re_compile +349; the rest is re_uninitialized_p() at its six call sites.

Environment

gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -g3 -O0 -DMRB_GC_STRESS -DMRB_USE_DEBUG_HOOK -DMRBGEM_MRUBY_REGEXP_VERSION=0.0.0 -DMRB_DEBUG -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER mrbgems/mruby-regexp/src/re_compile.c
gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -DMRB_GC_FIXED_ARENA -DMRBGEM_MRUBY_REGEXP_VERSION=0.0.0 -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER -DMRB_USE_DEBUG_HOOK mrbgems/mruby-regexp/src/re_compile.c
gcc -g -O3 -Wall -Wundef -Wwrite-strings -x c++ -std=gnu++03 -DMRB_GC_FIXED_ARENA -DMRBGEM_MRUBY_REGEXP_VERSION=0.0.0 -DMRB_USE_CXX_EXCEPTION -DMRB_USE_CXX_ABI -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER mrbgems/mruby-regexp/src/re_compile.c
gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -DMRBGEM_MRUBY_REGEXP_VERSION=0.0.0 -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER mrbgems/mruby-regexp/src/re_compile.c
gcc -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -DMRB_USE_ASCII_CASE -DMRBGEM_MRUBY_REGEXP_VERSION=0.0.0 -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER mrbgems/mruby-regexp/src/re_compile.c

MRuby::Build.new('regexp-leak-asan') do |conf|
  conf.toolchain :clang
  conf.gembox 'full-core'
  conf.enable_sanitizer "address"
  conf.enable_debug
  conf.enable_test
  conf.compilers.each do |c|
    c.defines += %w(MRB_GC_FIXED_ARENA)
  end
end

clang -std=gnu99 -g -O3 -Wall -Wundef -Werror-implicit-function-declaration -Wwrite-strings -Wzero-length-array -fsanitize=address -g3 -O0 -DMRB_GC_FIXED_ARENA -DMRBGEM_MRUBY_REGEXP_VERSION=0.0.0 -DMRB_DEBUG -DMRB_USE_BIGINT -DMRB_USE_COMPLEX -DHAVE_MRUBY_ENCODING_GEM -DMRB_UTF8_STRING -DHAVE_MRUBY_IO_GEM -DMRB_USE_RATIONAL -DHAVE_MRUBY_REGEXP_GEM -DMRB_USE_SET -DMRB_USE_TASK_SCHEDULER mrbgems/mruby-regexp/src/re_compile.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <mruby.h>
#include <mruby/compile.h>

static int counting = 0;
static int alloc_seq = 0;
static int fail_at = 0;

void *
mrb_basic_alloc_func(void *p, size_t size)
{
  if (size == 0) {
    free(p);
    return NULL;
  }
  if (counting) {
    alloc_seq++;
    if (fail_at && alloc_seq >= fail_at) {
      fprintf(stderr, "alloc #%d size %zu -> FAIL\n", alloc_seq, size);
      return NULL;
    }
    fprintf(stderr, "alloc #%d size %zu\n", alloc_seq, size);
  }
  return realloc(p, size);
}

static mrb_value count_on(mrb_state *mrb, mrb_value self) { counting = 1; return self; }
static mrb_value count_off(mrb_state *mrb, mrb_value self) { counting = 0; return self; }

static const char SCRIPT_VALID[] =
  "pat = \"[a] (?<n>b) \" + \"z\"*2000\n"
  "__count_on\n"
  "begin\n"
  "  Regexp.new(pat, Regexp::EXTENDED)\n"
  "rescue Exception => e\n"
  "end\n"
  "__count_off\n";

static const char SCRIPT_BROKEN[] =
  "pat = \"[a] (?<n>b) \" + \"z\"*2000 + \"(\"\n"
  "__count_on\n"
  "begin\n"
  "  Regexp.new(pat, Regexp::EXTENDED)\n"
  "rescue Exception => e\n"
  "end\n"
  "__count_off\n";

int
main(int argc, char **argv)
{
  mrb_state *mrb;
  const char *script = SCRIPT_BROKEN;

  if (argc > 1) fail_at = atoi(argv[1]);
  if (argc > 2 && strcmp(argv[2], "valid") == 0) script = SCRIPT_VALID;

  mrb = mrb_open();
  mrb_define_method(mrb, mrb->object_class, "__count_on", count_on, MRB_ARGS_NONE());
  mrb_define_method(mrb, mrb->object_class, "__count_off", count_off, MRB_ARGS_NONE());
  mrb_load_string(mrb, script);
  counting = 0;
  fail_at = 0;
  fprintf(stderr, "allocations in window: %d\n", alloc_seq);
  mrb_close(mrb);
  return 0;
}

Summary by CodeRabbit

• Bug Fixes

  • Improved regular expression compilation reliability, including safer handling of incomplete or failed patterns.
  • Prevented regular expressions from being initialized more than once; repeated initialization now raises a TypeError.
  • Strengthened validation across matching and replacement operations to avoid using uninitialized patterns.

• Tests

  • Added coverage for repeated initialization of Regexp subclasses.