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

Fix segfault in sqlite when a future is aborted #1314

Closed
wants to merge 4 commits into from

Conversation

madadam
Copy link
Contributor

@madadam madadam commented Jul 7, 2021

This PR attempts to fix a memory corruption that happens when a future executing a db query is interrupted (more details in #1300). It is built on top of #1186, only the last commit is new. Neither #1186, nor bb62cf7 are by themselves enough to fix this issue, but together they do seem to be.

Some notes:

The fix consist of making sure that calls to sqlite3_reset and sqlite3_step are never called concurrently without synchronization. The synchronization is implemented using an atomic bitflag in StatementHandle. This should be more efficient than using a mutex, however it makes the code a bit more difficult to reason about. Also the performance difference may or might not be significant. The solution only protects against concurrent use of reset and step, not any other functions. This should be fine as those seem to be the only two functions that might possibly be called concurrently from multiple threads. On the other hand this means that the StatementHandle API is strictly speaking not sound and should probably be marked as unsafe. I decided not to do it to keep the changes in this PR minimal.

Closes #1300.

link2xt and others added 4 commits April 23, 2021 00:00
This guarantees that StatementHandle is never used after calling
`sqlite3_finalize`. Now `sqlite3_finalize` is only called when
StatementHandle is dropped.
Otherwise some tests fail to close connection.
@madadam madadam changed the title Fix segfault in sqlite when future is aborted Fix segfault in sqlite when a future is aborted Jul 7, 2021
@abonander
Copy link
Collaborator

abonander commented Jul 23, 2021

@madadam if you're still interested in getting this merged, I have some concerns:

  • I don't want to use a hand-rolled mutex. If nothing else, maybe parking_lot::RawMutex.
  • When the reset is called on-drop, it has the potential to block because it's waiting for sqlite3_step() to complete and release the mutex, which isn't ideal.

It's a non-trivial refactor, but I think we should actually just call sqlite3_reset() on the worker thread to avoid any race conditions, and then provide a oneshot channel like with Step for any tasks that are waiting for the reset to complete before continuing.

I started on this in the ab/sqlite-segfaults branch using cherry-picks of your and @link2xt's commits but didn't have time to finish.

@madadam
Copy link
Contributor Author

madadam commented Jul 27, 2021

@abonander I checked the ab/sqlite-segfaults branch and the approach seems reasonable to me. Definitely clearer than what I came up with. What is still missing there before it can be merged? Maybe I can help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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