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 39a1b2b9..c4788f5a 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -22,5 +22,17 @@ def i_am_running_in_valgrind 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(/>>>.*<<