-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
fix: address static analysis warnings in SQLite and WebStorage #61380
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.
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
base: main
Are you sure you want to change the base?
fix: address static analysis warnings in SQLite and WebStorage #61380
Conversation
|
Review requested:
|
| 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>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks legit.
louwers
left a comment
There was a problem hiding this 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>(); \ |
There was a problem hiding this comment.
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) && |
There was a problem hiding this comment.
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.
|
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... |
|
@benjamingr Looks like the latter. |
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:
node_sqlite.cc- Null pointer dereference inSQLITE_VALUE_TO_JSmacro:sqlite3_value_text()before passing it toString::NewFromUtf8().Null(isolate)is returned instead of attempting to create a string from null data.node_sqlite.cc- Potential null dereference inTHROW_ERR_SQLITE_ERROR:Environment::GetCurrent(isolate)before using the environment object.node_webstorage.cc- Unchecked return value fromsqlite3_prepare_v2:CHECK_ERROR_OR_THROW()aftersqlite3_prepare_v2()call to validate the operation succeeded.Rationale:
Testing:
Notes: