Skip to content

Commit 14e802d

Browse files
trivikraduh95
authored andcommitted
sqlite: invalidate sessions when closing database
DatabaseSync::DeleteSessions() deleted native sqlite3_session handles without clearing the pointers held by their Session wrappers. Reopening the database allowed stale session handles to be used, crashing the process. Track Session wrappers and delete sessions through Session::Delete() so the wrappers are marked closed. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: codex:gpt-5.6-sol PR-URL: #64783Fixes: #64782 Reviewed-By: Aviv Keller <me@aviv.sh>
1 parent 31522c4 commit 14e802d

3 files changed

Lines changed: 37 additions & 7 deletions

File tree

‎src/node_sqlite.cc‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -912,10 +912,9 @@ void DatabaseSync::RemoveBackup(BackupJob* job) {
912912
voidDatabaseSync::DeleteSessions() {
913913
// all attached sessions need to be deleted before the database is closed
914914
// https://www.sqlite.org/session/sqlite3session_create.html
915-
for (auto* session : sessions_) {
916-
sqlite3session_delete(session);
915+
while (!sessions_.empty()) {
916+
(*sessions_.begin())->Delete();
917917
}
918-
sessions_.clear();
919918
}
920919

921920
DatabaseSync::~DatabaseSync() {
@@ -2118,13 +2117,22 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo<Value>& args) {
21182117
sqlite3_session* pSession;
21192118
int r = sqlite3session_create(db->connection_, db_name.c_str(), &pSession);
21202119
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
2121-
db->sessions_.insert(pSession);
2120+
bool wrapper_owns_session = false;
2121+
auto delete_session_on_failure = OnScopeLeave([&]() {
2122+
if (!wrapper_owns_session) {
2123+
sqlite3session_delete(pSession);
2124+
}
2125+
});
21222126

21232127
r = sqlite3session_attach(pSession, table == "" ? nullptr : table.c_str());
21242128
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
21252129

21262130
BaseObjectPtr<Session> session =
21272131
Session::Create(env, BaseObjectPtr<DatabaseSync>(db), pSession);
2132+
if (!session) {
2133+
return;
2134+
}
2135+
wrapper_owns_session = true;
21282136
args.GetReturnValue().Set(session->object());
21292137
}
21302138

@@ -3813,6 +3821,7 @@ Session::Session(Environment* env,
38133821
: BaseObject(env, object),
38143822
session_(session),
38153823
database_(std::move(database)) {
3824+
database_->sessions_.insert(this);
38163825
MakeWeak();
38173826
}
38183827

@@ -3905,10 +3914,12 @@ void Session::Dispose(const v8::FunctionCallbackInfo<v8::Value>& args) {
39053914
}
39063915

39073916
voidSession::Delete() {
3908-
if (!database_ || !database_->connection_ || session_ == nullptr) return;
3917+
if (session_ == nullptr) return;
39093918
sqlite3session_delete(session_);
3910-
database_->sessions_.erase(session_);
39113919
session_ = nullptr;
3920+
if (database_) {
3921+
database_->sessions_.erase(this);
3922+
}
39123923
}
39133924

39143925
voidDefineConstants(Local<Object> target) {

‎src/node_sqlite.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ class DatabaseSyncLimits;
133133
classStatementSyncIterator;
134134
classStatementSync;
135135
classBackupJob;
136+
classSession;
136137

137138
classStatementExecutionHelper {
138139
public:
@@ -243,7 +244,7 @@ class DatabaseSync : public BaseObject {
243244
bool ignore_next_sqlite_error_;
244245

245246
std::set<BackupJob*> backups_;
246-
std::set<sqlite3_session*> sessions_;
247+
std::unordered_set<Session*> sessions_;
247248
std::unordered_set<StatementSync*> statements_;
248249

249250
friendclassDatabaseSyncLimits;
@@ -360,6 +361,8 @@ class Session : public BaseObject {
360361
voidDelete();
361362
sqlite3_session* session_;
362363
BaseObjectPtr<DatabaseSync> database_; // The Parent Database
364+
365+
friendclassDatabaseSync;
363366
};
364367

365368
classSQLTagStore : publicBaseObject {

‎test/parallel/test-sqlite-session.js‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,22 @@ test('session.changeset() - closed database results in exception', (t) => {
8181
});
8282
});
8383

84+
test('session methods - reopened database results in exception',(t)=>{
85+
for(constmethodof['changeset','close']){
86+
constdatabase=newDatabaseSync(':memory:');
87+
constsession=database.createSession();
88+
database.close();
89+
database.open();
90+
91+
t.assert.throws(()=>{
92+
session[method]();
93+
},{
94+
name: 'Error',
95+
message: 'session is not open',
96+
});
97+
}
98+
});
99+
84100
test('database.applyChangeset() - closed database results in exception',(t)=>{
85101
constdatabase=newDatabaseSync(':memory:');
86102
constsession=database.createSession();

0 commit comments

Comments
 (0)