Skip to content

Commit e46e1d2

Browse files
committed
Fix a GC compaction issue with busy_handler
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.
1 parent 5361528 commit e46e1d2

File tree

2 files changed

+23
-14
lines changed

2 files changed

+23
-14
lines changed

ext/sqlite3/database.c

+22-14
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,13 @@
1212

1313
VALUE cSqlite3Database;
1414

15+
static void
16+
database_mark(void *ctx)
17+
{
18+
sqlite3RubyPtr c = (sqlite3RubyPtr)ctx;
19+
rb_gc_mark(c->busy_handler);
20+
}
21+
1522
static void
1623
deallocate(void *ctx)
1724
{
@@ -31,15 +38,13 @@ database_memsize(const void *ctx)
3138
}
3239

3340
static const rb_data_type_t database_type = {
34-
"SQLite3::Backup",
35-
{
36-
NULL,
37-
deallocate,
38-
database_memsize,
41+
.wrap_struct_name = "SQLite3::Backup",
42+
.function = {
43+
.dmark = database_mark,
44+
.dfree = deallocate,
45+
.dsize = database_memsize,
3946
},
40-
0,
41-
0,
42-
RUBY_TYPED_WB_PROTECTED, // Not freed immediately because the dfree function do IOs.
47+
.flags = RUBY_TYPED_WB_PROTECTED, // Not freed immediately because the dfree function do IOs.
4348
};
4449

4550
static VALUE
@@ -202,10 +207,11 @@ trace(int argc, VALUE *argv, VALUE self)
202207
}
203208

204209
static int
205-
rb_sqlite3_busy_handler(void *ctx, int count)
210+
rb_sqlite3_busy_handler(void *context, int count)
206211
{
207-
VALUE self = (VALUE)(ctx);
208-
VALUE handle = rb_iv_get(self, "@busy_handler");
212+
sqlite3RubyPtr ctx = (sqlite3RubyPtr)context;
213+
214+
VALUE handle = ctx->busy_handler;
209215
VALUE result = rb_funcall(handle, rb_intern("call"), 1, INT2NUM(count));
210216

211217
if (Qfalse == result) { return 0; }
@@ -240,11 +246,13 @@ busy_handler(int argc, VALUE *argv, VALUE self)
240246
rb_scan_args(argc, argv, "01", &block);
241247

242248
if (NIL_P(block) && rb_block_given_p()) { block = rb_block_proc(); }
243-
244-
rb_iv_set(self, "@busy_handler", block);
249+
ctx->busy_handler = block;
245250

246251
status = sqlite3_busy_handler(
247-
ctx->db, NIL_P(block) ? NULL : rb_sqlite3_busy_handler, (void *)self);
252+
ctx->db,
253+
NIL_P(block) ? NULL : rb_sqlite3_busy_handler,
254+
(void *)ctx
255+
);
248256

249257
CHECK(ctx->db, status);
250258

ext/sqlite3/database.h

+1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
struct _sqlite3Ruby {
77
sqlite3 *db;
8+
VALUE busy_handler;
89
};
910

1011
typedef struct _sqlite3Ruby sqlite3Ruby;

0 commit comments

Comments
 (0)