diff --git a/libsql/src/local/connection.rs b/libsql/src/local/connection.rs index 7012651699..e8bbfaf1c9 100644 --- a/libsql/src/local/connection.rs +++ b/libsql/src/local/connection.rs @@ -504,7 +504,15 @@ impl Connection { let (callback, user_data) = match hook { Some(_) => { let callback = authorizer_callback as unsafe extern "C" fn(_, _, _, _, _, _) -> _; - let user_data = self as *const Connection as *mut ::std::os::raw::c_void; + // The callback user data must not point at this `Connection` value: + // `Connection` is `Clone`, clones share the raw sqlite handle, and + // SQLite may invoke the callback after the value that registered it + // has been dropped (#2272). Point at the shared `authorizer` + // allocation instead - it outlives the raw handle, because every + // owner of the handle (`Connection`, `Statement`, `Transaction`) + // holds a clone of this `Arc`. + let user_data = + Arc::as_ptr(&self.authorizer) as *mut ::std::os::raw::c_void; (Some(callback), user_data) } None => (None, std::ptr::null_mut()), @@ -701,8 +709,11 @@ unsafe extern "C" fn authorizer_callback( database_name: *const ::std::os::raw::c_char, accessor: *const ::std::os::raw::c_char, ) -> ::std::os::raw::c_int { - let conn = user_data as *const Connection; - let hook = unsafe { (*conn).authorizer.read() }; + let authorizer = user_data as *const RwLock>; + // SAFETY: `user_data` points at the `Arc` allocation behind the + // connection's shared `authorizer`, which outlives the raw sqlite handle + // this callback is registered on (see `Connection::authorizer`). + let hook = unsafe { (*authorizer).read() }; let hook = match &*hook { Some(hook) => hook, None => return ffi::SQLITE_OK, diff --git a/libsql/tests/integration_tests.rs b/libsql/tests/integration_tests.rs index 697ac220ef..c7d873bb08 100644 --- a/libsql/tests/integration_tests.rs +++ b/libsql/tests/integration_tests.rs @@ -925,3 +925,34 @@ fn assert_sqlite_error(res: Result, code: i32) { } } } + +#[tokio::test] +async fn test_authorizer_survives_dropped_registering_connection() { + // Regression test for #2272: the authorizer callback used to receive a raw + // pointer to the `Connection` value that registered it. `transaction()` + // clones the connection (shared sqlite handle, different struct address), + // so dropping the original freed the address SQLite later dereferenced - + // a heap use-after-free reachable from safe Rust (reporter's ASan trace). + let fired = Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let fired_hook = fired.clone(); + + let db = Database::open(":memory:").unwrap(); + let conn = db.connect().unwrap(); + conn.authorizer(Some(Arc::new(move |ctx| { + let _ = format!("{:?}", ctx.action); + fired_hook.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + Authorization::Allow + }))) + .unwrap(); + + let tx = conn.transaction().await.unwrap(); + // Free the exact struct whose address the pre-fix code handed to SQLite as + // the authorizer user data; the transaction's clone keeps the raw sqlite + // handle (and its callback registration) alive. + drop(conn); + + tx.execute("CREATE TABLE t(x INTEGER)", ()).await.unwrap(); + tx.commit().await.unwrap(); + + assert!(fired.load(std::sync::atomic::Ordering::SeqCst) > 0); +}