From a01ee8d3ae904bdee53687f164262811b8dbe33d Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Fri, 31 Jul 2026 10:09:02 +0100 Subject: [PATCH 1/5] Stop sqlite calling into moved Ruby objects When you register a scalar function, aggregator, collation, trace handler or authorizer, we hand sqlite a pointer to a Ruby object. The garbage collector can move that object without updating sqlite's copy of the address. The database's C struct now points at the collections holding those callbacks, so the mark function can pin them. Trace and authorizer passed the database and read the callback out of an instance variable. They now pass the struct instead, which lives in malloc'd memory the collector never relocates. The busy handler already worked this way. Writes to the new fields go through RB_OBJ_WRITE so the collector keeps them alive. There is a test for each of the five that forces every movable object to move. None of them hold the object under test in a local variable, which would prevent that. --- ext/sqlite3/aggregator.c | 7 +++ ext/sqlite3/aggregator.h | 4 ++ ext/sqlite3/database.c | 82 +++++++++++++++++++++++++----- ext/sqlite3/database.h | 8 +++ test/helper.rb | 12 +++++ test/test_collation.rb | 23 +++++++++ test/test_integration.rb | 30 +++++++++++ test/test_integration_aggregate.rb | 30 +++++++++++ 8 files changed, 184 insertions(+), 12 deletions(-) diff --git a/ext/sqlite3/aggregator.c b/ext/sqlite3/aggregator.c index 9c17ea55..f2704306 100644 --- a/ext/sqlite3/aggregator.c +++ b/ext/sqlite3/aggregator.c @@ -254,10 +254,17 @@ rb_sqlite3_define_aggregator2(VALUE self, VALUE aggregator, VALUE ruby_name) CHECK(ctx->db, status); rb_ary_push(aggregators, aw); + RB_OBJ_WRITE(self, &ctx->aggregators, aggregators); return self; } +void +rb_sqlite3_aggregator_pin_instances(VALUE aw) +{ + rb_sqlite3_pin_array_and_contents(rb_iv_get(aw, "-instances")); +} + void rb_sqlite3_aggregator_init(void) { diff --git a/ext/sqlite3/aggregator.h b/ext/sqlite3/aggregator.h index 3f528ba5..bee9811b 100644 --- a/ext/sqlite3/aggregator.h +++ b/ext/sqlite3/aggregator.h @@ -7,4 +7,8 @@ VALUE rb_sqlite3_define_aggregator2(VALUE self, VALUE aggregator, VALUE ruby_nam void rb_sqlite3_aggregator_init(void); +/* sqlite stores each live instance's VALUE in its aggregate context, so those + * must not move either. */ +void rb_sqlite3_aggregator_pin_instances(VALUE aw); + #endif diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 734ca849..610d9ac7 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -70,11 +70,64 @@ close_or_discard_db(sqlite3RubyPtr ctx) } +void +rb_sqlite3_pin_array_and_contents(VALUE ary) +{ + long i; + + if (NIL_P(ary) || !ary) { return; } + + rb_gc_mark(ary); + for (i = 0; i < RARRAY_LEN(ary); i++) { + rb_gc_mark(RARRAY_AREF(ary, i)); + } +} + +static int +pin_hash_value(VALUE key, VALUE value, VALUE arg) +{ + rb_gc_mark(value); + return ST_CONTINUE; +} + +static void +pin_hash_and_contents(VALUE hash) +{ + if (NIL_P(hash) || !hash) { return; } + + rb_gc_mark(hash); + rb_hash_foreach(hash, pin_hash_value, 0); +} + +/* Each wrapper also owns live aggregate instances, whose VALUEs sqlite keeps in + * its own aggregate contexts. */ +static void +pin_aggregators(VALUE aggregators) +{ + long i; + + rb_sqlite3_pin_array_and_contents(aggregators); + + if (NIL_P(aggregators) || !aggregators) { return; } + + for (i = 0; i < RARRAY_LEN(aggregators); i++) { + rb_sqlite3_aggregator_pin_instances(RARRAY_AREF(aggregators, i)); + } +} + static void database_mark(void *ctx) { sqlite3RubyPtr c = (sqlite3RubyPtr)ctx; + + /* sqlite holds raw pointers to these, so they must not move. */ rb_gc_mark(c->busy_handler); + rb_gc_mark(c->trace_handler); + rb_gc_mark(c->authorizer); + + rb_sqlite3_pin_array_and_contents(c->functions); + pin_hash_and_contents(c->collations); + pin_aggregators(c->aggregators); } static void @@ -252,9 +305,8 @@ total_changes(VALUE self) static void tracefunc(void *data, const char *sql) { - VALUE self = (VALUE)data; - VALUE thing = rb_iv_get(self, "@tracefunc"); - rb_funcall(thing, rb_intern("call"), 1, rb_str_new2(sql)); + sqlite3RubyPtr ctx = (sqlite3RubyPtr)data; + rb_funcall(ctx->trace_handler, rb_intern("call"), 1, rb_str_new2(sql)); } /* call-seq: @@ -279,8 +331,9 @@ trace(int argc, VALUE *argv, VALUE self) if (NIL_P(block) && rb_block_given_p()) { block = rb_block_proc(); } rb_iv_set(self, "@tracefunc", block); + RB_OBJ_WRITE(self, &ctx->trace_handler, block); - sqlite3_trace(ctx->db, NIL_P(block) ? NULL : tracefunc, (void *)self); + sqlite3_trace(ctx->db, NIL_P(block) ? NULL : tracefunc, (void *)ctx); return self; } @@ -509,7 +562,7 @@ static VALUE define_function_with_flags(VALUE self, VALUE name, VALUE flags) { sqlite3RubyPtr ctx; - VALUE block; + VALUE block, functions; int status; TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); @@ -530,7 +583,9 @@ define_function_with_flags(VALUE self, VALUE name, VALUE flags) CHECK(ctx->db, status); - rb_ary_push(rb_iv_get(self, "@functions"), block); + functions = rb_iv_get(self, "@functions"); + rb_ary_push(functions, block); + RB_OBJ_WRITE(self, &ctx->functions, functions); return self; } @@ -632,14 +687,13 @@ rb_sqlite3_auth( const char *_c, const char *_d) { - VALUE self = (VALUE)ctx; + sqlite3RubyPtr db_ctx = (sqlite3RubyPtr)ctx; VALUE action = INT2NUM(_action); VALUE a = _a ? rb_str_new2(_a) : Qnil; VALUE b = _b ? rb_str_new2(_b) : Qnil; VALUE c = _c ? rb_str_new2(_c) : Qnil; VALUE d = _d ? rb_str_new2(_d) : Qnil; - VALUE callback = rb_iv_get(self, "@authorizer"); - VALUE result = rb_funcall(callback, rb_intern("call"), 5, action, a, b, c, d); + VALUE result = rb_funcall(db_ctx->authorizer, rb_intern("call"), 5, action, a, b, c, d); if (T_FIXNUM == TYPE(result)) { return (int)NUM2INT(result); } if (Qtrue == result) { return SQLITE_OK; } @@ -668,12 +722,13 @@ set_authorizer(VALUE self, VALUE authorizer) REQUIRE_OPEN_DB(ctx); status = sqlite3_set_authorizer( - ctx->db, NIL_P(authorizer) ? NULL : rb_sqlite3_auth, (void *)self + ctx->db, NIL_P(authorizer) ? NULL : rb_sqlite3_auth, (void *)ctx ); CHECK(ctx->db, status); rb_iv_set(self, "@authorizer", authorizer); + RB_OBJ_WRITE(self, &ctx->authorizer, authorizer); return self; } @@ -756,6 +811,7 @@ static VALUE collation(VALUE self, VALUE name, VALUE comparator) { sqlite3RubyPtr ctx; + VALUE collations; TypedData_Get_Struct(self, sqlite3Ruby, &database_type, ctx); REQUIRE_OPEN_DB(ctx); @@ -766,8 +822,10 @@ collation(VALUE self, VALUE name, VALUE comparator) (void *)comparator, NIL_P(comparator) ? NULL : rb_comparator_func)); - /* Make sure our comparator doesn't get garbage collected. */ - rb_hash_aset(rb_iv_get(self, "@collations"), name, comparator); + /* sqlite holds a raw pointer to the comparator, so keep it alive and unmoved. */ + collations = rb_iv_get(self, "@collations"); + rb_hash_aset(collations, name, comparator); + RB_OBJ_WRITE(self, &ctx->collations, collations); return self; } diff --git a/ext/sqlite3/database.h b/ext/sqlite3/database.h index 04124881..62fe9071 100644 --- a/ext/sqlite3/database.h +++ b/ext/sqlite3/database.h @@ -10,6 +10,11 @@ struct _sqlite3Ruby { sqlite3 *db; VALUE busy_handler; + VALUE functions; + VALUE collations; + VALUE aggregators; + VALUE trace_handler; + VALUE authorizer; int stmt_timeout; struct timespec stmt_deadline; rb_pid_t owner; @@ -19,6 +24,9 @@ struct _sqlite3Ruby { typedef struct _sqlite3Ruby sqlite3Ruby; typedef sqlite3Ruby *sqlite3RubyPtr; +/* Pinning a collection doesn't pin what's in it, hence both. */ +void rb_sqlite3_pin_array_and_contents(VALUE ary); + void init_sqlite3_database(); void set_sqlite3_func_result(sqlite3_context *ctx, VALUE result); diff --git a/test/helper.rb b/test/helper.rb index e52afbba..0ff74020 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -127,6 +127,18 @@ def skip_if_timing_unreliable def windows? ::RUBY_PLATFORM =~ /mingw|mswin/ end + + # Relocates every movable object, so any VALUE this extension has handed to + # sqlite moves and sqlite's copy of the address goes stale. Returns false + # where the runtime can't compact, so callers can skip. + def force_gc_compaction + return false unless ::GC.respond_to?(:verify_compaction_references) + + ::GC.verify_compaction_references(expand_heap: true, toward: :empty) + true + rescue ::NotImplementedError + false + end end end diff --git a/test/test_collation.rb b/test/test_collation.rb index 21c8871d..2918efd4 100644 --- a/test/test_collation.rb +++ b/test/test_collation.rb @@ -14,6 +14,13 @@ def compare left, right end end + # Used by one test only, so a live count of 1 means just the current one. + class ReleasableComparator + def compare(left, right) + left <=> right + end + end + def setup @db = SQLite3::Database.new(":memory:") @create = "create table ex(id int, data string)" @@ -33,6 +40,22 @@ def test_custom_collation assert_equal 1, comparator.calls.length end + def test_collation_does_not_use_moved_comparator_after_gc_compaction + @db.collation "foo", Comparator.new + + skip("GC compaction is unsupported on this runtime") unless force_gc_compaction + + @db.execute("select data from ex order by 1 collate foo") + assert_equal 1, @db.collations["foo"].calls.length + end + + def test_replacing_a_collation_releases_the_previous_comparator + 3.times { @db.collation "foo", ReleasableComparator.new } + GC.start(full_mark: true, immediate_sweep: true) + + assert_equal 1, ObjectSpace.each_object(ReleasableComparator).count + end + def test_remove_collation comparator = Comparator.new diff --git a/test/test_integration.rb b/test/test_integration.rb index 4e0706da..5d45d3e7 100644 --- a/test/test_integration.rb +++ b/test/test_integration.rb @@ -82,6 +82,25 @@ def test_trace assert_equal "select * from foo", result end + def test_trace_does_not_use_moved_block_after_gc_compaction + result = nil + @db.trace { |sql| result = sql } + + skip("GC compaction is unsupported on this runtime") unless force_gc_compaction + + @db.execute "select * from foo" + assert_equal "select * from foo", result + end + + def test_authorizer_does_not_use_moved_block_after_gc_compaction + @db.authorizer { |type, a, b, c, d| 0 } + + skip("GC compaction is unsupported on this runtime") unless force_gc_compaction + + rows = @db.execute "select * from foo" + assert_equal 3, rows.length + end + def test_authorizer_okay @db.authorizer { |type, a, b, c, d| 0 } rows = @db.execute "select * from foo" @@ -564,6 +583,17 @@ def test_create_function assert_match(/>>>.*<<>>#{x}<<<" + end + + skip("GC compaction is unsupported on this runtime") unless force_gc_compaction + + value = @db.get_first_value("select munge(b) from foo where a=1") + assert_match(/>>>.*<< Date: Thu, 6 Aug 2026 15:18:59 -0400 Subject: [PATCH 2/5] Rebased to use compaction helpers following the existing pattern from nokogiri --- test/helper.rb | 12 ------------ test/test_collation.rb | 4 +++- test/test_integration.rb | 12 +++++++++--- test/test_integration_aggregate.rb | 8 +++++--- 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/test/helper.rb b/test/helper.rb index 0ff74020..e52afbba 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -127,18 +127,6 @@ def skip_if_timing_unreliable def windows? ::RUBY_PLATFORM =~ /mingw|mswin/ end - - # Relocates every movable object, so any VALUE this extension has handed to - # sqlite moves and sqlite's copy of the address goes stale. Returns false - # where the runtime can't compact, so callers can skip. - def force_gc_compaction - return false unless ::GC.respond_to?(:verify_compaction_references) - - ::GC.verify_compaction_references(expand_heap: true, toward: :empty) - true - rescue ::NotImplementedError - false - end end end diff --git a/test/test_collation.rb b/test/test_collation.rb index 2918efd4..0a910ca5 100644 --- a/test/test_collation.rb +++ b/test/test_collation.rb @@ -41,9 +41,11 @@ def test_custom_collation end def test_collation_does_not_use_moved_comparator_after_gc_compaction + skip_unless_compaction_supported + @db.collation "foo", Comparator.new - skip("GC compaction is unsupported on this runtime") unless force_gc_compaction + gc_verify_compaction_references @db.execute("select data from ex order by 1 collate foo") assert_equal 1, @db.collations["foo"].calls.length diff --git a/test/test_integration.rb b/test/test_integration.rb index 5d45d3e7..4d9f6afd 100644 --- a/test/test_integration.rb +++ b/test/test_integration.rb @@ -83,19 +83,23 @@ def test_trace end def test_trace_does_not_use_moved_block_after_gc_compaction + skip_unless_compaction_supported + result = nil @db.trace { |sql| result = sql } - skip("GC compaction is unsupported on this runtime") unless force_gc_compaction + gc_verify_compaction_references @db.execute "select * from foo" assert_equal "select * from foo", result end def test_authorizer_does_not_use_moved_block_after_gc_compaction + skip_unless_compaction_supported + @db.authorizer { |type, a, b, c, d| 0 } - skip("GC compaction is unsupported on this runtime") unless force_gc_compaction + gc_verify_compaction_references rows = @db.execute "select * from foo" assert_equal 3, rows.length @@ -584,11 +588,13 @@ def test_create_function end def test_create_function_does_not_use_moved_block_after_gc_compaction + skip_unless_compaction_supported + @db.create_function("munge", 1) do |func, x| func.result = ">>>#{x}<<<" end - skip("GC compaction is unsupported on this runtime") unless force_gc_compaction + gc_verify_compaction_references value = @db.get_first_value("select munge(b) from foo where a=1") assert_match(/>>>.*<< Date: Mon, 10 Aug 2026 14:55:48 -0400 Subject: [PATCH 3/5] Make the collation compaction test move the comparator The compaction test for collations passed whether or not sqlite held a stale pointer to the comparator. The comparator was the only object in these tests constructed in the test method's own frame, so conservative machine-stack scanning pinned it and GC compaction never moved it. The comparator will be registered from another thread, whose stack is gone before compaction runs, so nothing pins it. --- test/test_collation.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/test_collation.rb b/test/test_collation.rb index 0a910ca5..2a168d38 100644 --- a/test/test_collation.rb +++ b/test/test_collation.rb @@ -43,7 +43,11 @@ def test_custom_collation def test_collation_does_not_use_moved_comparator_after_gc_compaction skip_unless_compaction_supported - @db.collation "foo", Comparator.new + # Registered on another thread so that no reference to the comparator is + # left on this thread's machine stack, where conservative scanning would + # pin it. A pinned comparator never moves and the test passes without + # exercising the fix. + Thread.new { @db.collation "foo", Comparator.new }.join gc_verify_compaction_references From bc854de3d944a8c85c9a5fcbc20e43db78ba5e4d Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 10 Aug 2026 15:47:46 -0400 Subject: [PATCH 4/5] Drop the callback ivars that the struct fields replaced The trace handler and the authorizer were each stored twice. sqlite reached both through instance variables on the Database, and moving those lookups onto the malloc'd struct left the writes to @tracefunc and @authorizer without a reader. Both writes and their initialisers will be removed, leaving the struct fields as the only record of either callback. --- ext/sqlite3/database.c | 4 +--- lib/sqlite3/database.rb | 2 -- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c index 610d9ac7..2c949b30 100644 --- a/ext/sqlite3/database.c +++ b/ext/sqlite3/database.c @@ -84,7 +84,7 @@ rb_sqlite3_pin_array_and_contents(VALUE ary) } static int -pin_hash_value(VALUE key, VALUE value, VALUE arg) +pin_hash_value(VALUE UNUSED(key), VALUE value, VALUE UNUSED(arg)) { rb_gc_mark(value); return ST_CONTINUE; @@ -330,7 +330,6 @@ trace(int argc, VALUE *argv, VALUE self) if (NIL_P(block) && rb_block_given_p()) { block = rb_block_proc(); } - rb_iv_set(self, "@tracefunc", block); RB_OBJ_WRITE(self, &ctx->trace_handler, block); sqlite3_trace(ctx->db, NIL_P(block) ? NULL : tracefunc, (void *)ctx); @@ -727,7 +726,6 @@ set_authorizer(VALUE self, VALUE authorizer) CHECK(ctx->db, status); - rb_iv_set(self, "@authorizer", authorizer); RB_OBJ_WRITE(self, &ctx->authorizer, authorizer); return self; diff --git a/lib/sqlite3/database.rb b/lib/sqlite3/database.rb index 4759f405..6fbff7b0 100644 --- a/lib/sqlite3/database.rb +++ b/lib/sqlite3/database.rb @@ -171,8 +171,6 @@ def initialize file, options = {}, zvfs = nil end end - @tracefunc = nil - @authorizer = nil @progress_handler = nil @collations = {} @functions = [] From d8171e3349ce9f2cb35d70b730a78b82c4760ac3 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 10 Aug 2026 15:47:46 -0400 Subject: [PATCH 5/5] Mark unused parameters and say what the collation release test guards pin_hash_value ignored its key and arg parameters without marking them, which trips -Wunused-parameter. Separately, the test asserting that replacing a collation releases the previous comparator passes on main as well, so it read as redundant to anyone checking it against an unfixed build. The parameters will use the UNUSED macro the extension already applies elsewhere, and the test will carry a note that it guards the design rather than the fix: pinning every comparator is what makes the lifetime of @collations matter. --- test/test_collation.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/test_collation.rb b/test/test_collation.rb index 2a168d38..d33e58ff 100644 --- a/test/test_collation.rb +++ b/test/test_collation.rb @@ -55,6 +55,9 @@ def test_collation_does_not_use_moved_comparator_after_gc_compaction assert_equal 1, @db.collations["foo"].calls.length end + # Passes on main as well. It guards the design rather than the fix: pinning + # every comparator makes the lifetime of @collations matter, so this rules + # out ever making it an append-only array. def test_replacing_a_collation_releases_the_previous_comparator 3.times { @db.collation "foo", ReleasableComparator.new } GC.start(full_mark: true, immediate_sweep: true)