From ac85999601a9752966b7f65ebee65160fba9bd0d Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 6 Aug 2026 13:04:33 -0700 Subject: [PATCH 1/2] Root the aggregate argument array so GC cannot free live values (GHSA-mwm8-39rw-8826) rb_sqlite3_aggregator_step converts an aggregate's SQLite arguments into a VALUE array allocated with xcalloc. That memory is not a GC root. sqlite3val2rb allocates, so a GC triggered while converting a later argument can collect a Ruby object already stored in an earlier slot and reuse it -- the step block then receives a wrong or freed object, and above Ruby 3.4's 640-byte embedded boundary (where every conversion mallocs) the process can segfault under ordinary GC. Needs arity >= 2: the argc == 1 branch stores into a stack local, which conservative stack scanning already pins. Fix: register each slot with rb_gc_register_address before filling any, so a GC during a later conversion scans every VALUE already stored, and unregister before xfree. This is the idiom nokogiri uses at xml_xpath_context.c. Distinct from #723, which pins the callback instances stored as SQLite user-data and does not touch this argument array. Both are needed. Verified on the released precompiled artifact's source, ruby 3.4.10 arm64-darwin, system SQLite, two separately built extensions: GC.stress, arity 2 before CORRUPT 3/3 after CLEAN 3/3 ordinary GC, ~4KB rows before SIGSEGV after CLEAN 8/8 (incl. the size that crashed before) arity 1 (control) CLEAN both patched under GC.stress CLEAN test_multi_argument_step_arguments_survive_gc reproduces it: the step block checks each argument against the column it came from, under GC.stress. It fails on an unpatched build (a slot holds an unrelated object) and passes with this change. --- ext/sqlite3/aggregator.c | 10 ++++++++ test/test_integration_aggregate.rb | 37 ++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/ext/sqlite3/aggregator.c b/ext/sqlite3/aggregator.c index 9a04aa64..4135b376 100644 --- a/ext/sqlite3/aggregator.c +++ b/ext/sqlite3/aggregator.c @@ -133,6 +133,13 @@ rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) } if (argc > 1) { params = xcalloc((size_t)argc, sizeof(VALUE)); + /* xcalloc'd memory is not a GC root. sqlite3val2rb allocates, so a GC + * triggered while converting a later argument can collect a Ruby object + * already stored in an earlier slot and reuse it. Register each slot as + * a root before filling any, so every stored VALUE is scanned. */ + for (i = 0; i < argc; i++) { + rb_gc_register_address(¶ms[i]); + } for (i = 0; i < argc; i++) { params[i] = sqlite3val2rb(argv[i]); } @@ -140,6 +147,9 @@ rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) rb_sqlite3_protected_funcall( handler_instance, rb_intern("step"), argc, params, &exc_status); if (argc > 1) { + for (i = 0; i < argc; i++) { + rb_gc_unregister_address(¶ms[i]); + } xfree(params); } diff --git a/test/test_integration_aggregate.rb b/test/test_integration_aggregate.rb index 437ddd23..3c581217 100644 --- a/test/test_integration_aggregate.rb +++ b/test/test_integration_aggregate.rb @@ -427,4 +427,41 @@ def test_step_on_statement_whose_database_was_closed_does_not_use_freed_aggregat values = stmt.step assert_equal 33, values[0] end + + # GHSA-mwm8-39rw-8826: rb_sqlite3_aggregator_step converts the arguments into + # an xcalloc'd VALUE array that is not a GC root. sqlite3val2rb allocates, so a + # GC while converting a later argument could collect a Ruby object already + # stored in an earlier slot and hand the step block a wrong or freed object. + # Needs arity >= 2 (the arity 1 branch uses a pinned stack local). Large column + # values make every conversion allocate, so the window is reachable. + def test_multi_argument_step_arguments_survive_gc + @db.execute("create table wide ( a text, b text )") + filler = "p" * 2000 + @db.transaction do + stmt = @db.prepare("insert into wide values ( ?, ? )") + 200.times { |i| stmt.execute("a-#{i}-#{filler}", "b-#{i}-#{filler}") } + stmt.close + end + + seen = 0 + bad = [] + @db.create_aggregate("checkcols", 2) do + step do |ctx, x, y| + seen += 1 + bad << x unless x.is_a?(String) && x.start_with?("a-") + bad << y unless y.is_a?(String) && y.start_with?("b-") + end + finalize { |ctx| ctx.result = seen } + end + + begin + GC.stress = true + @db.get_first_value("select checkcols(a, b) from wide") + ensure + GC.stress = false + end + + assert_equal 200, seen + assert_empty bad, "aggregate step received corrupted arguments after GC" + end end From 569765ffd38a2d9182d6de3f656bdc8b436a87c9 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Tue, 11 Aug 2026 14:42:58 -0400 Subject: [PATCH 2/2] Use ALLOCV_N instead of per-slot rb_gc_register_address Rooting the aggregate argument array with rb_gc_register_address made each step call pay a linear scan of the VM's global-address list per slot on unregister, and a raise from sqlite3val2rb mid-fill would have leaked the array and left its slots registered as permanent roots. The argument array will be allocated with ALLOCV_N, whose memory is conservatively marked (machine stack up to 1KB, a GC-managed tmpbuf above that), so stored VALUEs survive a GC during conversion at no per-slot cost, and the buffer is reclaimed by GC if the fill raises. This follows sparklemotion/nokogiri@ab050ed0, which replaced the same register-address idiom in xml_xpath_context.c. --- ext/sqlite3/aggregator.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/ext/sqlite3/aggregator.c b/ext/sqlite3/aggregator.c index 4135b376..8f790201 100644 --- a/ext/sqlite3/aggregator.c +++ b/ext/sqlite3/aggregator.c @@ -119,6 +119,7 @@ rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) VALUE inst = rb_sqlite3_aggregate_instance(ctx); VALUE handler_instance = rb_iv_get(inst, "-handler_instance"); VALUE *params = NULL; + VALUE params_handle = 0; VALUE one_param; int exc_status = NUM2INT(rb_iv_get(inst, "-exc_status")); int i; @@ -132,14 +133,9 @@ rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) params = &one_param; } if (argc > 1) { - params = xcalloc((size_t)argc, sizeof(VALUE)); - /* xcalloc'd memory is not a GC root. sqlite3val2rb allocates, so a GC - * triggered while converting a later argument can collect a Ruby object - * already stored in an earlier slot and reuse it. Register each slot as - * a root before filling any, so every stored VALUE is scanned. */ - for (i = 0; i < argc; i++) { - rb_gc_register_address(¶ms[i]); - } + /* ALLOCV memory is conservatively marked, so stored VALUEs survive a + * GC raised by a later sqlite3val2rb call. */ + params = ALLOCV_N(VALUE, params_handle, argc); for (i = 0; i < argc; i++) { params[i] = sqlite3val2rb(argv[i]); } @@ -147,10 +143,7 @@ rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv) rb_sqlite3_protected_funcall( handler_instance, rb_intern("step"), argc, params, &exc_status); if (argc > 1) { - for (i = 0; i < argc; i++) { - rb_gc_unregister_address(¶ms[i]); - } - xfree(params); + ALLOCV_END(params_handle); } rb_iv_set(inst, "-exc_status", INT2NUM(exc_status));