Skip to content

Commit 0157393

Browse files
committed
docs: document Bug 2d double-free fix in DeleteAllSessions
1 parent 3a6aaff commit 0157393

File tree

1 file changed

+38
-10
lines changed

1 file changed

+38
-10
lines changed

doc/todo/P02-investigate-flaky-native-crashes.md

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,29 @@ Affected tests: `extension-loading.test.ts`, `session-lifecycle.test.ts`, `sessi
8989

9090
**Commit**: `dadbb86`
9191

92+
### Bug 2d: Double-Free in DeleteAllSessions During GC
93+
94+
**The bug**: `DeleteAllSessions()` interleaved SQLite cleanup and `database_ref_.Reset()` calls in a single loop. When `Reset()` triggers GC, other Session objects in the iteration list may be finalized, causing their destructors to call `sqlite3session_delete()` on sessions that the loop will later process.
95+
96+
**How it manifested**:
97+
98+
1. `DeleteAllSessions()` iterates over `sessions_copy`
99+
2. For session X: `sqlite3session_delete(X)`, `X->session_ = nullptr`, `X->database_ref_.Reset()`
100+
3. `Reset()` triggers GC which finalizes session Y (also in `sessions_copy`, not yet processed)
101+
4. Y's destructor calls `Delete()``sqlite3session_delete(Y->session_)` (Y's session_ is still valid!)
102+
5. Loop continues to Y → calls `sqlite3session_delete(Y)` again → **double-free → SIGABRT**
103+
104+
**Fix**: Split cleanup into two passes:
105+
1. Pass 1: Delete all SQLite sessions and clear all `session_` pointers
106+
2. Pass 2: Release database references (can trigger GC, but Delete() is now a no-op for all sessions)
107+
108+
**Commit**: `3a6aaff`
109+
92110
---
93111

94112
## Fixes Implemented
95113

96-
### Session Fixes (Commits: a151cb6, fb283df, dadbb86)
114+
### Session Fixes (Commits: a151cb6, fb283df, dadbb86, 3a6aaff)
97115

98116
**[src/sqlite_impl.h](../../src/sqlite_impl.h)**:
99117

@@ -118,7 +136,7 @@ Napi::ObjectReference database_ref_;
118136
}
119137
```
120138
121-
3. `DeleteAllSessions()` - Release mutex before cleanup loop:
139+
3. `DeleteAllSessions()` - Two-pass cleanup to prevent double-free:
122140
123141
```cpp
124142
std::set<Session *> sessions_copy;
@@ -127,9 +145,18 @@ Napi::ObjectReference database_ref_;
127145
sessions_copy = sessions_;
128146
sessions_.clear(); // RemoveSession() becomes no-op
129147
}
130-
// Now iterate WITHOUT holding mutex
148+
// Pass 1: Delete SQLite sessions and clear pointers
149+
for (auto *session : sessions_copy) {
150+
if (session->GetSession()) {
151+
sqlite3session_delete(session->GetSession());
152+
session->session_ = nullptr; // Makes Delete() a no-op
153+
}
154+
}
155+
// Pass 2: Release references (can trigger GC safely now)
131156
for (auto *session : sessions_copy) {
132-
// ... cleanup including database_ref_.Reset()
157+
if (!session->database_ref_.IsEmpty()) {
158+
session->database_ref_.Reset();
159+
}
133160
}
134161
```
135162

@@ -289,9 +316,10 @@ docker run --rm -v "$(pwd)":/host:ro node:22-alpine sh -c '\
289316

290317
## Commits Summary
291318

292-
| Commit | Description |
293-
| --------- | ------------------------------------------------ |
294-
| `a151cb6` | Add `database_ref_` to Session |
295-
| `fb283df` | Release `database_ref_` in DeleteAllSessions |
296-
| `dadbb86` | Release mutex before GC-triggering operations |
297-
| `5d86172` | Fix multi-process test expected values (unrelated) |
319+
| Commit | Description |
320+
| --------- | ---------------------------------------------------- |
321+
| `a151cb6` | Add `database_ref_` to Session |
322+
| `fb283df` | Release `database_ref_` in DeleteAllSessions |
323+
| `dadbb86` | Release mutex before GC-triggering operations |
324+
| `3a6aaff` | Two-pass cleanup to prevent double-free during GC |
325+
| `5d86172` | Fix multi-process test expected values (unrelated) |

0 commit comments

Comments
 (0)