Skip to content

Commit

Permalink
Fix a GC compaction issue with busy_handler
Browse files Browse the repository at this point in the history
Previous discussion in sparklemotion#458

Storing `VALUE self` as context for `rb_sqlite3_busy_handler` is unsafe
because as of Ruby 2.7, the GC compactor may move objects around
which can lead to this reference pointing to either another random
object or to garbage.

Instead we can store the callback reference inside the malloced
struct (`sqlite3Ruby`) which can't possibly move, and then inside
the handler, get the callback reference from that struct.

This however requires to define a mark function for the database
object, and while I was at it, I implemented compaction support
for it so we don't pin that proc.
  • Loading branch information
byroot committed Jan 9, 2024
1 parent 5361528 commit b024d9b
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 14 deletions.
44 changes: 30 additions & 14 deletions ext/sqlite3/database.c
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@

VALUE cSqlite3Database;

static void
database_mark(void *ctx)
{
sqlite3RubyPtr c = (sqlite3RubyPtr)ctx;
rb_gc_mark_movable(c->busy_handler);
}

static void
deallocate(void *ctx)
{
Expand All @@ -30,16 +37,22 @@ database_memsize(const void *ctx)
return sizeof(*c);
}

static void
database_update_references(void *ctx)
{
sqlite3RubyPtr c = (sqlite3RubyPtr)ctx;
c->busy_handler = rb_gc_location(c->busy_handler);
}

static const rb_data_type_t database_type = {
"SQLite3::Backup",
{
NULL,
deallocate,
database_memsize,
.wrap_struct_name = "SQLite3::Backup",
.function = {
.dmark = database_mark,
.dfree = deallocate,
.dsize = database_memsize,
.dcompact = database_update_references,
},
0,
0,
RUBY_TYPED_WB_PROTECTED, // Not freed immediately because the dfree function do IOs.
.flags = RUBY_TYPED_WB_PROTECTED, // Not freed immediately because the dfree function do IOs.
};

static VALUE
Expand Down Expand Up @@ -202,10 +215,11 @@ trace(int argc, VALUE *argv, VALUE self)
}

static int
rb_sqlite3_busy_handler(void *ctx, int count)
rb_sqlite3_busy_handler(void *context, int count)
{
VALUE self = (VALUE)(ctx);
VALUE handle = rb_iv_get(self, "@busy_handler");
sqlite3RubyPtr ctx = (sqlite3RubyPtr)context;

VALUE handle = ctx->busy_handler;
VALUE result = rb_funcall(handle, rb_intern("call"), 1, INT2NUM(count));

if (Qfalse == result) { return 0; }
Expand Down Expand Up @@ -240,11 +254,13 @@ busy_handler(int argc, VALUE *argv, VALUE self)
rb_scan_args(argc, argv, "01", &block);

if (NIL_P(block) && rb_block_given_p()) { block = rb_block_proc(); }

rb_iv_set(self, "@busy_handler", block);
ctx->busy_handler = block;

status = sqlite3_busy_handler(
ctx->db, NIL_P(block) ? NULL : rb_sqlite3_busy_handler, (void *)self);
ctx->db,
NIL_P(block) ? NULL : rb_sqlite3_busy_handler,
(void *)ctx
);

CHECK(ctx->db, status);

Expand Down
1 change: 1 addition & 0 deletions ext/sqlite3/database.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

struct _sqlite3Ruby {
sqlite3 *db;
VALUE busy_handler;
};

typedef struct _sqlite3Ruby sqlite3Ruby;
Expand Down

0 comments on commit b024d9b

Please sign in to comment.