mruby-regexp: free the compile buffers before the message is built - #7229

compile_error() builds the message string first and frees the four compile buffers afterwards:

  mrb_value emsg = mrb_format(c->mrb, "%s: /%l/",
                              msg, c->orig, (size_t)(c->orig_end - c->orig));

  /* Free compile buffers before raising, since mrb_exc_raise longjmps out
     and the stack-local re_compiler is abandoned without a chance to clean
     up. mrb_free doesn't trigger GC, so emsg stays valid across these. */
  mrb_free(c->mrb, c->code);

The comment names the longjmp out of mrb_exc_raise, and misses that mrb_format leaves the same way. A failing allocation raises mrb->nomem_err, which is itself an mrb_exc_raise, so the four mrb_free calls below are never reached. re_compiler is a stack local of mrb_re_compile() and there is nothing above it that owns those buffers, so code, classes, named_captures and stripped go with it. On a MRB_GC_FIXED_ARENA build, which build_config/ci/gcc-clang.rb and build_config/ci/msvc.rb both use, an arena overflow in gc_arena_keep() opens the same exit.

The message quotes the pattern in full, so the size of that allocation is the size of the pattern. A short pattern makes a message that fits in an embedded string and never asks the allocator at all; the window belongs to long patterns.

The fix

Move the three allocating calls, mrb_format, mrb_exc_get_id and mrb_exc_new_str, below the frees. emsg reads only c->orig and c->orig_end, which mrb_re_compile() sets from the caller's pattern before it swaps pattern for c.stripped, so c->orig never points into a buffer being freed here and the message still quotes the pattern as written.

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, running

Regexp.new("[a] (?<n>b) " + "z"*2000 + "(", Regexp::EXTENDED)

under ASan and LeakSanitizer. This pattern's compile asks 14 times. The size column is the allocation that was refused; the two leaked columns are what LeakSanitizer reports at exit.

#12 and #13 are the two allocations mrb_format makes, the buffer growth and then the 2049-byte message itself (unmatched '(': / plus the 2013-byte preprocessed pattern plus /). The 10541 bytes they lost are the compile buffers: 8192 of code, 320 of one class's ranges, 16 of the class table and 2013 of stripped. #14 allocates the exception object, after the frees in both orders, so it loses nothing either way.

Rows #4 to #11 are a failure part-way through the compile, and they are unchanged because they are a different defect: mrb_re_compile() has nothing that catches a longjmp out of the middle of a parse. That is not what this PR is about.

The stack LeakSanitizer gives for the 320-byte object at #13 before the change:

Direct leak of 320 byte(s) in 1 object(s) allocated from:
    #4 add_class          mrbgems/mruby-regexp/src/re_compile.c:196
    #5 compile_charclass  mrbgems/mruby-regexp/src/re_compile.c:732
    #6 compile_atom       mrbgems/mruby-regexp/src/re_compile.c:1371
    ...
    #10 mrb_re_compile    mrbgems/mruby-regexp/src/re_compile.c:2137

Nothing observable changes when the allocator answers, so there is no test to add: the message, its wording and the exception class are the same, and rake -m test on the default configuration and on ci/gcc-clang, whose six builds report no failure and no warning, says so.

Environment

MRuby::Build.new('regexp-leak-asan') do |conf|
  conf.toolchain :clang
  conf.gembox 'full-core'
  conf.enable_sanitizer "address"
  conf.enable_debug
  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 <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[] =
  "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;
  if (argc > 1) fail_at = atoi(argv[1]);
  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 error handling to reliably release temporary resources before reporting compilation errors.
  • Preserved accurate formatting of invalid pattern messages, including patterns containing special or nonstandard characters.