fix: address static analysis warnings in SQLite and WebStorage#61380
fix: address static analysis warnings in SQLite and WebStorage#61380GoldFish2500 wants to merge 4 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
| 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>()); |
| 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.
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.
There was a problem hiding this comment.
agree that hiding OOM is not acceptable. I suggest throwing an appropriate exception
src/node_sqlite.cc
Outdated
| 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.
I don't think this is a problem because we assume Environment::GetCurrent(isolate) does not return null throughout the module.
There was a problem hiding this comment.
Ok. I removed the unnecessary check.
|
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. |
| sqlite3_##from##_blob(__VA_ARGS__)); \ | ||
| auto store = ArrayBuffer::NewBackingStore( \ | ||
| (isolate), size, BackingStoreInitializationMode::kUninitialized); \ | ||
| memcpy(store->Data(), data, size); \ |
There was a problem hiding this comment.
I suggest adding the pointer check to prevent UB. The behavior will not change, but the static analyzer will stop triggering.
4 participants
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: