From a5cc976254df818dc7dfbe23de00b386dbc1ed2f Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 2 Feb 2025 20:43:44 -0800 Subject: [PATCH] src: improve error handling in node_sqlite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/56891 Reviewed-By: Michaël Zasso Reviewed-By: Colin Ihrig --- src/node_sqlite.cc | 158 +++++++++++++++++++++++++++++---------------- 1 file changed, 104 insertions(+), 54 deletions(-) diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 1639c196ce7e33..dca5b67b80a181 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -126,13 +126,15 @@ inline void THROW_ERR_SQLITE_ERROR(Isolate* isolate, int errcode) { const char* errstr = sqlite3_errstr(errcode); Environment* env = Environment::GetCurrent(isolate); - auto error = CreateSQLiteError(isolate, errstr).ToLocalChecked(); - error - ->Set(isolate->GetCurrentContext(), - env->errcode_string(), - Integer::New(isolate, errcode)) - .ToChecked(); - isolate->ThrowException(error); + Local error; + if (CreateSQLiteError(isolate, errstr).ToLocal(&error) && + error + ->Set(isolate->GetCurrentContext(), + env->errcode_string(), + Integer::New(isolate, errcode)) + .IsJust()) { + isolate->ThrowException(error); + } } UserDefinedFunction::UserDefinedFunction(Environment* env, @@ -734,12 +736,14 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { } } - Local db_key = - String::NewFromUtf8(env->isolate(), "db", NewStringType::kNormal) - .ToLocalChecked(); + Local db_key = FIXED_ONE_BYTE_STRING(env->isolate(), "db"); + if (options->HasOwnProperty(env->context(), db_key).FromJust()) { - Local db_value = - options->Get(env->context(), db_key).ToLocalChecked(); + Local db_value; + if (!options->Get(env->context(), db_key).ToLocal(&db_value)) { + // An error will have been scheduled. + return; + } if (db_value->IsString()) { String::Utf8Value str(env->isolate(), db_value); db_name = std::string(*str); @@ -808,8 +812,12 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { } Local options = args[1].As(); - Local conflictValue = - options->Get(env->context(), env->onconflict_string()).ToLocalChecked(); + Local conflictValue; + if (!options->Get(env->context(), env->onconflict_string()) + .ToLocal(&conflictValue)) { + // An error will have been scheduled. + return; + } if (!conflictValue->IsUndefined()) { if (!conflictValue->IsFunction()) { @@ -837,8 +845,12 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { if (options->HasOwnProperty(env->context(), env->filter_string()) .FromJust()) { - Local filterValue = - options->Get(env->context(), env->filter_string()).ToLocalChecked(); + Local filterValue; + if (!options->Get(env->context(), env->filter_string()) + .ToLocal(&filterValue)) { + // An error will have been scheduled. + return; + } if (!filterValue->IsFunction()) { THROW_ERR_INVALID_ARG_TYPE( @@ -850,6 +862,10 @@ void DatabaseSync::ApplyChangeset(const FunctionCallbackInfo& args) { Local filterFunc = filterValue.As(); filterCallback = [env, filterFunc](std::string item) -> bool { + // TODO(@jasnell): The use of ToLocalChecked here means that if + // the filter function throws an error the process will crash. + // The filterCallback should be updated to avoid the check and + // propagate the error correctly. Local argv[] = {String::NewFromUtf8(env->isolate(), item.c_str(), NewStringType::kNormal) @@ -1214,11 +1230,18 @@ void StatementSync::IterateReturnCallback( auto self = args.This(); // iterator has fetch all result or break, prevent next func to return result - self->Set(context, env->isfinished_string(), Boolean::New(isolate, true)) - .ToChecked(); + if (self->Set(context, env->isfinished_string(), Boolean::New(isolate, true)) + .IsNothing()) { + // An error will have been scheduled. + return; + } - auto external_stmt = Local::Cast( - self->Get(context, env->statement_string()).ToLocalChecked()); + Local val; + if (!self->Get(context, env->statement_string()).ToLocal(&val)) { + // An error will have been scheduled. + return; + } + auto external_stmt = Local::Cast(val); auto stmt = static_cast(external_stmt->Value()); if (!stmt->IsFinalized()) { sqlite3_reset(stmt->statement_); @@ -1242,28 +1265,38 @@ void StatementSync::IterateNextCallback( auto self = args.This(); + Local val; + if (!self->Get(context, env->isfinished_string()).ToLocal(&val)) { + // An error will have been scheduled. + return; + } + // skip iteration if is_finished - auto is_finished = Local::Cast( - self->Get(context, env->isfinished_string()).ToLocalChecked()); + auto is_finished = Local::Cast(val); if (is_finished->Value()) { - LocalVector keys(isolate, {env->done_string(), env->value_string()}); - LocalVector values(isolate, - {Boolean::New(isolate, true), Null(isolate)}); - - DCHECK_EQ(keys.size(), values.size()); + Local keys[] = {env->done_string(), env->value_string()}; + Local values[] = {Boolean::New(isolate, true), Null(isolate)}; + static_assert(arraysize(keys) == arraysize(values)); Local result = Object::New( - isolate, Null(isolate), keys.data(), values.data(), keys.size()); + isolate, Null(isolate), &keys[0], &values[0], arraysize(keys)); args.GetReturnValue().Set(result); return; } - auto external_stmt = Local::Cast( - self->Get(context, env->statement_string()).ToLocalChecked()); + if (!self->Get(context, env->statement_string()).ToLocal(&val)) { + // An error will have been scheduled. + return; + } + + auto external_stmt = Local::Cast(val); auto stmt = static_cast(external_stmt->Value()); - auto num_cols = - Local::Cast( - self->Get(context, env->num_cols_string()).ToLocalChecked()) - ->Value(); + + if (!self->Get(context, env->num_cols_string()).ToLocal(&val)) { + // An error will have been scheduled. + return; + } + + auto num_cols = Local::Cast(val)->Value(); THROW_AND_RETURN_ON_BAD_STATE( env, stmt->IsFinalized(), "statement has been finalized"); @@ -1275,8 +1308,12 @@ void StatementSync::IterateNextCallback( // cleanup when no more rows to fetch sqlite3_reset(stmt->statement_); - self->Set(context, env->isfinished_string(), Boolean::New(isolate, true)) - .ToChecked(); + if (self->Set( + context, env->isfinished_string(), Boolean::New(isolate, true)) + .IsNothing()) { + // An error would have been scheduled + return; + } LocalVector keys(isolate, {env->done_string(), env->value_string()}); LocalVector values(isolate, @@ -1329,15 +1366,19 @@ void StatementSync::Iterate(const FunctionCallbackInfo& args) { return; } - Local next_func = - Function::New(context, StatementSync::IterateNextCallback) - .ToLocalChecked(); - Local return_func = - Function::New(context, StatementSync::IterateReturnCallback) - .ToLocalChecked(); + Local next_func; + Local return_func; + if (!Function::New(context, StatementSync::IterateNextCallback) + .ToLocal(&next_func) || + !Function::New(context, StatementSync::IterateReturnCallback) + .ToLocal(&return_func)) { + // An error will have been scheduled. + return; + } - LocalVector keys(isolate, {env->next_string(), env->return_string()}); - LocalVector values(isolate, {next_func, return_func}); + Local keys[] = {env->next_string(), env->return_string()}; + Local values[] = {next_func, return_func}; + static_assert(arraysize(keys) == arraysize(values)); Local global = context->Global(); Local js_iterator; @@ -1349,32 +1390,41 @@ void StatementSync::Iterate(const FunctionCallbackInfo& args) { .ToLocal(&js_iterator_prototype)) return; - DCHECK_EQ(keys.size(), values.size()); Local iterable_iterator = Object::New( - isolate, js_iterator_prototype, keys.data(), values.data(), keys.size()); + isolate, js_iterator_prototype, &keys[0], &values[0], arraysize(keys)); auto num_cols_pd = v8::PropertyDescriptor( v8::Integer::New(isolate, sqlite3_column_count(stmt->statement_)), false); num_cols_pd.set_enumerable(false); num_cols_pd.set_configurable(false); - iterable_iterator - ->DefineProperty(context, env->num_cols_string(), num_cols_pd) - .ToChecked(); + if (iterable_iterator + ->DefineProperty(context, env->num_cols_string(), num_cols_pd) + .IsNothing()) { + // An error will have been scheduled. + return; + } auto stmt_pd = v8::PropertyDescriptor(v8::External::New(isolate, stmt), false); stmt_pd.set_enumerable(false); stmt_pd.set_configurable(false); - iterable_iterator->DefineProperty(context, env->statement_string(), stmt_pd) - .ToChecked(); + if (iterable_iterator + ->DefineProperty(context, env->statement_string(), stmt_pd) + .IsNothing()) { + // An error will have been scheduled. + return; + } auto is_finished_pd = v8::PropertyDescriptor(v8::Boolean::New(isolate, false), true); stmt_pd.set_enumerable(false); stmt_pd.set_configurable(false); - iterable_iterator - ->DefineProperty(context, env->isfinished_string(), is_finished_pd) - .ToChecked(); + if (iterable_iterator + ->DefineProperty(context, env->isfinished_string(), is_finished_pd) + .IsNothing()) { + // An error will have been scheduled. + return; + } args.GetReturnValue().Set(iterable_iterator); }