From 3f25b66f4c173dd777bf9b6af44ee0104e2df3a5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 13 Oct 2025 15:42:40 +0200 Subject: [PATCH 1/5] src: use `Utf8Value` and `TwoByteValue` instead of V8 helpers Our own helper classes have the advantage of using stack storage a lot of the time, so they should always be preferred. --- src/api/async_resource.cc | 6 ++++++ src/api/hooks.cc | 13 ++++++++++--- src/inspector_js_api.cc | 4 ++-- src/node.h | 8 ++++++++ src/node_api.cc | 4 ++-- src/node_buffer.cc | 21 +++++++++------------ src/node_errors.cc | 12 ++++-------- src/node_report.cc | 3 +-- src/node_sqlite.cc | 6 ++---- src/node_v8.cc | 4 ++-- src/permission/permission.cc | 14 +++++--------- src/string_bytes.cc | 12 ++++++------ 12 files changed, 57 insertions(+), 50 deletions(-) diff --git a/src/api/async_resource.cc b/src/api/async_resource.cc index 38d53a9aec0e58..6865eba65300bd 100644 --- a/src/api/async_resource.cc +++ b/src/api/async_resource.cc @@ -16,6 +16,12 @@ AsyncResource::AsyncResource(Isolate* isolate, Local resource, const char* name, async_id trigger_async_id) + : AsyncResource(isolate, resource, std::string_view(name), trigger_async_id) {} + +AsyncResource::AsyncResource(Isolate* isolate, + Local resource, + std::string_view name, + async_id trigger_async_id) : env_(Environment::GetCurrent(isolate)), resource_(isolate, resource), context_frame_(isolate, async_context_frame::current(isolate)) { diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 7b73f7db2427d9..a89d6a9795da3a 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -201,10 +201,17 @@ async_context EmitAsyncInit(Isolate* isolate, Local resource, const char* name, async_id trigger_async_id) { + return EmitAsyncInit(isolate, resource, std::string_view(name), trigger_async_id); +} + +async_context EmitAsyncInit(Isolate* isolate, + Local resource, + std::string_view name, + async_id trigger_async_id) { HandleScope handle_scope(isolate); - Local type = - String::NewFromUtf8(isolate, name, NewStringType::kInternalized) - .ToLocalChecked(); + Local type = ToV8Value(isolate->GetCurrentContext(), name, isolate) + .ToLocalChecked() + .As(); return EmitAsyncInit(isolate, resource, type, trigger_async_id); } diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index cbad4651f1db21..54a91765ac6b1f 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -240,8 +240,8 @@ static void AsyncTaskScheduledWrapper(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); Local task_name = args[0].As(); - String::Value task_name_value(args.GetIsolate(), task_name); - StringView task_name_view(*task_name_value, task_name_value.length()); + TwoByteValue task_name_value(args.GetIsolate(), task_name); + StringView task_name_view(task_name_value.out(), task_name_value.length()); CHECK(args[1]->IsNumber()); int64_t task_id; diff --git a/src/node.h b/src/node.h index 94c307e3f84923..70b60e79ac2969 100644 --- a/src/node.h +++ b/src/node.h @@ -1402,6 +1402,10 @@ NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate, v8::Local resource, const char* name, async_id trigger_async_id = -1); +NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate, + v8::Local resource, + std::string_view name, + async_id trigger_async_id = -1); NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate, v8::Local resource, @@ -1508,6 +1512,10 @@ class NODE_EXTERN AsyncResource { v8::Local resource, const char* name, async_id trigger_async_id = -1); + AsyncResource(v8::Isolate* isolate, + v8::Local resource, + std::string_view name, + async_id trigger_async_id = -1); virtual ~AsyncResource(); diff --git a/src/node_api.cc b/src/node_api.cc index 7274030f9491ef..a72ba0ddc96816 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -212,7 +212,7 @@ class ThreadSafeFunction : public node::AsyncResource { napi_threadsafe_function_call_js call_js_cb_) : AsyncResource(env_->isolate, resource, - *v8::String::Utf8Value(env_->isolate, name)), + node::Utf8Value(env_->isolate, name).ToStringView()), thread_count(thread_count_), is_closing(false), dispatch_state(kDispatchIdle), @@ -1150,7 +1150,7 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork { : AsyncResource( env->isolate, async_resource, - *v8::String::Utf8Value(env->isolate, async_resource_name)), + node::Utf8Value(env->isolate, async_resource_name).ToStringView()), ThreadPoolWork(env->node_env(), "node_api"), _env(env), _data(data), diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 0e4d437c1ea501..8b896f4c1007b5 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -986,11 +986,7 @@ void IndexOfString(const FunctionCallbackInfo& args) { size_t result = haystack_length; if (enc == UCS2) { - String::Value needle_value(isolate, needle); - if (*needle_value == nullptr) { - return args.GetReturnValue().Set(-1); - } - + TwoByteValue needle_value(isolate, needle); if (haystack_length < 2 || needle_value.length() < 1) { return args.GetReturnValue().Set(-1); } @@ -1014,21 +1010,22 @@ void IndexOfString(const FunctionCallbackInfo& args) { result = nbytes::SearchString(reinterpret_cast(haystack), haystack_length / 2, - reinterpret_cast(*needle_value), + needle_value.out(), needle_value.length(), offset / 2, is_forward); } result *= 2; } else if (enc == UTF8) { - String::Utf8Value needle_value(isolate, needle); + Utf8Value needle_value(isolate, needle); if (*needle_value == nullptr) return args.GetReturnValue().Set(-1); + CHECK_GE(needle_length, needle_value.length()); result = nbytes::SearchString(reinterpret_cast(haystack), haystack_length, - reinterpret_cast(*needle_value), + reinterpret_cast(needle_value.out()), needle_length, offset, is_forward); @@ -1316,10 +1313,10 @@ static void Btoa(const FunctionCallbackInfo& args) { input->Length(), buffer.out()); } else { - String::Value value(env->isolate(), input); + TwoByteValue value(env->isolate(), input); MaybeStackBuffer stack_buf(value.length()); size_t out_len = simdutf::convert_utf16_to_latin1( - reinterpret_cast(*value), + reinterpret_cast(value.out()), value.length(), stack_buf.out()); if (out_len == 0) { // error @@ -1370,8 +1367,8 @@ static void Atob(const FunctionCallbackInfo& args) { buffer.SetLength(expected_length); result = simdutf::base64_to_binary(data, input->Length(), buffer.out()); } else { // 16-bit case - String::Value value(env->isolate(), input); - auto data = reinterpret_cast(*value); + TwoByteValue value(env->isolate(), input); + auto data = reinterpret_cast(value.out()); size_t expected_length = simdutf::maximal_binary_length_from_base64(data, value.length()); buffer.AllocateSufficientStorage(expected_length); diff --git a/src/node_errors.cc b/src/node_errors.cc index 7ad099a11b98aa..4ae66c14a73023 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -1024,15 +1024,11 @@ void PerIsolateMessageListener(Local message, Local error) { break; } Utf8Value filename(isolate, message->GetScriptOrigin().ResourceName()); + Utf8Value msg(isolate, message->Get()); // (filename):(line) (message) - std::stringstream warning; - warning << *filename; - warning << ":"; - warning << message->GetLineNumber(env->context()).FromMaybe(-1); - warning << " "; - v8::String::Utf8Value msg(isolate, message->Get()); - warning << *msg; - USE(ProcessEmitWarningGeneric(env, warning.str().c_str(), "V8")); + std::string warning = SPrintF("%s:%s %s", + filename, message->GetLineNumber(env->context()).FromMaybe(-1), msg); + USE(ProcessEmitWarningGeneric(env, warning, "V8")); break; } case Isolate::MessageErrorLevel::kMessageError: diff --git a/src/node_report.cc b/src/node_report.cc index 0bbf829b45fb61..e4cb681fd76890 100644 --- a/src/node_report.cc +++ b/src/node_report.cc @@ -455,8 +455,7 @@ static Maybe ErrorToString(Isolate* isolate, if (!maybe_str.ToLocal(&js_str)) { return Nothing(); } - String::Utf8Value sv(isolate, js_str); - return Just<>(std::string(*sv, sv.length())); + return Just(Utf8Value(isolate, js_str).ToString()); } static void PrintEmptyJavaScriptStack(JSONWriter* writer) { diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index cf9ce6686cffca..1cab6dde9df945 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -1508,8 +1508,7 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { } if (table_value->IsString()) { - String::Utf8Value str(env->isolate(), table_value); - table = *str; + table = Utf8Value(env->isolate(), table_value).ToString(); } else { THROW_ERR_INVALID_ARG_TYPE( env->isolate(), "The \"options.table\" argument must be a string."); @@ -1529,8 +1528,7 @@ void DatabaseSync::CreateSession(const FunctionCallbackInfo& args) { return; } if (db_value->IsString()) { - String::Utf8Value str(env->isolate(), db_value); - db_name = std::string(*str); + db_name = Utf8Value(env->isolate(), db_value).ToString(); } else { THROW_ERR_INVALID_ARG_TYPE( env->isolate(), "The \"options.db\" argument must be a string."); diff --git a/src/node_v8.cc b/src/node_v8.cc index ce253d7fa0a8b1..935ea2cf5157c3 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -242,8 +242,8 @@ void UpdateHeapCodeStatisticsBuffer(const FunctionCallbackInfo& args) { void SetFlagsFromString(const FunctionCallbackInfo& args) { CHECK(args[0]->IsString()); - String::Utf8Value flags(args.GetIsolate(), args[0]); - V8::SetFlagsFromString(*flags, static_cast(flags.length())); + Utf8Value flags(args.GetIsolate(), args[0]); + V8::SetFlagsFromString(flags.out(), flags.length()); } void StartCpuProfile(const FunctionCallbackInfo& args) { diff --git a/src/permission/permission.cc b/src/permission/permission.cc index 199b36ee76b34c..0fd13fd9143f03 100644 --- a/src/permission/permission.cc +++ b/src/permission/permission.cc @@ -34,24 +34,20 @@ static void Has(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK(args[0]->IsString()); - String::Utf8Value utf8_deny_scope(env->isolate(), args[0]); - if (*utf8_deny_scope == nullptr) { - return; - } - - const std::string deny_scope = *utf8_deny_scope; + const std::string deny_scope = Utf8Value(env->isolate(), args[0]).ToString(); PermissionScope scope = Permission::StringToPermission(deny_scope); if (scope == PermissionScope::kPermissionsRoot) { return args.GetReturnValue().Set(false); } if (args.Length() > 1 && !args[1]->IsUndefined()) { - String::Utf8Value utf8_arg(env->isolate(), args[1]); - if (*utf8_arg == nullptr) { + Utf8Value utf8_arg(env->isolate(), args[1]); + if (utf8_arg.length() == 0) { + args.GetReturnValue().Set(false); return; } return args.GetReturnValue().Set( - env->permission()->is_granted(env, scope, *utf8_arg)); + env->permission()->is_granted(env, scope, utf8_arg.ToStringView())); } return args.GetReturnValue().Set(env->permission()->is_granted(env, scope)); diff --git a/src/string_bytes.cc b/src/string_bytes.cc index d78bbb237325d2..03b5fd7ebe3816 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -304,10 +304,10 @@ size_t StringBytes::Write(Isolate* isolate, input_view.length()); } } else { - String::Value value(isolate, str); + TwoByteValue value(isolate, str); size_t written_len = buflen; auto result = simdutf::base64_to_binary_safe( - reinterpret_cast(*value), + reinterpret_cast(value.out()), value.length(), buf, written_len, @@ -343,10 +343,10 @@ size_t StringBytes::Write(Isolate* isolate, input_view.length()); } } else { - String::Value value(isolate, str); + TwoByteValue value(isolate, str); size_t written_len = buflen; auto result = simdutf::base64_to_binary_safe( - reinterpret_cast(*value), + reinterpret_cast(value.out()), value.length(), buf, written_len); @@ -368,8 +368,8 @@ size_t StringBytes::Write(Isolate* isolate, reinterpret_cast(input_view.data8()), input_view.length()); } else { - String::Value value(isolate, str); - nbytes = nbytes::HexDecode(buf, buflen, *value, value.length()); + TwoByteValue value(isolate, str); + nbytes = nbytes::HexDecode(buf, buflen, value.out(), value.length()); } break; From 83907b6ca2e475c5a1efa658e8378632c1ce8243 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 13 Oct 2025 15:44:30 +0200 Subject: [PATCH 2/5] tools: add C++ lint rule to avoid using `String::Utf8Value` We should be using our own helpers for this instead. --- tools/cpplint.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tools/cpplint.py b/tools/cpplint.py index 622139efb1e2a7..4b1df2da82da0d 100755 --- a/tools/cpplint.py +++ b/tools/cpplint.py @@ -6489,6 +6489,23 @@ def CheckLocalVectorUsage(filename, lines, error): 'Do not use std::vector>. ' 'Use v8::LocalVector instead.') +def CheckStringValueUsage(filename, lines, error): + """Logs an error if v8's String::Value/Utf8Value are used. + Args: + filename: The name of the current file. + lines: An array of strings, each representing a line of the file. + error: The function to call with any errors found. + """ + for linenum, line in enumerate(lines): + if Search(r'\bString::Utf8Value\b', line): + error(filename, linenum, 'runtime/v8_string_value', 5, + 'Do not use v8::String::Utf8Value. ' + 'Use node::Utf8Value instead.') + if Search(r'\bString::Value\b', line): + error(filename, linenum, 'runtime/v8_string_value', 5, + 'Do not use v8::String::Value. ' + 'Use node::TwoByteValue instead.') + def ProcessLine(filename, file_extension, clean_lines, line, include_state, function_state, nesting_state, error, extra_check_functions=None): @@ -6660,6 +6677,8 @@ def ProcessFileData(filename, file_extension, lines, error, CheckLocalVectorUsage(filename, lines, error) + CheckStringValueUsage(filename, lines, error) + def ProcessConfigOverrides(filename): """ Loads the configuration files and processes the config overrides. From 9226bb1560f57aeed0f56902ee9002b3a398e263 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 13 Oct 2025 20:20:03 +0200 Subject: [PATCH 3/5] fixup! src: use `Utf8Value` and `TwoByteValue` instead of V8 helpers --- src/api/async_resource.cc | 3 ++- src/api/hooks.cc | 8 ++++---- src/node_buffer.cc | 27 +++++++++++++-------------- src/node_errors.cc | 7 +++++-- src/permission/permission.cc | 1 - 5 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/api/async_resource.cc b/src/api/async_resource.cc index 6865eba65300bd..621fca4ba4d1da 100644 --- a/src/api/async_resource.cc +++ b/src/api/async_resource.cc @@ -16,7 +16,8 @@ AsyncResource::AsyncResource(Isolate* isolate, Local resource, const char* name, async_id trigger_async_id) - : AsyncResource(isolate, resource, std::string_view(name), trigger_async_id) {} + : AsyncResource( + isolate, resource, std::string_view(name), trigger_async_id) {} AsyncResource::AsyncResource(Isolate* isolate, Local resource, diff --git a/src/api/hooks.cc b/src/api/hooks.cc index a89d6a9795da3a..e012ee03ceda60 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -12,7 +12,6 @@ using v8::Isolate; using v8::Just; using v8::Local; using v8::Maybe; -using v8::NewStringType; using v8::Nothing; using v8::Object; using v8::String; @@ -201,7 +200,8 @@ async_context EmitAsyncInit(Isolate* isolate, Local resource, const char* name, async_id trigger_async_id) { - return EmitAsyncInit(isolate, resource, std::string_view(name), trigger_async_id); + return EmitAsyncInit( + isolate, resource, std::string_view(name), trigger_async_id); } async_context EmitAsyncInit(Isolate* isolate, @@ -210,8 +210,8 @@ async_context EmitAsyncInit(Isolate* isolate, async_id trigger_async_id) { HandleScope handle_scope(isolate); Local type = ToV8Value(isolate->GetCurrentContext(), name, isolate) - .ToLocalChecked() - .As(); + .ToLocalChecked() + .As(); return EmitAsyncInit(isolate, resource, type, trigger_async_id); } diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 8b896f4c1007b5..cc418017f76803 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -1007,13 +1007,12 @@ void IndexOfString(const FunctionCallbackInfo& args) { offset / 2, is_forward); } else { - result = - nbytes::SearchString(reinterpret_cast(haystack), - haystack_length / 2, - needle_value.out(), - needle_value.length(), - offset / 2, - is_forward); + result = nbytes::SearchString(reinterpret_cast(haystack), + haystack_length / 2, + needle_value.out(), + needle_value.length(), + offset / 2, + is_forward); } result *= 2; } else if (enc == UTF8) { @@ -1022,13 +1021,13 @@ void IndexOfString(const FunctionCallbackInfo& args) { return args.GetReturnValue().Set(-1); CHECK_GE(needle_length, needle_value.length()); - result = - nbytes::SearchString(reinterpret_cast(haystack), - haystack_length, - reinterpret_cast(needle_value.out()), - needle_length, - offset, - is_forward); + result = nbytes::SearchString( + reinterpret_cast(haystack), + haystack_length, + reinterpret_cast(needle_value.out()), + needle_length, + offset, + is_forward); } else if (enc == LATIN1) { uint8_t* needle_data = node::UncheckedMalloc(needle_length); if (needle_data == nullptr) { diff --git a/src/node_errors.cc b/src/node_errors.cc index 4ae66c14a73023..ec84aaafbdc99a 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -1026,8 +1026,11 @@ void PerIsolateMessageListener(Local message, Local error) { Utf8Value filename(isolate, message->GetScriptOrigin().ResourceName()); Utf8Value msg(isolate, message->Get()); // (filename):(line) (message) - std::string warning = SPrintF("%s:%s %s", - filename, message->GetLineNumber(env->context()).FromMaybe(-1), msg); + std::string warning = + SPrintF("%s:%s %s", + filename, + message->GetLineNumber(env->context()).FromMaybe(-1), + msg); USE(ProcessEmitWarningGeneric(env, warning, "V8")); break; } diff --git a/src/permission/permission.cc b/src/permission/permission.cc index 0fd13fd9143f03..2d0e1723050e30 100644 --- a/src/permission/permission.cc +++ b/src/permission/permission.cc @@ -21,7 +21,6 @@ using v8::IntegrityLevel; using v8::Local; using v8::MaybeLocal; using v8::Object; -using v8::String; using v8::Value; namespace permission { From 2f9d49a19c0e51d5d4ef405cfaa5ffa09eee57d4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 13 Oct 2025 20:20:14 +0200 Subject: [PATCH 4/5] fixup! tools: add C++ lint rule to avoid using `String::Utf8Value` --- tools/cpplint.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/cpplint.py b/tools/cpplint.py index 4b1df2da82da0d..20faef069de5ee 100755 --- a/tools/cpplint.py +++ b/tools/cpplint.py @@ -6496,6 +6496,9 @@ def CheckStringValueUsage(filename, lines, error): lines: An array of strings, each representing a line of the file. error: The function to call with any errors found. """ + if filename.startswith('test/') or filename.startswith('test\\'): + return # Skip test files, where Node.js headers may not be available + for linenum, line in enumerate(lines): if Search(r'\bString::Utf8Value\b', line): error(filename, linenum, 'runtime/v8_string_value', 5, From c05de02ea134b3b0543e027269cdbbd3f40a551f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 14 Oct 2025 11:27:45 +0200 Subject: [PATCH 5/5] fixup! src: use `Utf8Value` and `TwoByteValue` instead of V8 helpers --- src/api/hooks.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/api/hooks.cc b/src/api/hooks.cc index e012ee03ceda60..0594dca02cd60a 100644 --- a/src/api/hooks.cc +++ b/src/api/hooks.cc @@ -12,6 +12,7 @@ using v8::Isolate; using v8::Just; using v8::Local; using v8::Maybe; +using v8::NewStringType; using v8::Nothing; using v8::Object; using v8::String; @@ -209,9 +210,10 @@ async_context EmitAsyncInit(Isolate* isolate, std::string_view name, async_id trigger_async_id) { HandleScope handle_scope(isolate); - Local type = ToV8Value(isolate->GetCurrentContext(), name, isolate) - .ToLocalChecked() - .As(); + Local type = + String::NewFromUtf8( + isolate, name.data(), NewStringType::kInternalized, name.size()) + .ToLocalChecked(); return EmitAsyncInit(isolate, resource, type, trigger_async_id); }