fix: null sqlite3 handle after close to prevent double-free (#2251)

[wasim-builds]

Problem

Closes #2251

The local connection's sqlite3 handle is sqlite3_close_v2'd twice on teardown. LibsqlConnection's Drop calls disconnect(), then the inner Connection's Drop calls it again. Both go through Connection::disconnect (connection.rs:110). disconnect() isn't idempotent — the Arc::get_mut(drop_ref) guard only checks unique ownership (true both times) and doesn't record that the handle was already closed.

On Windows this is a hard STATUS_ACCESS_VIOLATION (0xC0000005); on Linux it doesn't fault (glibc keeps the freed page mapped) but the double free is real, as valgrind shows — 800 errors from 1 context at 800 iterations, one per connection.

Root Cause

LibsqlConnection::drop -> self.conn.disconnect()   <- first close
Connection::drop       -> self.disconnect()         <- second close

Both calls succeed because Arc::get_mut(drop_ref) returns Some both times (the first close doesn't mark the handle as closed).

Fix

After calling sqlite3_close_v2, set self.raw to std::ptr::null_mut(). This makes disconnect() idempotent — the second call sees a null pointer, and even if Arc::get_mut passes, calling sqlite3_close_v2(null) is documented as safe (no-op) in SQLite.

Changes

• libsql/src/local/connection.rs: Null raw after sqlite3_close_v2 in disconnect()

Reproduction (from issue)

for _ in 0..50_000 {
    let rt = tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap();
    rt.block_on(async {
        let db = libsql::Builder::new_local(":memory:").build().await.unwrap();
        let conn = db.connect().unwrap();
        conn.execute("CREATE TABLE t(x INTEGER)", ()).await.unwrap();
    });
}

With valgrind: 800 errors at 800 iterations. On Windows: STATUS_ACCESS_VIOLATION within first few hundred iterations.

Testing

• cargo check passes
• The fix is minimal and makes disconnect() idempotent as suggested in the issue

Co-Authored-By: Claude noreply@anthropic.com

[wasim-builds]

Hi! Bumping this — it's a small 4-line fix that nulls the sqlite3 handle after close to prevent double-free. Simple and ready for review!

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[ki7dk]

We hit this in production and have been carrying this exact line as a patch on a vendored 0.9.30 since mid-August, so I can confirm it fixes it.

Our setup opens a local connection per request on a multi-threaded runtime, so it was double-closing on every query. Symptom was SQLite failure: bad parameter or other API misuse in about 1 test run out of 260, a different test each time, and never with --test-threads=1. Same bug as #1189, I think. The second close lands on whatever connection reused the freed block, so the error surfaces somewhere unrelated, and we spent a while hunting a race in our own code first.

The valgrind repro in #2251 is deterministic on Linux but doesn't fault. macOS Guard Malloc unmaps the freed page instead of leaving the stale CLOSED marker in it, so five single-threaded iterations are enough to crash:

[dependencies]
libsql = { version = "=0.9.30", default-features = false, features = ["core"] }
tokio  = { version = "1", features = ["rt-multi-thread", "macros"] }

#[tokio::main]
async fn main() {
    let db = libsql::Builder::new_local(":memory:").build().await.unwrap();
    for i in 0..5 {
        let conn = db.connect().unwrap();
        let mut rows = conn.query("SELECT 1", ()).await.unwrap();
        rows.next().await.unwrap();
        drop(rows);
        drop(conn);
        println!("closed connection {i}");
    }
}

cargo run                                                    # exit 0
DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib cargo run    # SIGSEGV, exit 139

Ran it again today against crates.io 0.9.30 on macOS 26.6.2 / arm64 / rustc 1.92.0, 5 of 5 crash. With this PR applied, 5 of 5 clean. Backtrace from when we first diagnosed it:

* thread #1, stop reason = EXC_BAD_ACCESS (code=1, address=0x17213bd41)
  frame #0: sqlite3SafetyCheckSickOrOk    sqlite3.c:36835   eOpenState = db->eOpenState;
  frame #1: sqlite3Close(db=…, forceZombie=1)  sqlite3.c:182133
  frame #2: sqlite3_close_v2              sqlite3.c:182234
  frame #3: libsql::local::connection::Connection::disconnect   connection.rs:110
  frame #4: <libsql::local::connection::Connection as Drop>::drop  connection.rs:40
  frame #5: core::ptr::drop_in_place<libsql::local::connection::Connection>
  frame #6: core::ptr::drop_in_place<libsql::local::impls::LibsqlConnection>

The inner Connection is dropping inside a LibsqlConnection::drop that already called disconnect() (frames 5 and 6).

We looked at the other suggestion in #2251, removing the wrapper's Drop, and preferred this one: disconnect() is public, and it stays non-idempotent no matter how many Drop impls end up calling it. Might be worth a line in the PR description that handle() returns null after a close instead of a dangling pointer, and that hand-calling disconnect() and then using the connection gets you null-pointer behaviour rather than a use-after-free. Neither of those makes disconnect() safe to call by hand.

Since we've been running it: test suite went from ~1 failure in 260 runs to 400 runs clean, and a loop of 8 threads doing 2000 connect/query/drop cycles each, which used to segfault or hang most of the time, has been clean.

That repro is macOS-only, so it's no use in CI here. A plain unit test does the job though, and it fails on main:

#[tokio::test]
async fn disconnect_is_idempotent() {
    let temp_dir = tempfile::tempdir().unwrap();
    let path = temp_dir.path().join("local.db");
    let db = Database::new(path.to_str().unwrap().to_string(), OpenFlags::default());
    let mut conn = Connection::connect(&db).unwrap();
    assert!(!conn.handle().is_null());

    conn.disconnect();

    assert!(conn.handle().is_null(), "disconnect() left a dangling handle");
    conn.disconnect();
}

Goes in the existing mod tests at the bottom of connection.rs, and tempfile is already a dev-dependency. On unpatched code it panics with disconnect() left a dangling handle. Happy to send it as a PR against this branch if you want it.

The failing check doesn't look related to the diff, fwiw. It's test_many_concurrent in libsql-server failing on SQLITE_BUSY, and that test builds its connections through MakeLegacyConnection/libsql-sys so it never touches local::Connection. Both jobs were green on the commit before main got merged in, so a rerun would probably clear it.

Minor nit, ignore if you like: the assignment doesn't need to be inside the unsafe block.

@penberg any chance of getting this one reviewed?