Skip to content

sqlite: fix use-after-free in StatementSync due to premature GC #56840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 5, 2025
Merged

sqlite: fix use-after-free in StatementSync due to premature GC #56840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions src/node_sqlite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,8 @@ void DatabaseSync::Prepare(const FunctionCallbackInfo<Value>& args) {
sqlite3_stmt* s = nullptr;
int r = sqlite3_prepare_v2(db->connection_, *sql, -1, &s, 0);
CHECK_ERROR_OR_THROW(env->isolate(), db, r, SQLITE_OK, void());
BaseObjectPtr<StatementSync> stmt = StatementSync::Create(env, db, s);
BaseObjectPtr<StatementSync> stmt =
StatementSync::Create(env, BaseObjectPtr<DatabaseSync>(db), s);
db->statements_.insert(stmt.get());
args.GetReturnValue().Set(stmt->object());
}
Expand Down Expand Up @@ -946,11 +947,10 @@ void DatabaseSync::LoadExtension(const FunctionCallbackInfo<Value>& args) {

StatementSync::StatementSync(Environment* env,
Local<Object> object,
DatabaseSync* db,
BaseObjectPtr<DatabaseSync> db,
sqlite3_stmt* stmt)
: BaseObject(env, object) {
: BaseObject(env, object), db_(std::move(db)) {
MakeWeak();
db_ = db;
statement_ = stmt;
// In the future, some of these options could be set at the database
// connection level and inherited by statements to reduce boilerplate.
Expand All @@ -977,7 +977,7 @@ inline bool StatementSync::IsFinalized() {

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

int anon_idx = 1;
int anon_start = 0;
Expand Down Expand Up @@ -1107,7 +1107,7 @@ bool StatementSync::BindValue(const Local<Value>& value, const int index) {
return false;
}

CHECK_ERROR_OR_THROW(env()->isolate(), db_, r, SQLITE_OK, false);
CHECK_ERROR_OR_THROW(env()->isolate(), db_.get(), r, SQLITE_OK, false);
return true;
}

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

if (!stmt->BindParams(args)) {
return;
Expand Down Expand Up @@ -1202,7 +1202,7 @@ void StatementSync::All(const FunctionCallbackInfo<Value>& args) {
rows.emplace_back(row);
}

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

Expand Down Expand Up @@ -1270,7 +1270,8 @@ void StatementSync::IterateNextCallback(

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

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

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

if (!stmt->BindParams(args)) {
return;
Expand All @@ -1396,7 +1397,7 @@ void StatementSync::Get(const FunctionCallbackInfo<Value>& args) {
r = sqlite3_step(stmt->statement_);
if (r == SQLITE_DONE) return;
if (r != SQLITE_ROW) {
THROW_ERR_SQLITE_ERROR(isolate, stmt->db_);
THROW_ERR_SQLITE_ERROR(isolate, stmt->db_.get());
return;
}

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

if (!stmt->BindParams(args)) {
return;
Expand All @@ -1441,7 +1442,7 @@ void StatementSync::Run(const FunctionCallbackInfo<Value>& args) {
auto reset = OnScopeLeave([&]() { sqlite3_reset(stmt->statement_); });
r = sqlite3_step(stmt->statement_);
if (r != SQLITE_ROW && r != SQLITE_DONE) {
THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_);
THROW_ERR_SQLITE_ERROR(env->isolate(), stmt->db_.get());
return;
}

Expand Down Expand Up @@ -1597,9 +1598,8 @@ Local<FunctionTemplate> StatementSync::GetConstructorTemplate(
return tmpl;
}

BaseObjectPtr<StatementSync> StatementSync::Create(Environment* env,
DatabaseSync* db,
sqlite3_stmt* stmt) {
BaseObjectPtr<StatementSync> StatementSync::Create(
Environment* env, BaseObjectPtr<DatabaseSync> db, sqlite3_stmt* stmt) {
Local<Object> obj;
if (!GetConstructorTemplate(env)
->InstanceTemplate()
Expand All @@ -1608,7 +1608,7 @@ BaseObjectPtr<StatementSync> StatementSync::Create(Environment* env,
return BaseObjectPtr<StatementSync>();
}

return MakeBaseObject<StatementSync>(env, obj, db, stmt);
return MakeBaseObject<StatementSync>(env, obj, std::move(db), stmt);
}

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

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

Expand Down
6 changes: 3 additions & 3 deletions src/node_sqlite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,13 @@ class StatementSync : public BaseObject {
public:
StatementSync(Environment* env,
v8::Local<v8::Object> object,
DatabaseSync* db,
BaseObjectPtr<DatabaseSync> db,
sqlite3_stmt* stmt);
void MemoryInfo(MemoryTracker* tracker) const override;
static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
Environment* env);
static BaseObjectPtr<StatementSync> Create(Environment* env,
DatabaseSync* db,
BaseObjectPtr<DatabaseSync> db,
sqlite3_stmt* stmt);
static void All(const v8::FunctionCallbackInfo<v8::Value>& args);
static void Iterate(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand All @@ -125,7 +125,7 @@ class StatementSync : public BaseObject {

private:
~StatementSync() override;
DatabaseSync* db_;
BaseObjectPtr<DatabaseSync> db_;
sqlite3_stmt* statement_;
bool use_big_ints_;
bool allow_bare_named_params_;
Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-sqlite-statement-sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,14 @@ suite('StatementSync.prototype.all()', () => {
suite('StatementSync.prototype.iterate()', () => {
test('executes a query and returns an empty iterator on no results', (t) => {
const db = new DatabaseSync(nextDb());
t.after(() => { db.close(); });
const stmt = db.prepare('CREATE TABLE storage(key TEXT, val TEXT)');
t.assert.deepStrictEqual(stmt.iterate().toArray(), []);
});

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