Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User code can lead to infinite panics in interact in deadpool-sqlite #113

Closed
SergioBenitez opened this issue Jul 18, 2021 · 1 comment · Fixed by #115
Closed

User code can lead to infinite panics in interact in deadpool-sqlite #113

SergioBenitez opened this issue Jul 18, 2021 · 1 comment · Fixed by #115
Labels
A-sqlite Area: SQLite / deadpool-sqlite bug Category: This is a bug.

Comments

@SergioBenitez
Copy link

SergioBenitez commented Jul 18, 2021

Here's the interact method, for reference:

deadpool/sqlite/src/lib.rs

Lines 136 to 146 in 2c7a0b7

pub async fn interact<F, R>(&self, f: F) -> R
where
F: FnOnce(&rusqlite::Connection) -> R + Send + 'static,
R: Send + 'static,
{
let conn = self.conn.clone();
tokio::task::spawn_blocking(move || f(&*conn.lock().unwrap()))
.await
.unwrap()
}
}

If f() panics, it will do so while the lock is held. This poisons the lock. Every attempt to interact() thereafter will panic!() because of the unwrap() on line 144 (itself because of the unwrap() in 142). I would suggest the following changes:

  1. Use a non-poisoning lock, like parking_lot::Mutex().
  2. Propagate errors from spawn_blocking(), don't panic.

Alternatively, detect the error and close the connection so that the poisoned mutex is never reused.

bikeshedder added a commit that referenced this issue Jul 18, 2021
@bikeshedder
Copy link
Owner

bikeshedder commented Jul 18, 2021

I wonder what's the best practice. My gut feeling is the following:

  • The interact method should return a Result<InteractError, R>:
    /// This error is returned when the call to [`Connection::interact`]
    /// fails.
    #[derive(Debug)]
    pub enum InteractError {
        /// The provided callback panicked
        Panic(Box<dyn Any + Send + 'static>),
        /// The callback was aborted
        Aborted,
        /// rusqlite returned an error
        Rusqlite(rusqlite::Error),
    }
  • Panics should can be propagated using this struct. The InteractError has a variant called Rusqlite so users can use ? inside the function and no double nesting takes place.
  • When recycling connections detect the poisoned mutex and throw away that connection. Actually I feel like this is the correct approach anyways as that panic could've happened inside the rusqlite code and there is no safe way to check what caused the panic.

I just added this fix to the PR #115.

@bikeshedder bikeshedder added A-sqlite Area: SQLite / deadpool-sqlite bug Category: This is a bug. labels Jul 18, 2021
bikeshedder added a commit that referenced this issue Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sqlite Area: SQLite / deadpool-sqlite bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants