Skip to content

Commit acd3fb5

Browse files
littledivyaduh95
littledivy
authored and
aduh95
committed
sqlite: fix use-after-free in StatementSync due to premature GC
PR-URL: #56840 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 5d0d0b3 commit acd3fb5

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
@@ -597,7 +597,8 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo<Value>& args) {
597597
sqlite3_stmt* s = nullptr;
598598
int r = sqlite3_prepare_v2(db->connection_, *sql, -1, &s, 0);
599599
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
600-
BaseObjectPtr<StatementSync> stmt = StatementSync::Create(env, db, s);
600+
BaseObjectPtr<StatementSync> stmt =
601+
StatementSync::Create(env, BaseObjectPtr<DatabaseSync>(db), s);
601602
db->statements_.insert(stmt.get());
602603
args.GetReturnValue().Set(stmt->object());
603604
}
@@ -1033,11 +1034,10 @@ void DatabaseSync::LoadExtension(const FunctionCallbackInfo<Value>& args) {
10331034

10341035
StatementSync::StatementSync(Environment* env,
10351036
Local<Object> object,
1036-
DatabaseSync* db,
1037+
BaseObjectPtr<DatabaseSync> db,
10371038
sqlite3_stmt* stmt)
1038-
: BaseObject(env, object) {
1039+
: BaseObject(env, object), db_(std::move(db)) {
10391040
MakeWeak();
1040-
db_ = db;
10411041
statement_ = stmt;
10421042
// In the future, some of these options could be set at the database
10431043
// connection level and inherited by statements to reduce boilerplate.
@@ -1065,7 +1065,7 @@ inline bool StatementSync::IsFinalized() {
10651065

10661066
bool StatementSync::BindParams(const FunctionCallbackInfo<Value>& args) {
10671067
int r = sqlite3_clear_bindings(statement_);
1068-
CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false);
1068+
CHECK_ERROR_OR_THROW(env()->isolate(), db_.get(), r, SQLITE_OK, false);
10691069

10701070
int anon_idx = 1;
10711071
int anon_start = 0;
@@ -1199,7 +1199,7 @@ bool StatementSync::BindValue(const Local<Value>& value, const int index) {
11991199
return false;
12001200
}
12011201

1202-
CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false);
1202+
CHECK_ERROR_OR_THROW(env()->isolate(), db_.get(), r, SQLITE_OK, false);
12031203
return true;
12041204
}
12051205

@@ -1265,7 +1265,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
12651265
env, stmt->IsFinalized(), "statement has been finalized");
12661266
Isolate* isolate = env->isolate();
12671267
int r = sqlite3_reset(stmt->statement_);
1268-
CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_OK, void());
1268+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
12691269

12701270
if (!stmt->BindParams(args)) {
12711271
return;
@@ -1298,7 +1298,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
12981298
rows.emplace_back(row);
12991299
}
13001300

1301-
CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_DONE, void());
1301+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_DONE, void());
13021302
args.GetReturnValue().Set(Array::New(isolate, rows.data(), rows.size()));
13031303
}
13041304

@@ -1383,7 +1383,8 @@ void StatementSync::IterateNextCallback(
13831383

13841384
int r = sqlite3_step(stmt->statement_);
13851385
if (r != SQLITE_ROW) {
1386-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_DONE, void());
1386+
CHECK_ERROR_OR_THROW(
1387+
env->isolate(), stmt->db_.get(), r, SQLITE_DONE, void());
13871388

13881389
// cleanup when no more rows to fetch
13891390
sqlite3_reset(stmt->statement_);
@@ -1439,7 +1440,7 @@ void StatementSync::Iterate(const FunctionCallbackInfo<Value>& args) {
14391440
auto isolate = env->isolate();
14401441
auto context = env->context();
14411442
int r = sqlite3_reset(stmt->statement_);
1442-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void());
1443+
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
14431444

14441445
if (!stmt->BindParams(args)) {
14451446
return;
@@ -1516,7 +1517,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
15161517
env, stmt->IsFinalized(), "statement has been finalized");
15171518
Isolate* isolate = env->isolate();
15181519
int r = sqlite3_reset(stmt->statement_);
1519-
CHECK_ERROR_OR_THROW(isolate, stmt->db_, r, SQLITE_OK, void());
1520+
CHECK_ERROR_OR_THROW(isolate, stmt->db_.get(), r, SQLITE_OK, void());
15201521

15211522
if (!stmt->BindParams(args)) {
15221523
return;
@@ -1526,7 +1527,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
15261527
r = sqlite3_step(stmt->statement_);
15271528
if (r == SQLITE_DONE) return;
15281529
if (r != SQLITE_ROW) {
1529-
THROW_ERR_SQLITE_ERROR(isolate, stmt->db_);
1530+
THROW_ERR_SQLITE_ERROR(isolate, stmt->db_.get());
15301531
return;
15311532
}
15321533

@@ -1562,7 +1563,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
15621563
THROW_AND_RETURN_ON_BAD_STATE(
15631564
env, stmt->IsFinalized(), "statement has been finalized");
15641565
int r = sqlite3_reset(stmt->statement_);
1565-
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_, r, SQLITE_OK, void());
1566+
CHECK_ERROR_OR_THROW(env->isolate(), stmt->db_.get(), r, SQLITE_OK, void());
15661567

15671568
if (!stmt->BindParams(args)) {
15681569
return;
@@ -1571,7 +1572,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
15711572
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
15721573
r = sqlite3_step(stmt->statement_);
15731574
if (r != SQLITE_ROW && r != SQLITE_DONE) {
1574-
THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_);
1575+
THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_.get());
15751576
return;
15761577
}
15771578

