Skip to content

Conversation

@GoldFish2500
Copy link

Fix static analysis warnings in SQLite and Web Storage code

This PR addresses several issues reported by static analysis tools to improve code safety and reliability in both SQLite and Web Storage modules.

Changes:

  1. node_sqlite.cc - Null pointer dereference in SQLITE_VALUE_TO_JS macro:

    • Added null check for the pointer returned by sqlite3_value_text() before passing it to String::NewFromUtf8().
    • When the pointer is null, Null(isolate) is returned instead of attempting to create a string from null data.
  2. node_sqlite.cc - Potential null dereference in THROW_ERR_SQLITE_ERROR:

    • Added null check for Environment::GetCurrent(isolate) before using the environment object.
    • This prevents potential crashes when the environment cannot be retrieved.
  3. node_webstorage.cc - Unchecked return value from sqlite3_prepare_v2:

    • Added CHECK_ERROR_OR_THROW() after sqlite3_prepare_v2() call to validate the operation succeeded.
    • This ensures the prepared statement pointer is valid before further use in the Web Storage initialization.

Rationale:

  • These fixes prevent potential null pointer dereferences that could lead to crashes.
  • The changes align with existing error handling patterns in the codebase.
  • Static analysis tools (like Coverity, Clang analyzer) consistently flag these patterns as high-risk issues.

Testing:

  • Existing tests should continue to pass as these are defensive programming changes.
  • No behavioral changes are expected for normal execution paths.

Notes:

  • All changes are minimal and focused only on fixing the identified issues.
  • Code maintains backward compatibility and follows existing error handling conventions.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. labels Jan 14, 2026
sqlite3_stmt* s = nullptr;
r = sqlite3_prepare_v2(
db, get_schema_version_sql.data(), get_schema_version_sql.size(), &s, 0);
CHECK_ERROR_OR_THROW(env(), r, SQLITE_OK, Nothing<void>());
Copy link
Contributor

@louwers louwers Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks legit.

Copy link
Contributor

@louwers louwers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

case SQLITE_TEXT: { \
const char* v = \
reinterpret_cast<const char*>(sqlite3_##from##_text(__VA_ARGS__)); \
(result) = String::NewFromUtf8((isolate), v).As<Value>(); \
Copy link
Contributor

@louwers louwers Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings returned by sqlite3_column_text() and sqlite3_column_text16(), even empty strings, are always zero-terminated.

If an out-of-memory error occurs, then the return value from these routines is the same as if the column had contained an SQL NULL value. Valid SQL NULL returns can be distinguished from out-of-memory errors by invoking the sqlite3_errcode() immediately after the suspect return value is obtained and before any other SQLite interface is called on the same database connection.

https://sqlite.org/c3ref/column_blob.html

If an OOM error occurs we should not just silently return null.

Environment* env = Environment::GetCurrent(isolate);
Local<Object> error;
if (CreateSQLiteError(isolate, errstr).ToLocal(&error) &&
if (env && CreateSQLiteError(isolate, errstr).ToLocal(&error) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a problem because we assume Environment::GetCurrent(isolate) does not return null throughout the module.

@benjamingr
Copy link
Member

Did you write and do you understand this code or did you ask an AI coding tool to fix it based on the warnings since a bunch of these changes don't look correct...

@louwers
Copy link
Contributor

louwers commented Jan 14, 2026

@benjamingr Looks like the latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants