Skip to content

fix: prevent use-after-free during database invalidation#384

Closed
isaacrowntree wants to merge 1 commit intoOP-Engineering:mainfrom
isaacrowntree:fix/invalidate-race-condition
Closed

fix: prevent use-after-free during database invalidation#384
isaacrowntree wants to merge 1 commit intoOP-Engineering:mainfrom
isaacrowntree:fix/invalidate-race-condition

Conversation

@isaacrowntree
Copy link

Summary

Fixes an EXC_BAD_ACCESS crash caused by a race condition in DBHostObject::invalidate() where restartPool() creates new worker threads that can access db after opsqlite_close(db) has freed it.

Crash trace

sqlite3FreeCodecArg (sqlite3.c:111639)
sqlite3PagerClose (sqlite3.c:62507)
sqlite3BtreeClose (sqlite3.c:74836)
sqlite3Close (sqlite3.c:189787)
opsqlite::DBHostObject::invalidate (DBHostObject.cpp:679)
opsqlite::DBHostObject::~DBHostObject (DBHostObject.cpp:685)

Root cause

invalidate() calls thread_pool->restartPool() which:

  1. Sets done = true and joins existing threads (correct — waits for in-flight work)
  2. Creates NEW threads and sets 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 — accessing db concurrently with or after opsqlite_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 in invalidate() instead of restartPool().

shutdown() is essentially the first half of restartPool() — it sets done = 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() in utils.cpp checks the global opsqlite::invalidated flag (line 343) rather than the per-instance DBHostObject::invalidated flag. 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

  • Verified the fix compiles on iOS and Android
  • Confirmed invalidate() properly drains in-flight tasks before closing the database
  • No behavioral change for normal database operations — shutdown() is only called during invalidation/destruction

…nt use-after-free

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@isaacrowntree
Copy link
Author

Closing in favour of #385 which is a more complete fix — it also drains the thread pool in the close() JS function (not just invalidate()), and uses the existing waitFinished() method instead of adding a new shutdown() method.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant