Skip to content

Commit 8410c95

Browse files
littledivytargos
littledivy
authored and
targos
committed
sqlite: fix use-after-free in StatementSync due to premature GC
PR-URL: #56840 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent f0bf35d commit 8410c95

File tree

3 files changed

+24
-22
lines changed

3 files changed

+24
-22
lines changed

‎src/node_sqlite.cc‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -535,7 +535,8 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo<Value>& args) {
535535
sqlite3_stmt* s = nullptr;
536536
int r = sqlite3_prepare_v2(db->connection_, *sql, -1, &s, 0);
537537
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
538-
BaseObjectPtr<StatementSync> stmt = StatementSync::Create(env, db, s);
538+
BaseObjectPtr<StatementSync> stmt =
539+
StatementSync::Create(env, BaseObjectPtr<DatabaseSync>(db), s);
539540
db->statements_.insert(stmt.get());
540541
args.GetReturnValue().Set(stmt->object());
541542
}
@@ -946,11 +947,10 @@ void DatabaseSync::LoadExtension(const FunctionCallbackInfo<Value>& args) {
946947

947948
StatementSync::StatementSync(Environment* env,
948949
Local<Object> object,
949-
DatabaseSync* db,
950+
BaseObjectPtr<DatabaseSync> db,
950951
sqlite3_stmt* stmt)
951-
: BaseObject(env, object) {
952+
: BaseObject(env, object), db_(std::move(db)) {
952953
MakeWeak();
953-
db_ = db;
954954
statement_ = stmt;
955955
// In the future, some of these options could be set at the database
956956
// connection level and inherited by statements to reduce boilerplate.
@@ -977,7 +977,7 @@ inline bool StatementSync::IsFinalized() {
977977

978978
bool StatementSync::BindParams(const FunctionCallbackInfo<Value>& args) {
979979
int r = sqlite3_clear_bindings(statement_);
980-
CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false);
980+
CHECK_ERROR_OR_THROW(env()->isolate(), db_.get(), r, SQLITE_OK, false);
981981

982982
int anon_idx = 1;
983983
int anon_start = 0;
@@ -1107,7 +1107,7 @@ bool StatementSync::BindValue(const Local<Value>& value, const int index) {
11071107
return false;
11081108
}
11091109

1110-
CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false);
1110+
CHECK_ERROR_OR_THROW(env()->isolate(), db_.get(), r, SQLITE_OK, false);
11111111
return true;
11121112
}
11131113

@@ -1173,7 +1173,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
11731173
env, stmt->IsFinalized(), "statement has been finalized");
11741174
Isolate* isolate = env->isolate();
11751175
int r = sqlite3_reset(stmt->statement_);
1176-
CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_OK, void());
1176+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
11771177

11781178
if (!stmt->BindParams(args)) {
11791179
return;
@@ -1202,7 +1202,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
12021202
rows.emplace_back(row);
12031203
}
12041204

1205-
CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_DONE, void());
1205+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_DONE, void());
12061206
args.GetReturnValue().Set(Array::New(isolate, rows.data(), rows.size()));
12071207
}
12081208

@@ -1270,7 +1270,8 @@ void StatementSync::IterateNextCallback(
12701270

12711271
int r = sqlite3_step(stmt->statement_);
12721272
if (r != SQLITE_ROW) {
1273-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_DONE, void());
1273+
CHECK_ERROR_OR_THROW(
1274+
env->isolate(), stmt->db_.get(), r, SQLITE_DONE, void());
12741275

12751276
// cleanup when no more rows to fetch
12761277
sqlite3_reset(stmt->statement_);
@@ -1322,7 +1323,7 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
13221323
auto isolate = env->isolate();
13231324
auto context = env->context();
13241325
int r = sqlite3_reset(stmt->statement_);
1325-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void());
1326+
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
13261327

13271328
if (!stmt->BindParams(args)) {
13281329
return;
@@ -1386,7 +1387,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
13861387
env, stmt->IsFinalized(), "statement has been finalized");
13871388
Isolate* isolate = env->isolate();
13881389
int r = sqlite3_reset(stmt->statement_);
1389-
CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_OK, void());
1390+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
13901391

13911392
if (!stmt->BindParams(args)) {
13921393
return;
@@ -1396,7 +1397,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
13961397
r = sqlite3_step(stmt->statement_);
13971398
if (r == SQLITE_DONE) return;
13981399
if (r != SQLITE_ROW) {
1399-
THROW_ERR_SQLITE_ERROR(isolate, stmt->db_);
1400+
THROW_ERR_SQLITE_ERROR(isolate, stmt->db_.get());
14001401
return;
14011402
}
14021403

@@ -1432,7 +1433,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
14321433
THROW_AND_RETURN_ON_BAD_STATE(
14331434
env, stmt->IsFinalized(), "statement has been finalized");
14341435
int r = sqlite3_reset(stmt->statement_);
1435-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void());
1436+
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
14361437

