From e46e1d217826dc94537e0cd17a1e5d571d952511 Mon Sep 17 00:00:00 2001
From: Jean Boussier <jean.boussier@gmail.com>
Date: Tue, 9 Jan 2024 13:11:03 +0100
Subject: [PATCH] Fix a GC compaction issue with `busy_handler`

Previous discussion in https://github.com/sparklemotion/sqlite3-ruby/pull/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.
---
 ext/sqlite3/database.c | 36 ++++++++++++++++++++++--------------
 ext/sqlite3/database.h |  1 +
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/ext/sqlite3/database.c b/ext/sqlite3/database.c
index 7180c0b0..411e6336 100644
--- a/ext/sqlite3/database.c
+++ b/ext/sqlite3/database.c
@@ -12,6 +12,13 @@
 
 VALUE cSqlite3Database;
 
+static void
+database_mark(void *ctx)
+{
+    sqlite3RubyPtr c = (sqlite3RubyPtr)ctx;
+    rb_gc_mark(c->busy_handler);
+}
+
 static void
 deallocate(void *ctx)
 {
@@ -31,15 +38,13 @@ database_memsize(const void *ctx)
 }
 
 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,
     },
-    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
@@ -202,10 +207,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; }
@@ -240,11 +246,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);
 
diff --git a/ext/sqlite3/database.h b/ext/sqlite3/database.h
index ad3527a2..56833020 100644
--- a/ext/sqlite3/database.h
+++ b/ext/sqlite3/database.h
@@ -5,6 +5,7 @@
 
 struct _sqlite3Ruby {
     sqlite3 *db;
+    VALUE busy_handler;
 };
 
 typedef struct _sqlite3Ruby sqlite3Ruby;