sqlite: validate connection after reading options

[lazerg]

function(), aggregate(), deserialize(), prepare(), applyChangeset(), backup() and the tag store check that the connection is open before reading their options bag. A getter on that object can call db.close(), leaving the rest of the call running against a null connection. Five of them segfault; prepare() and the tag store report ERR_SQLITE_ERROR: out of memory instead.

The open check now runs again once the options have been read, right before the connection reaches SQLite. Re-checking keeps the current error precedence, which parsing the options up front the way createSession() does would change.

Fixes: #65586

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (9baabd4) to head (3e733a7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	64.28%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65591 +/- ##
=======================================
Coverage 90.07% 90.08% =======================================
Files 751 751 Lines 254875 254895 +20 Branches 48108 48137 +29 =======================================
+ Hits 229579 229617 +38 + Misses 16466 16459 -7 + Partials 8830 8819 -11 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.05% <64.28%> (+0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[araujogui]

Maybe we can just check only once after arguments validation

[lazerg]

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

• test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
• test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

[lazerg]

#65595 landed the same fix for prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup(), and it goes further on the deserialize() buffer length and the length cast. Closing this in favour of it.

One site is still unguarded on main. SQLTagStore::PrepareStatement builds the SQL with strings->Get(context, i), so a getter on the template literal array can close the database. sqlite3_prepare_v3() then receives a null sqlite3* and returns SQLITE_MISUSE, and sqlite3_errmsg(nullptr) reports "out of memory" instead of ERR_INVALID_STATE:

constdb=newDatabaseSync(':memory:');constsql=db.createTagStore(10);conststrings=['SELECT ',''];Object.defineProperty(strings,0,{get(){db.close();return'SELECT ';}});sql.get(strings,1);// ERR_SQLITE_ERROR: out of memory

The guard needs to be written by hand there, because THROW_AND_RETURN_ON_BAD_STATE hardcodes return; and the function returns a BaseObjectPtr<StatementSync>. Happy to send a small follow-up PR if that is useful.

[lazerg]

function(), aggregate(), deserialize(), prepare(), applyChangeset(), backup() and the tag store check that the connection is open before reading their options bag. A getter on that object can call db.close(), leaving the rest of the call running against a null connection. Five of them segfault; prepare() and the tag store report ERR_SQLITE_ERROR: out of memory instead.

The open check now runs again once the options have been read, right before the connection reaches SQLite. Re-checking keeps the current error precedence, which parsing the options up front the way createSession() does would change.

Fixes: #65586

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (9baabd4) to head (3e733a7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	64.28%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65591 +/- ##
=======================================
Coverage 90.07% 90.08% =======================================
Files 751 751 Lines 254875 254895 +20 Branches 48108 48137 +29 =======================================
+ Hits 229579 229617 +38 + Misses 16466 16459 -7 + Partials 8830 8819 -11 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.05% <64.28%> (+0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[araujogui]

Maybe we can just check only once after arguments validation

[lazerg]

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

• test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
• test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

[lazerg]

#65595 landed the same fix for prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup(), and it goes further on the deserialize() buffer length and the length cast. Closing this in favour of it.

One site is still unguarded on main. SQLTagStore::PrepareStatement builds the SQL with strings->Get(context, i), so a getter on the template literal array can close the database. sqlite3_prepare_v3() then receives a null sqlite3* and returns SQLITE_MISUSE, and sqlite3_errmsg(nullptr) reports "out of memory" instead of ERR_INVALID_STATE:

constdb=newDatabaseSync(':memory:');constsql=db.createTagStore(10);conststrings=['SELECT ',''];Object.defineProperty(strings,0,{get(){db.close();return'SELECT ';}});sql.get(strings,1);// ERR_SQLITE_ERROR: out of memory

The guard needs to be written by hand there, because THROW_AND_RETURN_ON_BAD_STATE hardcodes return; and the function returns a BaseObjectPtr<StatementSync>. Happy to send a small follow-up PR if that is useful.

[lazerg]

function(), aggregate(), deserialize(), prepare(), applyChangeset(), backup() and the tag store check that the connection is open before reading their options bag. A getter on that object can call db.close(), leaving the rest of the call running against a null connection. Five of them segfault; prepare() and the tag store report ERR_SQLITE_ERROR: out of memory instead.

The open check now runs again once the options have been read, right before the connection reaches SQLite. Re-checking keeps the current error precedence, which parsing the options up front the way createSession() does would change.

Fixes: #65586

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (9baabd4) to head (3e733a7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	64.28%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65591 +/- ##
=======================================
Coverage 90.07% 90.08% =======================================
Files 751 751 Lines 254875 254895 +20 Branches 48108 48137 +29 =======================================
+ Hits 229579 229617 +38 + Misses 16466 16459 -7 + Partials 8830 8819 -11 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.05% <64.28%> (+0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[araujogui]

Maybe we can just check only once after arguments validation

[lazerg]

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

• test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
• test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

[lazerg]

#65595 landed the same fix for prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup(), and it goes further on the deserialize() buffer length and the length cast. Closing this in favour of it.

One site is still unguarded on main. SQLTagStore::PrepareStatement builds the SQL with strings->Get(context, i), so a getter on the template literal array can close the database. sqlite3_prepare_v3() then receives a null sqlite3* and returns SQLITE_MISUSE, and sqlite3_errmsg(nullptr) reports "out of memory" instead of ERR_INVALID_STATE:

constdb=newDatabaseSync(':memory:');constsql=db.createTagStore(10);conststrings=['SELECT ',''];Object.defineProperty(strings,0,{get(){db.close();return'SELECT ';}});sql.get(strings,1);// ERR_SQLITE_ERROR: out of memory

The guard needs to be written by hand there, because THROW_AND_RETURN_ON_BAD_STATE hardcodes return; and the function returns a BaseObjectPtr<StatementSync>. Happy to send a small follow-up PR if that is useful.

[lazerg]

function(), aggregate(), deserialize(), prepare(), applyChangeset(), backup() and the tag store check that the connection is open before reading their options bag. A getter on that object can call db.close(), leaving the rest of the call running against a null connection. Five of them segfault; prepare() and the tag store report ERR_SQLITE_ERROR: out of memory instead.

The open check now runs again once the options have been read, right before the connection reaches SQLite. Re-checking keeps the current error precedence, which parsing the options up front the way createSession() does would change.

Fixes: #65586

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (9baabd4) to head (3e733a7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	64.28%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65591 +/- ##
=======================================
Coverage 90.07% 90.08% =======================================
Files 751 751 Lines 254875 254895 +20 Branches 48108 48137 +29 =======================================
+ Hits 229579 229617 +38 + Misses 16466 16459 -7 + Partials 8830 8819 -11 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.05% <64.28%> (+0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[araujogui]

Maybe we can just check only once after arguments validation

[lazerg]

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

• test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
• test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

[lazerg]

#65595 landed the same fix for prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup(), and it goes further on the deserialize() buffer length and the length cast. Closing this in favour of it.

One site is still unguarded on main. SQLTagStore::PrepareStatement builds the SQL with strings->Get(context, i), so a getter on the template literal array can close the database. sqlite3_prepare_v3() then receives a null sqlite3* and returns SQLITE_MISUSE, and sqlite3_errmsg(nullptr) reports "out of memory" instead of ERR_INVALID_STATE:

constdb=newDatabaseSync(':memory:');constsql=db.createTagStore(10);conststrings=['SELECT ',''];Object.defineProperty(strings,0,{get(){db.close();return'SELECT ';}});sql.get(strings,1);// ERR_SQLITE_ERROR: out of memory

The guard needs to be written by hand there, because THROW_AND_RETURN_ON_BAD_STATE hardcodes return; and the function returns a BaseObjectPtr<StatementSync>. Happy to send a small follow-up PR if that is useful.

[lazerg]

function(), aggregate(), deserialize(), prepare(), applyChangeset(), backup() and the tag store check that the connection is open before reading their options bag. A getter on that object can call db.close(), leaving the rest of the call running against a null connection. Five of them segfault; prepare() and the tag store report ERR_SQLITE_ERROR: out of memory instead.

The open check now runs again once the options have been read, right before the connection reaches SQLite. Re-checking keeps the current error precedence, which parsing the options up front the way createSession() does would change.

Fixes: #65586

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (9baabd4) to head (3e733a7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	64.28%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65591 +/- ##
=======================================
Coverage 90.07% 90.08% =======================================
Files 751 751 Lines 254875 254895 +20 Branches 48108 48137 +29 =======================================
+ Hits 229579 229617 +38 + Misses 16466 16459 -7 + Partials 8830 8819 -11 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.05% <64.28%> (+0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[araujogui]

Maybe we can just check only once after arguments validation

[lazerg]

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

• test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
• test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

[lazerg]

#65595 landed the same fix for prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup(), and it goes further on the deserialize() buffer length and the length cast. Closing this in favour of it.

One site is still unguarded on main. SQLTagStore::PrepareStatement builds the SQL with strings->Get(context, i), so a getter on the template literal array can close the database. sqlite3_prepare_v3() then receives a null sqlite3* and returns SQLITE_MISUSE, and sqlite3_errmsg(nullptr) reports "out of memory" instead of ERR_INVALID_STATE:

constdb=newDatabaseSync(':memory:');constsql=db.createTagStore(10);conststrings=['SELECT ',''];Object.defineProperty(strings,0,{get(){db.close();return'SELECT ';}});sql.get(strings,1);// ERR_SQLITE_ERROR: out of memory

The guard needs to be written by hand there, because THROW_AND_RETURN_ON_BAD_STATE hardcodes return; and the function returns a BaseObjectPtr<StatementSync>. Happy to send a small follow-up PR if that is useful.

[lazerg]

function(), aggregate(), deserialize(), prepare(), applyChangeset(), backup() and the tag store check that the connection is open before reading their options bag. A getter on that object can call db.close(), leaving the rest of the call running against a null connection. Five of them segfault; prepare() and the tag store report ERR_SQLITE_ERROR: out of memory instead.

The open check now runs again once the options have been read, right before the connection reaches SQLite. Re-checking keeps the current error precedence, which parsing the options up front the way createSession() does would change.

Fixes: #65586

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (9baabd4) to head (3e733a7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	64.28%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65591 +/- ##
=======================================
Coverage 90.07% 90.08% =======================================
Files 751 751 Lines 254875 254895 +20 Branches 48108 48137 +29 =======================================
+ Hits 229579 229617 +38 + Misses 16466 16459 -7 + Partials 8830 8819 -11 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.05% <64.28%> (+0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[araujogui]

Maybe we can just check only once after arguments validation

[lazerg]

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

• test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
• test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

[lazerg]

#65595 landed the same fix for prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup(), and it goes further on the deserialize() buffer length and the length cast. Closing this in favour of it.

One site is still unguarded on main. SQLTagStore::PrepareStatement builds the SQL with strings->Get(context, i), so a getter on the template literal array can close the database. sqlite3_prepare_v3() then receives a null sqlite3* and returns SQLITE_MISUSE, and sqlite3_errmsg(nullptr) reports "out of memory" instead of ERR_INVALID_STATE:

constdb=newDatabaseSync(':memory:');constsql=db.createTagStore(10);conststrings=['SELECT ',''];Object.defineProperty(strings,0,{get(){db.close();return'SELECT ';}});sql.get(strings,1);// ERR_SQLITE_ERROR: out of memory

The guard needs to be written by hand there, because THROW_AND_RETURN_ON_BAD_STATE hardcodes return; and the function returns a BaseObjectPtr<StatementSync>. Happy to send a small follow-up PR if that is useful.

[lazerg]

function(), aggregate(), deserialize(), prepare(), applyChangeset(), backup() and the tag store check that the connection is open before reading their options bag. A getter on that object can call db.close(), leaving the rest of the call running against a null connection. Five of them segfault; prepare() and the tag store report ERR_SQLITE_ERROR: out of memory instead.

The open check now runs again once the options have been read, right before the connection reaches SQLite. Re-checking keeps the current error precedence, which parsing the options up front the way createSession() does would change.

Fixes: #65586

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (9baabd4) to head (3e733a7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	64.28%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65591 +/- ##
=======================================
Coverage 90.07% 90.08% =======================================
Files 751 751 Lines 254875 254895 +20 Branches 48108 48137 +29 =======================================
+ Hits 229579 229617 +38 + Misses 16466 16459 -7 + Partials 8830 8819 -11 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.05% <64.28%> (+0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[araujogui]

Maybe we can just check only once after arguments validation

[lazerg]

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

• test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
• test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

[lazerg]

#65595 landed the same fix for prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup(), and it goes further on the deserialize() buffer length and the length cast. Closing this in favour of it.

One site is still unguarded on main. SQLTagStore::PrepareStatement builds the SQL with strings->Get(context, i), so a getter on the template literal array can close the database. sqlite3_prepare_v3() then receives a null sqlite3* and returns SQLITE_MISUSE, and sqlite3_errmsg(nullptr) reports "out of memory" instead of ERR_INVALID_STATE:

constdb=newDatabaseSync(':memory:');constsql=db.createTagStore(10);conststrings=['SELECT ',''];Object.defineProperty(strings,0,{get(){db.close();return'SELECT ';}});sql.get(strings,1);// ERR_SQLITE_ERROR: out of memory

The guard needs to be written by hand there, because THROW_AND_RETURN_ON_BAD_STATE hardcodes return; and the function returns a BaseObjectPtr<StatementSync>. Happy to send a small follow-up PR if that is useful.

[lazerg]

function(), aggregate(), deserialize(), prepare(), applyChangeset(), backup() and the tag store check that the connection is open before reading their options bag. A getter on that object can call db.close(), leaving the rest of the call running against a null connection. Five of them segfault; prepare() and the tag store report ERR_SQLITE_ERROR: out of memory instead.

The open check now runs again once the options have been read, right before the connection reaches SQLite. Re-checking keeps the current error precedence, which parsing the options up front the way createSession() does would change.

Fixes: #65586

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[codecov]

Codecov Report

❌ Patch coverage is 64.28571% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (9baabd4) to head (3e733a7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	64.28%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #65591 +/- ##
=======================================
Coverage 90.07% 90.08% =======================================
Files 751 751 Lines 254875 254895 +20 Branches 48108 48137 +29 =======================================
+ Hits 229579 229617 +38 + Misses 16466 16459 -7 + Partials 8830 8819 -11 

Files with missing lines	Coverage Δ
src/node_sqlite.cc	82.05% <64.28%> (+0.01%)	⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[araujogui]

Maybe we can just check only once after arguments validation

[lazerg]

Thanks for the review. I kept the early check on purpose, and the fix that landed in #65595 made the same call.

Checking only after argument validation changes which error wins when the database is already closed. Two existing tests depend on the current order:

• test/parallel/test-sqlite-database-sync.js:379 calls db.prepare() with no arguments on a closed database and expects ERR_INVALID_STATE. Without the early check the missing sql argument throws ERR_INVALID_ARG_TYPE first.
• test/parallel/test-sqlite-serialize.js:182 calls db.deserialize(new Uint8Array(0)) on a closed database and expects ERR_INVALID_STATE. Without the early check it becomes ERR_INVALID_ARG_VALUE. I applied that change and rebuilt to confirm it.

The early check also rejects the call while an authorizer callback is running, so the options getters never run re-entrantly there.

Since #65595 covers the same six functions, I am closing this PR.

[lazerg]

#65595 landed the same fix for prepare(), function(), aggregate(), deserialize(), applyChangeset() and backup(), and it goes further on the deserialize() buffer length and the length cast. Closing this in favour of it.

One site is still unguarded on main. SQLTagStore::PrepareStatement builds the SQL with strings->Get(context, i), so a getter on the template literal array can close the database. sqlite3_prepare_v3() then receives a null sqlite3* and returns SQLITE_MISUSE, and sqlite3_errmsg(nullptr) reports "out of memory" instead of ERR_INVALID_STATE:

constdb=newDatabaseSync(':memory:');constsql=db.createTagStore(10);conststrings=['SELECT ',''];Object.defineProperty(strings,0,{get(){db.close();return'SELECT ';}});sql.get(strings,1);// ERR_SQLITE_ERROR: out of memory

The guard needs to be written by hand there, because THROW_AND_RETURN_ON_BAD_STATE hardcodes return; and the function returns a BaseObjectPtr<StatementSync>. Happy to send a small follow-up PR if that is useful.