fix: collation use-after-free, and callback-lifetime follow-ups

CHANGELOG.md

@@ -6,10 +6,24 @@
 
 - Raise `RangeError` when a SQL string, a bound String or Blob value, or a String or Blob returned from a custom function or aggregate is longer than `INT_MAX` bytes. Previously the length was truncated to 32 bits, so SQLite could read only part of the string and drop trailing clauses from a query. See [GHSA-4fqr-96q8-xrfx](https://github.com/sparklemotion/sqlite3-ruby/security/advisories/GHSA-4fqr-96q8-xrfx) for more information. #762 @flavorjones
 - Fix a crash when a custom aggregate's `finalize` returns a value that can't be converted to a SQLite result. The exception is now raised from the query and the connection remains usable. #762 @flavorjones
+- Fix a use-after-free when the hash returned by `Database#collations` is cleared, modified or frozen. GC could free a comparator that SQLite still called. When that hash is frozen, `Database#collation` still raises `FrozenError`, but the new comparator stays registered and in use. #764 @flavorjones
+- Fix a deadlock when a collation comparator, `trace` block, `busy_handler` block or authorizer raises, or when an interrupt such as `Thread#raise` arrives while an aggregate's handler is being created. Another thread that used the connection blocked forever. The exception is now raised from the query and the connection remains usable. See "Changed" below for what SQLite does before the exception is raised. #764 @flavorjones
+- Fix a crash when a custom function or aggregate uses `throw`. The throw now unwinds to its `catch`. #764 @flavorjones
+
+### Changed
+
+- When a callback raises or throws, SQLite now finishes its current call before the exception is raised, instead of being unwound in the middle of it. #764 @flavorjones
+  - When a `trace` block raises, SQLite runs the statement up to its first row, or to completion, before the exception is raised. Previously the statement stopped where the block raised. For example, an `INSERT` whose `trace` block raises now inserts its row.
+  - The other callbacks in that SQLite call skip their Ruby code: collation comparators return 0, functions and aggregates report an error to SQLite, the busy handler stops retrying and the authorizer denies.
+  - `Database#execute_batch2` now raises the exception from a custom function, instead of `SQLite3::SQLException` "Ruby Exception occurred".
+  - An exception from a callback that SQLite runs during `Statement#reset!` or `Database#close` is discarded.
+- The extension keeps per-fiber state in `Thread.current[:__sqlite3_callback_frames__]`. A query raises `FrozenError` on a `Thread` that was frozen before its first query. #764 @flavorjones
+- When SQLite can't allocate an aggregate's context, the query raises `SQLite3::MemoryException` instead of aborting the process. #764 @flavorjones
 
 ### Improved
 
 - `Statement#step` no longer allocates and discards an array when the statement has no more rows. This saves one object per statement execution. #758 @thomasklemm
+- `ObjectSpace.dump` and heap profilers now report `SQLite3::Database` and `SQLite3::Statement` objects under their own type names, instead of `SQLite3::Backup`. #764 @flavorjones
 
 ### Documentation
 

ext/sqlite3/aggregator.c

@@ -14,169 +14,149 @@ static VALUE cAggregatorWrapper;
  * the xFinal callback.
  *
  * An AggregatorInstance holds the following instance variables:
- * -handler_instance: the instance to call `step` and `finalize` on.
- * -exc_status:       status returned by rb_protect.
- *                    != 0 if an exception occurred. If an exception occurred
- *                    `step` and `finalize` won't be called any more. */
+ * -handler_instance: the instance to call `step` and `finalize` on. */
 static VALUE cAggregatorInstance;
 
-typedef struct rb_sqlite3_protected_funcall_args {
-    VALUE self;
-    ID method;
-    int argc;
-    VALUE *params;
-    sqlite3_context *result_ctx;
-} protected_funcall_args_t;
-
-/* why isn't there something like this in the ruby API? */
-static VALUE
-rb_sqlite3_protected_funcall_body(VALUE protected_funcall_args_ptr)
-{
-    protected_funcall_args_t *args =
-        (protected_funcall_args_t *)protected_funcall_args_ptr;
-
-    VALUE result = rb_funcall2(args->self, args->method, args->argc, args->params);
-
-    if (args->result_ctx) {
-        set_sqlite3_func_result(args->result_ctx, result);
-    }
+/* interned at init, because interning can allocate and rb_sqlite3_aggregator_pin
+ * runs during GC */
+static ID id_instances;
+static ID id_handler_instance;
 
-    return result;
-}
-
-static VALUE
-rb_sqlite3_protected_funcall(VALUE self, ID method, int argc, VALUE *params,
-                             sqlite3_context *result_ctx, int *exc_status)
-{
-    protected_funcall_args_t args = {
-        .self = self, .method = method, .argc = argc, .params = params, .result_ctx = result_ctx
-    };
-    return rb_protect(rb_sqlite3_protected_funcall_body, (VALUE)(&args), exc_status);
-}
+/* sqlite's aggregate context for one invocation of an aggregate function. */
+typedef struct {
+    /* the AggregatorInstance: Qfalse until the first callback creates it, Qnil
+     * once xFinal destroys it */
+    VALUE inst;
+    /* != 0 once a callback for this invocation raised, threw or was skipped.
+     * After that, `step` and `finalize` won't be called any more. */
+    int exc_status;
+} aggregate_context_t;
+
+typedef struct {
+    sqlite3_context *ctx;
+    aggregate_context_t *agg;
+    int argc;
+    sqlite3_value **argv;
+} aggregator_call_t;
 
-/* called in rb_sqlite3_aggregator_step and rb_sqlite3_aggregator_final. It
- * checks if the execution context already has an associated instance. If it
- * has one, it returns it. If there is no instance yet, it creates one and
- * associates it with the context. */
+/* called in the step and final callbacks. Returns the invocation's handler
+ * instance, creating it on the first call. */
 static VALUE
-rb_sqlite3_aggregate_instance(sqlite3_context *ctx)
+rb_sqlite3_aggregate_handler_instance(aggregator_call_t *call)
 {
-    VALUE aw = (VALUE) sqlite3_user_data(ctx);
-    VALUE handler_klass = rb_iv_get(aw, "-handler_klass");
-    VALUE inst;
-    VALUE *inst_ptr = sqlite3_aggregate_context(ctx, (int)sizeof(VALUE));
+    VALUE aw, handler_instance, inst;
 
-    if (!inst_ptr) {
-        rb_fatal("SQLite is out-of-memory");
+    if (call->agg->inst == Qnil) {
+        rb_fatal("SQLite called us back on an already destroyed aggregate instance");
     }
 
-    inst = *inst_ptr;
-
-    if (inst == Qfalse) { /* Qfalse == 0 */
-        VALUE instances = rb_iv_get(aw, "-instances");
-        int exc_status;
+    if (call->agg->inst == Qfalse) { /* Qfalse == 0 */
+        aw = (VALUE) sqlite3_user_data(call->ctx);
+        handler_instance = rb_funcall(rb_ivar_get(aw, rb_intern("-handler_klass")), rb_intern("new"), 0);
 
         inst = rb_class_new_instance(0, NULL, cAggregatorInstance);
-        rb_iv_set(inst, "-handler_instance", rb_sqlite3_protected_funcall(
-                      handler_klass, rb_intern("new"), 0, NULL, NULL, &exc_status));
-        rb_iv_set(inst, "-exc_status", INT2NUM(exc_status));
-
-        rb_ary_push(instances, inst);
+        rb_ivar_set(inst, id_handler_instance, handler_instance);
+        rb_ary_push(rb_ivar_get(aw, id_instances), inst);
 
-        *inst_ptr = inst;
+        call->agg->inst = inst;
     }
 
-    if (inst == Qnil) {
-        rb_fatal("SQLite called us back on an already destroyed aggregate instance");
-    }
-
-    return inst;
+    return rb_ivar_get(call->agg->inst, id_handler_instance);
 }
 
-/* called by rb_sqlite3_aggregator_final. Unlinks and frees the
- * aggregator_instance_t, so the handler_instance won't be marked any more
- * and Ruby's GC may free it. */
+/* called by rb_sqlite3_aggregator_final, after any skipped or failed callback
+ * too, so it runs no Ruby code. Unlinks the instance, so the handler_instance
+ * won't be marked any more and Ruby's GC may free it. */
 static void
-rb_sqlite3_aggregate_instance_destroy(sqlite3_context *ctx)
+rb_sqlite3_aggregate_instance_destroy(sqlite3_context *ctx, aggregate_context_t *agg)
 {
-    VALUE aw = (VALUE) sqlite3_user_data(ctx);
-    VALUE instances = rb_iv_get(aw, "-instances");
-    VALUE *inst_ptr = sqlite3_aggregate_context(ctx, 0);
-    VALUE inst;
+    VALUE instances = rb_ivar_get((VALUE) sqlite3_user_data(ctx), id_instances);
+    long i;
 
-    if (!inst_ptr || !(inst = *inst_ptr)) {
+    if (!agg->inst) {
         return;
     }
 
-    if (inst == Qnil) {
+    if (agg->inst == Qnil) {
         rb_fatal("attempt to destroy aggregate instance twice");
     }
 
-    rb_iv_set(inst, "-handler_instance", Qnil); // may catch use-after-free
-    if (rb_ary_delete(instances, inst) == Qnil) {
+    rb_ivar_set(agg->inst, id_handler_instance, Qnil); // may catch use-after-free
+    for (i = 0; i < RARRAY_LEN(instances) && RARRAY_AREF(instances, i) != agg->inst; i++);
+    if (i == RARRAY_LEN(instances)) {
         rb_fatal("must be in instances at that point");
     }
+    rb_ary_delete_at(instances, i);
 
-    *inst_ptr = Qnil;
+    agg->inst = Qnil;
+}
+
+static VALUE
+rb_sqlite3_aggregator_step_protected(VALUE call_value)
+{
+    aggregator_call_t *call = (aggregator_call_t *)call_value;
+    VALUE handler_instance = rb_sqlite3_aggregate_handler_instance(call);
+    VALUE params = rb_ary_new2(call->argc);
+    int i;
+
+    for (i = 0; i < call->argc; i++) {
+        rb_ary_push(params, sqlite3val2rb(call->argv[i]));
+    }
+
+    return rb_apply(handler_instance, rb_intern("step"), params);
+}
+
+static VALUE
+rb_sqlite3_aggregator_final_protected(VALUE call_value)
+{
+    aggregator_call_t *call = (aggregator_call_t *)call_value;
+    VALUE handler_instance = rb_sqlite3_aggregate_handler_instance(call);
+
+    set_sqlite3_func_result(call->ctx, rb_funcall(handler_instance, rb_intern("finalize"), 0));
+
+    return Qnil;
 }
 
 static void
 rb_sqlite3_aggregator_step(sqlite3_context *ctx, int argc, sqlite3_value **argv)
 {
-    VALUE inst = rb_sqlite3_aggregate_instance(ctx);
-    VALUE handler_instance = rb_iv_get(inst, "-handler_instance");
-    VALUE *params = NULL;
-    VALUE params_handle = 0;
-    VALUE one_param;
-    int exc_status = NUM2INT(rb_iv_get(inst, "-exc_status"));
-    int i;
+    aggregator_call_t call = {
+        .ctx = ctx, .agg = sqlite3_aggregate_context(ctx, (int)sizeof(aggregate_context_t)), .argc = argc, .argv = argv
+    };
 
-    if (exc_status) {
+    if (!call.agg) {
+        sqlite3_result_error_nomem(ctx);
         return;
     }
 
-    if (argc == 1) {
-        one_param = sqlite3val2rb(argv[0]);
-        params = &one_param;
-    }
-    if (argc > 1) {
-        /* ALLOCV memory is conservatively marked, so stored VALUEs survive a
-         * GC raised by a later sqlite3val2rb call. */
-        params = ALLOCV_N(VALUE, params_handle, argc);
-        for (i = 0; i < argc; i++) {
-            params[i] = sqlite3val2rb(argv[i]);
-        }
+    if (!call.agg->exc_status) {
+        rb_sqlite3_protect_callback(rb_sqlite3_aggregator_step_protected, (VALUE)&call, &call.agg->exc_status);
     }
-    rb_sqlite3_protected_funcall(
-        handler_instance, rb_intern("step"), argc, params, NULL, &exc_status);
-    if (argc > 1) {
-        ALLOCV_END(params_handle);
-    }
-
-    rb_iv_set(inst, "-exc_status", INT2NUM(exc_status));
 }
 
 /* we assume that this function is only called once per execution context */
 static void
 rb_sqlite3_aggregator_final(sqlite3_context *ctx)
 {
-    VALUE inst = rb_sqlite3_aggregate_instance(ctx);
-    VALUE handler_instance = rb_iv_get(inst, "-handler_instance");
-    int exc_status = NUM2INT(rb_iv_get(inst, "-exc_status"));
+    aggregator_call_t call = { .ctx = ctx, .agg = sqlite3_aggregate_context(ctx, (int)sizeof(aggregate_context_t)) };
+
+    if (!call.agg) {
+        sqlite3_result_error_nomem(ctx);
+        return;
+    }
 
-    if (!exc_status) {
-        rb_sqlite3_protected_funcall(
-            handler_instance, rb_intern("finalize"), 0, NULL, ctx, &exc_status);
+    if (!call.agg->exc_status) {
+        rb_sqlite3_protect_callback(rb_sqlite3_aggregator_final_protected, (VALUE)&call, &call.agg->exc_status);
     }
 
-    if (exc_status) {
+    if (call.agg->exc_status) {
         /* the user should never see this, as Statement.step() will pick up the
          * outstanding exception and raise it instead of generating a new one
          * for SQLITE_ERROR with message "Ruby Exception occurred" */
         sqlite3_result_error(ctx, "Ruby Exception occurred", -1);
     }
 
-    rb_sqlite3_aggregate_instance_destroy(ctx);
+    rb_sqlite3_aggregate_instance_destroy(ctx, call.agg);
 }
 
 /* call-seq: define_aggregator2(aggregator)
@@ -267,9 +247,16 @@ rb_sqlite3_define_aggregator2(VALUE self, VALUE aggregator, VALUE ruby_name)
 }
 
 void
-rb_sqlite3_aggregator_pin_instances(VALUE aw)
+rb_sqlite3_aggregator_pin(VALUE aggregators)
 {
-    rb_sqlite3_pin_array_and_contents(rb_iv_get(aw, "-instances"));
+    long i;
+
+    if (NIL_P(aggregators) || !aggregators) { return; }
+
+    rb_sqlite3_pin_array_and_contents(aggregators);
+    for (i = 0; i < RARRAY_LEN(aggregators); i++) {
+        rb_sqlite3_pin_array_and_contents(rb_ivar_get(RARRAY_AREF(aggregators, i), id_instances));
+    }
 }
 
 void
@@ -281,4 +268,7 @@ rb_sqlite3_aggregator_init(void)
 
     cAggregatorInstance = rb_funcall(rb_cClass, rb_intern("new"), 0);
     rb_gc_register_mark_object(cAggregatorInstance);
+
+    id_instances = rb_intern("-instances");
+    id_handler_instance = rb_intern("-handler_instance");
 }

ext/sqlite3/aggregator.h

@@ -7,8 +7,9 @@ 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);
+/* Pins every aggregator wrapper and its live instances. sqlite holds raw
+ * pointers to the wrappers, and stores each instance's VALUE in its aggregate
+ * context. */
+void rb_sqlite3_aggregator_pin(VALUE aggregators);
 
 #endif

ext/sqlite3/backup.c

@@ -113,11 +113,16 @@ static VALUE
 step(VALUE self, VALUE nPage)
 {
     sqlite3BackupRubyPtr ctx;
-    int status;
+    rb_sqlite3_callback_frame_t frame;
+    int pages, status, callback_state;
 
     TypedData_Get_Struct(self, sqlite3BackupRuby, &backup_type, ctx);
     REQUIRE_OPEN_BACKUP(ctx);
-    status = sqlite3_backup_step(ctx->p, NUM2INT(nPage));
+    pages = NUM2INT(nPage);
+
+    rb_sqlite3_push_callback_frame(&frame);
+    status = sqlite3_backup_step(ctx->p, pages);
+    if ((callback_state = rb_sqlite3_pop_callback_frame(&frame))) { rb_jump_tag(callback_state); }
     return INT2NUM(status);
 }