fix: prevent use-after-free during database invalidation#384
Closed
isaacrowntree wants to merge 1 commit intoOP-Engineering:mainfrom
Closed
fix: prevent use-after-free during database invalidation#384isaacrowntree wants to merge 1 commit intoOP-Engineering:mainfrom
isaacrowntree wants to merge 1 commit intoOP-Engineering:mainfrom
Conversation
…nt use-after-free Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
Closing in favour of #385 which is a more complete fix — it also drains the thread pool in the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an
EXC_BAD_ACCESScrash caused by a race condition inDBHostObject::invalidate()whererestartPool()creates new worker threads that can accessdbafteropsqlite_close(db)has freed it.Crash trace
Root cause
invalidate()callsthread_pool->restartPool()which:done = trueand joins existing threads (correct — waits for in-flight work)done = false(incorrect for invalidation)After
restartPool()returns, the new threads are running. If any pending tasks remain in the queue, they can be dequeued and executed by these new threads — accessingdbconcurrently with or afteropsqlite_close(db). This causes a use-after-free in SQLCipher's codec teardown.Fix
Add a
ThreadPool::shutdown()method that drains the pool and stops threads without restarting. Use it ininvalidate()instead ofrestartPool().shutdown()is essentially the first half ofrestartPool()— it setsdone = true, wakes all threads, and joins them. It does not create new threads afterward, because the database is about to be closed and no further work should be processed.Additional note
There is also a secondary issue where
promisify()inutils.cppchecks the globalopsqlite::invalidatedflag (line 343) rather than the per-instanceDBHostObject::invalidatedflag. This means if multiple databases are open, closing one does not prevent queued tasks for that specific instance from executing. This PR does not address that issue but it would be worth considering as a follow-up.Test plan
invalidate()properly drains in-flight tasks before closing the databaseshutdown()is only called during invalidation/destruction