Skip to content

Commit e1a3fcb

Browse files
committed
fix(Session): prevent dangling pointers in DeleteAllSessions
Remove database_ref_.Reset() calls during DeleteAllSessions() iteration. The issue: Reset() can trigger GC, which may finalize Session JS objects that are still in our iteration list, creating dangling pointers → SIGSEGV. Why this is safe: - Our iteration only uses pure C/C++ ops (no Napi calls that trigger GC) - When Sessions are later GC'd, Delete() returns early (session_ is nullptr) - Napi::ObjectReference destructor handles reference cleanup automatically
1 parent 0157393 commit e1a3fcb

File tree

1 file changed

+9
-17
lines changed

1 file changed

+9
-17
lines changed

src/sqlite_impl.cpp

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1515,22 +1515,22 @@ void DatabaseSync::RemoveSession(Session *session) {
15151515

15161516
void DatabaseSync::DeleteAllSessions() {
15171517
// Copy and clear sessions while holding the lock, then release lock
1518-
// before calling Reset() which can trigger GC. GC can finalize other
1519-
// Session objects which call Delete() -> RemoveSession() which also
1520-
// tries to lock sessions_mutex_. Since std::mutex is not recursive,
1521-
// we must release the lock before any operation that could trigger GC.
1518+
// before doing cleanup. GC can finalize Session objects which call
1519+
// Delete() -> RemoveSession() which also tries to lock sessions_mutex_.
1520+
// Since std::mutex is not recursive, we must release the lock first.
15221521
std::set<Session *> sessions_copy;
15231522
{
15241523
std::lock_guard<std::mutex> lock(sessions_mutex_);
15251524
sessions_copy = sessions_;
15261525
sessions_.clear(); // Clear first so RemoveSession() becomes a no-op
15271526
}
15281527

1529-
// TWO-PASS cleanup to prevent double-free race condition:
1530-
// Pass 1: Delete all SQLite sessions and clear session_ pointers.
1531-
// This MUST happen before any Reset() calls because Reset() can trigger GC,
1532-
// which may finalize other Session objects whose destructors call Delete().
1533-
// By clearing all session_ pointers first, those Delete() calls become no-ops.
1528+
// Delete all SQLite sessions and clear session_ pointers.
1529+
// We do NOT call database_ref_.Reset() here because:
1530+
// 1. Reset() can trigger GC which may finalize Session JS objects
1531+
// 2. That would destroy objects we're still iterating over (dangling pointers!)
1532+
// 3. The Sessions will release their references when naturally GC'd later,
1533+
// and their Delete() will be a no-op since session_ is nullptr.
15341534
for (auto *session : sessions_copy) {
15351535
if (session->GetSession()) {
15361536
sqlite3session_delete(session->GetSession());
@@ -1540,14 +1540,6 @@ void DatabaseSync::DeleteAllSessions() {
15401540
// Note: Don't null database_ - we need it to check IsOpen()
15411541
}
15421542
}
1543-
1544-
// Pass 2: Release database references. This can trigger GC which may finalize
1545-
// Session objects, but their Delete() calls are now no-ops (session_ == nullptr).
1546-
for (auto *session : sessions_copy) {
1547-
if (!session->database_ref_.IsEmpty()) {
1548-
session->database_ref_.Reset();
1549-
}
1550-
}
15511543
}
15521544

15531545
void DatabaseSync::AddBackup(BackupJob *backup) {

0 commit comments

Comments
 (0)