14371438
if (!stmt->BindParams(args)) {
14381439
return;
@@ -1441,7 +1442,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
14411442
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
14421443
r = sqlite3_step(stmt->statement_);
14431444
if (r != SQLITE_ROW && r != SQLITE_DONE) {
1444-
THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_);
1445+
THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_.get());
14451446
return;
14461447
}
14471448

@@ -1597,9 +1598,8 @@ Local<FunctionTemplate> StatementSync::GetConstructorTemplate(
15971598
return tmpl;
15981599
}
15991600

1600-
BaseObjectPtr<StatementSync> StatementSync::Create(Environment* env,
1601-
DatabaseSync* db,
1602-
sqlite3_stmt* stmt) {
1601+
BaseObjectPtr<StatementSync> StatementSync::Create(
1602+
Environment* env, BaseObjectPtr<DatabaseSync> db, sqlite3_stmt* stmt) {
16031603
Local<Object> obj;
16041604
if (!GetConstructorTemplate(env)
16051605
->InstanceTemplate()
@@ -1608,7 +1608,7 @@ BaseObjectPtr<StatementSync> StatementSync::Create(Environment* env,
16081608
return BaseObjectPtr<StatementSync>();
16091609
}
16101610

1611-
return MakeBaseObject<StatementSync>(env, obj, db, stmt);
1611+
return MakeBaseObject<StatementSync>(env, obj, std::move(db), stmt);
16121612
}
16131613

16141614
Session::Session(Environment* env,
@@ -1675,7 +1675,7 @@ void Session::Changeset(const FunctionCallbackInfo<Value>& args) {
16751675
void* pChangeset;
16761676
int r = sqliteChangesetFunc(session->session_, &nChangeset, &pChangeset);
16771677
CHECK_ERROR_OR_THROW(
1678-
env->isolate(), session->database_, r, SQLITE_OK, void());
1678+
env->isolate(), session->database_.get(), r, SQLITE_OK, void());
16791679

16801680
auto freeChangeset = OnScopeLeave([&] { sqlite3_free(pChangeset); });
16811681

‎src/node_sqlite.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,13 @@ class StatementSync : public BaseObject {
9999
public:
100100
StatementSync(Environment* env,
101101
v8::Local<v8::Object> object,
102-
DatabaseSync* db,
102+
BaseObjectPtr<DatabaseSync> db,
103103
sqlite3_stmt* stmt);
104104
void MemoryInfo(MemoryTracker* tracker) const override;
105105
static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
106106
Environment* env);
107107
static BaseObjectPtr<StatementSync> Create(Environment* env,
108-
DatabaseSync* db,
108+
BaseObjectPtr<DatabaseSync> db,
109109
sqlite3_stmt* stmt);
110110
static void All(const v8::FunctionCallbackInfo<v8::Value>& args);
111111
static void Iterate(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -125,7 +125,7 @@ class StatementSync : public BaseObject {
125125

126126
private:
127127
~StatementSync() override;
128-
DatabaseSync* db_;
128+
BaseObjectPtr<DatabaseSync> db_;
129129
sqlite3_stmt* statement_;
130130
bool use_big_ints_;
131131
bool allow_bare_named_params_;

‎test/parallel/test-sqlite-statement-sync.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,12 +87,14 @@ suite('StatementSync.prototype.all()', () => {
8787
suite('StatementSync.prototype.iterate()', () => {
8888
test('executes a query and returns an empty iterator on no results', (t) => {
8989
const db = new DatabaseSync(nextDb());
90+
t.after(() => { db.close(); });
9091
const stmt = db.prepare('CREATE TABLE storage(key TEXT, val TEXT)');
9192
t.assert.deepStrictEqual(stmt.iterate().toArray(), []);
9293
});
9394

9495
test('executes a query and returns all results', (t) => {
9596
const db = new DatabaseSync(nextDb());
97+
t.after(() => { db.close(); });
9698
let stmt = db.prepare('CREATE TABLE storage(key TEXT, val TEXT)');
9799
t.assert.deepStrictEqual(stmt.run(), { changes: 0, lastInsertRowid: 0 });
98100
stmt = db.prepare('INSERT INTO storage (key, val) VALUES (?, ?)');

0 commit comments

Comments
 (0)