@@ -1748,9 +1749,8 @@ Local<FunctionTemplate> StatementSync::GetConstructorTemplate(
17481749
return tmpl;
17491750
}
17501751

1751-
BaseObjectPtr<StatementSync> StatementSync::Create(Environment* env,
1752-
DatabaseSync* db,
1753-
sqlite3_stmt* stmt) {
1752+
BaseObjectPtr<StatementSync> StatementSync::Create(
1753+
Environment* env, BaseObjectPtr<DatabaseSync> db, sqlite3_stmt* stmt) {
17541754
Local<Object> obj;
17551755
if (!GetConstructorTemplate(env)
17561756
->InstanceTemplate()
@@ -1759,7 +1759,7 @@ BaseObjectPtr<StatementSync> StatementSync::Create(Environment* env,
17591759
return BaseObjectPtr<StatementSync>();
17601760
}
17611761

1762-
return MakeBaseObject<StatementSync>(env, obj, db, stmt);
1762+
return MakeBaseObject<StatementSync>(env, obj, std::move(db), stmt);
17631763
}
17641764

17651765
Session::Session(Environment* env,
@@ -1826,7 +1826,7 @@ void Session::Changeset(const FunctionCallbackInfo<Value>& args) {
18261826
void* pChangeset;
18271827
int r = sqliteChangesetFunc(session->session_, &nChangeset, &pChangeset);
18281828
CHECK_ERROR_OR_THROW(
1829-
env->isolate(), session->database_, r, SQLITE_OK, void());
1829+
env->isolate(), session->database_.get(), r, SQLITE_OK, void());
18301830

18311831
auto freeChangeset = OnScopeLeave([&] { sqlite3_free(pChangeset); });
18321832

‎src/node_sqlite.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,13 +102,13 @@ class StatementSync : public BaseObject {
102102
public:
103103
StatementSync(Environment* env,
104104
v8::Local<v8::Object> object,
105-
DatabaseSync* db,
105+
BaseObjectPtr<DatabaseSync> db,
106106
sqlite3_stmt* stmt);
107107
void MemoryInfo(MemoryTracker* tracker) const override;
108108
static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
109109
Environment* env);
110110
static BaseObjectPtr<StatementSync> Create(Environment* env,
111-
DatabaseSync* db,
111+
BaseObjectPtr<DatabaseSync> db,
112112
sqlite3_stmt* stmt);
113113
static void All(const v8::FunctionCallbackInfo<v8::Value>& args);
114114
static void Iterate(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -130,7 +130,7 @@ class StatementSync : public BaseObject {
130130

131131
private:
132132
~StatementSync() override;
133-
DatabaseSync* db_;
133+
BaseObjectPtr<DatabaseSync> db_;
134134
sqlite3_stmt* statement_;
135135
bool use_big_ints_;
136136
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
@@ -86,12 +86,14 @@ suite('StatementSync.prototype.all()', () => {
8686
suite('StatementSync.prototype.iterate()', () => {
8787
test('executes a query and returns an empty iterator on no results', (t) => {
8888
const db = new DatabaseSync(nextDb());
89+
t.after(() => { db.close(); });
8990
const stmt = db.prepare('CREATE TABLE storage(key TEXT, val TEXT)');
9091
t.assert.deepStrictEqual(stmt.iterate().toArray(), []);
9192
});
9293

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

0 commit comments

Comments
 (0)