From 6af6cd34aa16b567cb0386dbb47fe10debb06c41 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 1 Feb 2025 12:10:03 -0800 Subject: [PATCH 1/8] src: remove USE from node_url_pattern to handle error correctly --- src/node_url_pattern.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index 3d6340565d1e06..aa31489fcd147a 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -371,7 +371,9 @@ MaybeLocal URLPattern::URLPatternComponentResult::ToJSObject( } else { value = Undefined(isolate); } - USE(parsed_group->Set(context, key, value)); + if (parsed_group->Set(context, key, value).IsNothing()) { + return {}; + } } Local input; if (!ToV8Value(env->context(), result.input).ToLocal(&input)) { From 73a974563e33fec98bb01eefec4a1524302a410e Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 1 Feb 2025 12:12:20 -0800 Subject: [PATCH 2/8] src: add basic_string_view to memorytracker TrackField --- src/memory_tracker-inl.h | 8 ++++++++ src/memory_tracker.h | 4 ++++ src/node_url_pattern.cc | 14 +++++++------- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/memory_tracker-inl.h b/src/memory_tracker-inl.h index c99ff8607100ba..8446dd4e6a4290 100644 --- a/src/memory_tracker-inl.h +++ b/src/memory_tracker-inl.h @@ -211,6 +211,14 @@ void MemoryTracker::TrackField(const char* edge_name, TrackFieldWithSize(edge_name, value.size() * sizeof(T), "std::basic_string"); } +template +void MemoryTracker::TrackField(const char* edge_name, + const std::basic_string_view& value, + const char* node_name) { + TrackFieldWithSize( + edge_name, value.size() * sizeof(T), "std::basic_string_view"); +} + template void MemoryTracker::TrackField(const char* edge_name, const v8::Eternal& value, diff --git a/src/memory_tracker.h b/src/memory_tracker.h index cae4e5c7a663c1..367a88a92d19cb 100644 --- a/src/memory_tracker.h +++ b/src/memory_tracker.h @@ -199,6 +199,10 @@ class MemoryTracker { inline void TrackField(const char* edge_name, const std::basic_string& value, const char* node_name = nullptr); + template + inline void TrackField(const char* edge_name, + const std::basic_string_view& value, + const char* node_name = nullptr); template ::is_specialized, bool>::type, diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index aa31489fcd147a..bbfad88fba046f 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -123,13 +123,13 @@ URLPattern::URLPattern(Environment* env, } void URLPattern::MemoryInfo(MemoryTracker* tracker) const { - tracker->TrackFieldWithSize("protocol", url_pattern_.get_protocol().size()); - tracker->TrackFieldWithSize("username", url_pattern_.get_username().size()); - tracker->TrackFieldWithSize("password", url_pattern_.get_password().size()); - tracker->TrackFieldWithSize("hostname", url_pattern_.get_hostname().size()); - tracker->TrackFieldWithSize("pathname", url_pattern_.get_pathname().size()); - tracker->TrackFieldWithSize("search", url_pattern_.get_search().size()); - tracker->TrackFieldWithSize("hash", url_pattern_.get_hash().size()); + tracker->TrackField("protocol", url_pattern_.get_protocol()); + tracker->TrackField("username", url_pattern_.get_username()); + tracker->TrackField("password", url_pattern_.get_password()); + tracker->TrackField("hostname", url_pattern_.get_hostname()); + tracker->TrackField("pathname", url_pattern_.get_pathname()); + tracker->TrackField("search", url_pattern_.get_search()); + tracker->TrackField("hash", url_pattern_.get_hash()); } void URLPattern::New(const FunctionCallbackInfo& args) { From 0b9f108cc9b405f5d5a846b7381f6fa75650ad26 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 1 Feb 2025 12:16:48 -0800 Subject: [PATCH 3/8] src: simplify if block in node_url_pattern --- src/node_url_pattern.cc | 42 ++++++++++++++--------------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index bbfad88fba046f..f6da479d2451aa 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -420,34 +420,20 @@ MaybeLocal URLPattern::URLPatternResult::ToJSValue( LocalVector values(isolate, arraysize(names)); values[0] = Array::New(isolate, inputs.data(), inputs.size()); if (!URLPatternComponentResult::ToJSObject(env, result.protocol) - .ToLocal(&values[1])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.username) - .ToLocal(&values[2])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.password) - .ToLocal(&values[3])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.hostname) - .ToLocal(&values[4])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.port) - .ToLocal(&values[5])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.pathname) - .ToLocal(&values[6])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.search) - .ToLocal(&values[7])) { - return {}; - } - if (!URLPatternComponentResult::ToJSObject(env, result.hash) + .ToLocal(&values[1]) || + !URLPatternComponentResult::ToJSObject(env, result.username) + .ToLocal(&values[2]) || + !URLPatternComponentResult::ToJSObject(env, result.password) + .ToLocal(&values[3]) || + !URLPatternComponentResult::ToJSObject(env, result.hostname) + .ToLocal(&values[4]) || + !URLPatternComponentResult::ToJSObject(env, result.port) + .ToLocal(&values[5]) || + !URLPatternComponentResult::ToJSObject(env, result.pathname) + .ToLocal(&values[6]) || + !URLPatternComponentResult::ToJSObject(env, result.search) + .ToLocal(&values[7]) || + !URLPatternComponentResult::ToJSObject(env, result.hash) .ToLocal(&values[8])) { return {}; } From a442a22cfe848b8283853842f9c2a1d892af50dd Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 1 Feb 2025 12:26:37 -0800 Subject: [PATCH 4/8] src: improve error handling in node_url_pattern --- src/node_url_pattern.cc | 14 +++++++++++--- src/node_url_pattern.h | 4 ++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index f6da479d2451aa..2dba47ea4e400a 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -300,7 +300,7 @@ MaybeLocal URLPattern::URLPatternInit::ToJsObject( return result; } -ada::url_pattern_init URLPattern::URLPatternInit::FromJsObject( +std::optional URLPattern::URLPatternInit::FromJsObject( Environment* env, Local obj) { ada::url_pattern_init init{}; Local components[] = { @@ -344,6 +344,10 @@ ada::url_pattern_init URLPattern::URLPatternInit::FromJsObject( Utf8Value utf8_value(isolate, value); set_parameter(key.ToStringView(), utf8_value.ToStringView()); } + } else { + // If ToLocal failed then we assume an error occurred, + // bail out early to propagate the error. + return std::nullopt; } } return init; @@ -541,7 +545,9 @@ void URLPattern::Exec(const FunctionCallbackInfo& args) { input_base = input_value.ToString(); input = std::string_view(input_base); } else if (args[0]->IsObject()) { - input = URLPatternInit::FromJsObject(env, args[0].As()); + auto maybeInput = URLPatternInit::FromJsObject(env, args[0].As()); + if (!maybeInput.has_value()) return; + input = std::move(*maybeInput); } else { THROW_ERR_INVALID_ARG_TYPE( env, "URLPattern input needs to be a string or an object"); @@ -582,7 +588,9 @@ void URLPattern::Test(const FunctionCallbackInfo& args) { input_base = input_value.ToString(); input = std::string_view(input_base); } else if (args[0]->IsObject()) { - input = URLPatternInit::FromJsObject(env, args[0].As()); + auto maybeInput = URLPatternInit::FromJsObject(env, args[0].As()); + if (!maybeInput.has_value()) return; + input = std::move(*maybeInput); } else { THROW_ERR_INVALID_ARG_TYPE( env, "URLPattern input needs to be a string or an object"); diff --git a/src/node_url_pattern.h b/src/node_url_pattern.h index 99a2521513b55a..bfb0cd00fa8151 100644 --- a/src/node_url_pattern.h +++ b/src/node_url_pattern.h @@ -59,8 +59,8 @@ class URLPattern : public BaseObject { class URLPatternInit { public: - static ada::url_pattern_init FromJsObject(Environment* env, - v8::Local obj); + static std::optional FromJsObject( + Environment* env, v8::Local obj); static v8::MaybeLocal ToJsObject( Environment* env, const ada::url_pattern_init& init); }; From 5ed8d3d196139d7aeca0e15b435dd55875744809 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 1 Feb 2025 12:38:22 -0800 Subject: [PATCH 5/8] src: fix error propagation on URLPattern init --- src/node_url_pattern.cc | 18 ++++++++++++++++-- test/parallel/test-urlpattern.js | 28 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-urlpattern.js diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index 2dba47ea4e400a..0a7a03583ad2e4 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -165,6 +165,10 @@ void URLPattern::New(const FunctionCallbackInfo& args) { input = input_buffer.ToString(); } else if (args[0]->IsObject()) { init = URLPatternInit::FromJsObject(env, args[0].As()); + // If init does not have a value here, the implication is that an + // error was thrown. Let's allow that to be handled now by returning + // early. If we don't, the error thrown will be swallowed. + if (!init.has_value()) return; } else { THROW_ERR_INVALID_ARG_TYPE(env, "Input must be an object or a string"); return; @@ -180,7 +184,9 @@ void URLPattern::New(const FunctionCallbackInfo& args) { CHECK(!options.has_value()); options = URLPatternOptions::FromJsObject(env, args[1].As()); if (!options) { - THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean"); + // If options does not have a value, we assume an error was + // thrown and scheduled on the isolate. Return early to + // propagate it. return; } } else { @@ -197,7 +203,9 @@ void URLPattern::New(const FunctionCallbackInfo& args) { CHECK(!options.has_value()); options = URLPatternOptions::FromJsObject(env, args[2].As()); if (!options) { - THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean"); + // If options does not have a value, we assume an error was + // thrown and scheduled on the isolate. Return early to + // propagate it. return; } } @@ -453,9 +461,15 @@ URLPattern::URLPatternOptions::FromJsObject(Environment* env, if (obj->Get(env->context(), env->ignore_case_string()) .ToLocal(&ignore_case)) { if (!ignore_case->IsBoolean()) { + THROW_ERR_INVALID_ARG_TYPE(env, "options.ignoreCase must be a boolean"); return std::nullopt; } options.ignore_case = ignore_case->IsTrue(); + } else { + // If ToLocal returns false, the assumption is that getting the + // ignore_case_string threw an error, let's propagate that now + // by returning std::nullopt. + return std::nullopt; } return options; } diff --git a/test/parallel/test-urlpattern.js b/test/parallel/test-urlpattern.js new file mode 100644 index 00000000000000..1a8d722c5d3e87 --- /dev/null +++ b/test/parallel/test-urlpattern.js @@ -0,0 +1,28 @@ +'use strict'; + +require('../common'); + +const { throws } = require('assert'); +const { URLPattern } = require('url'); + +// Verify that if an error is thrown while accessing any of the +// init options, the error is appropriately propagated. +throws(() => { + new URLPattern({ + get protocol() { + throw new Error('boom'); + } + }); +}, { + message: 'boom', +}); + +// Verify that if an error is thrown while accessing the ignoreCase +// option, the error is appropriately propagated. +throws(() => { + new URLPattern({}, { get ignoreCase() { + throw new Error('boom'); + } }); +}, { + message: 'boom' +}); From 00f904fc2451bf5a8652e81d13c450c6bfa8687b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 1 Feb 2025 12:49:40 -0800 Subject: [PATCH 6/8] src: use ToV8Value where more convenient in node_url_pattern --- src/node_url_pattern.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index 0a7a03583ad2e4..1e41f374f8e57b 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -367,12 +367,8 @@ MaybeLocal URLPattern::URLPatternComponentResult::ToJSObject( auto context = env->context(); auto parsed_group = Object::New(isolate); for (const auto& [group_key, group_value] : result.groups) { - Local key; - if (!String::NewFromUtf8(isolate, - group_key.c_str(), - NewStringType::kNormal, - group_key.size()) - .ToLocal(&key)) { + Local key; + if (!ToV8Value(context, group_key).ToLocal(&key)) { return {}; } Local value; From c9962a692373b15fad05b423b76c6281f750937b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 1 Feb 2025 13:07:44 -0800 Subject: [PATCH 7/8] src: simplify URLPatternInit::ToJsObject by reducing boilerplate --- src/memory_tracker-inl.h | 8 ---- src/memory_tracker.h | 4 -- src/node_url_pattern.cc | 94 +++++++++++----------------------------- 3 files changed, 25 insertions(+), 81 deletions(-) diff --git a/src/memory_tracker-inl.h b/src/memory_tracker-inl.h index 8446dd4e6a4290..c99ff8607100ba 100644 --- a/src/memory_tracker-inl.h +++ b/src/memory_tracker-inl.h @@ -211,14 +211,6 @@ void MemoryTracker::TrackField(const char* edge_name, TrackFieldWithSize(edge_name, value.size() * sizeof(T), "std::basic_string"); } -template -void MemoryTracker::TrackField(const char* edge_name, - const std::basic_string_view& value, - const char* node_name) { - TrackFieldWithSize( - edge_name, value.size() * sizeof(T), "std::basic_string_view"); -} - template void MemoryTracker::TrackField(const char* edge_name, const v8::Eternal& value, diff --git a/src/memory_tracker.h b/src/memory_tracker.h index 367a88a92d19cb..cae4e5c7a663c1 100644 --- a/src/memory_tracker.h +++ b/src/memory_tracker.h @@ -199,10 +199,6 @@ class MemoryTracker { inline void TrackField(const char* edge_name, const std::basic_string& value, const char* node_name = nullptr); - template - inline void TrackField(const char* edge_name, - const std::basic_string_view& value, - const char* node_name = nullptr); template ::is_specialized, bool>::type, diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index 1e41f374f8e57b..b9f9d5585e3722 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -123,13 +123,13 @@ URLPattern::URLPattern(Environment* env, } void URLPattern::MemoryInfo(MemoryTracker* tracker) const { - tracker->TrackField("protocol", url_pattern_.get_protocol()); - tracker->TrackField("username", url_pattern_.get_username()); - tracker->TrackField("password", url_pattern_.get_password()); - tracker->TrackField("hostname", url_pattern_.get_hostname()); - tracker->TrackField("pathname", url_pattern_.get_pathname()); - tracker->TrackField("search", url_pattern_.get_search()); - tracker->TrackField("hash", url_pattern_.get_hash()); + tracker->TrackFieldWithSize("protocol", url_pattern_.get_protocol().size()); + tracker->TrackFieldWithSize("username", url_pattern_.get_username().size()); + tracker->TrackFieldWithSize("password", url_pattern_.get_password().size()); + tracker->TrackFieldWithSize("hostname", url_pattern_.get_hostname().size()); + tracker->TrackFieldWithSize("pathname", url_pattern_.get_pathname().size()); + tracker->TrackFieldWithSize("search", url_pattern_.get_search().size()); + tracker->TrackFieldWithSize("hash", url_pattern_.get_hash().size()); } void URLPattern::New(const FunctionCallbackInfo& args) { @@ -242,68 +242,24 @@ MaybeLocal URLPattern::URLPatternInit::ToJsObject( auto isolate = env->isolate(); auto context = env->context(); auto result = Object::New(isolate); - if (init.protocol) { - Local protocol; - if (!ToV8Value(context, *init.protocol).ToLocal(&protocol) || - result->Set(context, env->protocol_string(), protocol).IsNothing()) { - return {}; - } - } - if (init.username) { - Local username; - if (!ToV8Value(context, *init.username).ToLocal(&username) || - result->Set(context, env->username_string(), username).IsNothing()) { - return {}; - } - } - if (init.password) { - Local password; - if (!ToV8Value(context, *init.password).ToLocal(&password) || - result->Set(context, env->password_string(), password).IsNothing()) { - return {}; - } - } - if (init.hostname) { - Local hostname; - if (!ToV8Value(context, *init.hostname).ToLocal(&hostname) || - result->Set(context, env->hostname_string(), hostname).IsNothing()) { - return {}; - } - } - if (init.port) { - Local port; - if (!ToV8Value(context, *init.port).ToLocal(&port) || - result->Set(context, env->port_string(), port).IsNothing()) { - return {}; - } - } - if (init.pathname) { - Local pathname; - if (!ToV8Value(context, *init.pathname).ToLocal(&pathname) || - result->Set(context, env->pathname_string(), pathname).IsNothing()) { - return {}; - } - } - if (init.search) { - Local search; - if (!ToV8Value(context, *init.search).ToLocal(&search) || - result->Set(context, env->search_string(), search).IsNothing()) { - return {}; - } - } - if (init.hash) { - Local hash; - if (!ToV8Value(context, *init.hash).ToLocal(&hash) || - result->Set(context, env->hash_string(), hash).IsNothing()) { - return {}; - } - } - if (init.base_url) { - Local base_url; - if (!ToV8Value(context, *init.base_url).ToLocal(&base_url) || - result->Set(context, env->base_url_string(), base_url).IsNothing()) { - return {}; - } + + const auto trySet = [&](auto name, const std::optional& val) { + if (!val.has_value()) return true; + Local temp; + return ToV8Value(context, *val).ToLocal(&temp) && + result->Set(context, name, temp).IsJust(); + }; + + if (!trySet(env->protocol_string(), init.protocol) || + !trySet(env->username_string(), init.username) || + !trySet(env->password_string(), init.password) || + !trySet(env->hostname_string(), init.hostname) || + !trySet(env->port_string(), init.port) || + !trySet(env->pathname_string(), init.pathname) || + !trySet(env->search_string(), init.search) || + !trySet(env->hash_string(), init.hash) || + !trySet(env->base_url_string(), init.base_url)) { + return {}; } return result; } From 72e39c383e43914b59b2de1093660fc739cf76b4 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 2 Feb 2025 09:14:18 -0800 Subject: [PATCH 8/8] Apply suggestions from code review Co-authored-by: Yagiz Nizipli --- src/node_url_pattern.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_url_pattern.cc b/src/node_url_pattern.cc index b9f9d5585e3722..23331460853c29 100644 --- a/src/node_url_pattern.cc +++ b/src/node_url_pattern.cc @@ -168,7 +168,7 @@ void URLPattern::New(const FunctionCallbackInfo& args) { // If init does not have a value here, the implication is that an // error was thrown. Let's allow that to be handled now by returning // early. If we don't, the error thrown will be swallowed. - if (!init.has_value()) return; + if (!init) return; } else { THROW_ERR_INVALID_ARG_TYPE(env, "Input must be an object or a string"); return; @@ -244,7 +244,7 @@ MaybeLocal URLPattern::URLPatternInit::ToJsObject( auto result = Object::New(isolate); const auto trySet = [&](auto name, const std::optional& val) { - if (!val.has_value()) return true; + if (!val) return true; Local temp; return ToV8Value(context, *val).ToLocal(&temp) && result->Set(context, name, temp).IsJust();