Memory corruption (SIGSEGV) when a future executing a sqlite query is aborted

[madadam]

It seems there is some invalid memory access in the sqlite backend which sometimes happen when a future that is executing some query is aborted before it has chance to run to completion. Here is a minimal reproductible case:

#[tokio::test(flavor = "multi_thread")]
async fn sqlx_memory_corruption() {
    use sqlx::{sqlite::SqliteConnectOptions, ConnectOptions};
    use std::{str::FromStr, time::Duration};
    use tokio::time;

    let mut conn = SqliteConnectOptions::from_str(":memory:")
        .unwrap()
        .connect()
        .await
        .unwrap();

    sqlx::query("CREATE TABLE stuff (name INTEGER, value INTEGER)")
        .execute(&mut conn)
        .await
        .unwrap();

    let task = tokio::spawn(async move {
        for i in 0..1000 {
            sqlx::query("INSERT INTO stuff (name, value) VALUES (?, ?)")
                .bind(i)
                .bind(0)
                .execute(&mut conn)
                .await
                .unwrap();
        }
    });

    time::sleep(Duration::from_millis(1)).await;

    task.abort();
}

(it might need to be run a couple of times before the bug is observed).

My hypothesis is that when the future is aborted, some sqlx objects might be dropped in unexpected order. e.g. a connection might be dropped before a statement handle, etc... which causes the underlying sqlite3 resources to be cleaned up in wrong order, causing an use-after-free or similar. I haven't verified this though.

[madadam]

Spent more time on this and I think I have an idea about what's going on. There seems to be a race condition between the StatementWorker and StatementResetter - sometimes the worker tries to step the statement at the same time the resetter tries to reset it which leads to memory corruption. An obvious fix would be to protect the statement pointer with a mutex, but perhaps there is a better way?

[andrewwhitehead]

Maybe related to this PR? #1186

[madadam]

Yep, seem to be related. Although I don't think that PR fixes this issue because they still seem to be unconditionally resetting the statement in the resetter regardless of how many owners the underlying Arc has. But at least it seems to be a step in the right direction.