url: fix array overrun in node:url::SetArgs() by anonrig · Pull Request #47001 · nodejs/node · GitHub

I am following up on @tniessen's pull request and @bnoordhuis's comments. @TimothyGu, thanks for the mention & reminder

Review requested:

@nodejs/url

CI: https://ci.nodejs.org/job/node-test-pull-request/50263/

CI: https://ci.nodejs.org/job/node-test-pull-request/50268/

Small suggestion; otherwise lgtm.

Ideally this parameter should be an const ada::url&. It doesn't make sense to pass an ada::result in when we always expect it to be filled with the "valid" value.

In fact the call sites should probably change from this:

  ada::result out = ada::parse(input.ToStringView());
  CHECK(out);
  out->set_protocol(…);

to

  ada::result out = ada::parse(input.ToStringView());
  CHECK(out);
  const ada::url& url = out.value();
  url.set_protocol(…);

out.value() returns a copy. The current implementation looks unsafe, but CHECK(out) ensures that it is not. I tried to avoid returning a copy. @lemire wrote about this on Ada's discussion board: ada-url/ada#200

I think you misunderstood ada-url/ada#200. out.value() (and equivalently *out) does not return a copy by itself. It returns a reference, so if you put it into a const ada::url& then no copies are made.

On the other hand, ada::url url = *out, which would make a copy. That's not what I'm suggesting here.

Alternatively, you can also do

  ada::result out = ada::parse(input.ToStringView());
  CHECK(out);
  ada::url url = std::move(*out);

which is written under the "performance tip". It would also avoid a copy.

It is likely that the current code is safe and efficient. I think we are discussing 'coding style' which is subjective.

I don't personally find the url->... annoying.

I am slightly triggered by the (*argv)[..] however. :-)

What about using a reference to an array instead?

Local<Value> (&argv)[10]

(The ampersand would be dropped from the calling site.)

It's a style guide thing: pointers for things that are mutated, const references otherwise.

With mutable references it's sometimes ambiguous to a reader if code operates on the original or on a copy. With pointers, no such ambiguity exists.

The current fix LGTM but it feels like a C-style fix. Since this is a C++ file, we could also leverage the power of C++:

Since the problem described in #46541 (comment) is about array arguments decaying to pointers, why don't we use std::array here instead? The doc states:

Unlike a C-style array, it doesn't decay to T* automatically.

Doing that should also address https://github.com/nodejs/node/pull/47001/files#r1130218455:

I am slightly triggered by the (*argv)[..] however. :-)

@RaisinTen per convention, the std::array would have to be passed by pointer, so IMO it doesn't provide much benefit. More elaboration in #47001 (comment).

Or you could just return a std::vector<Local<Value> and do Call(...., argv.size(), argv.data()) later ;) I doubt there is any meaningful performance difference between the two, and no one needs to actually count the number of arguments in the code that way, and you can get rid of the undef fillers in the caller.

You could also just return a std::array? That would make intention clear better than an out parameters does, and would still avoid the dynamic memory allocation from std::vector (in fact, I'd expect it to compile to pretty much the same code as the current state of this PR).

Since there isn't any request for changes on this pull request, I'll add a commit-queue label.

Landed in 72e971e

Implements a review suggestion from 72e971e. Refs: nodejs#47001 (comment)

PR-URL: #47001 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com>

Implements a review suggestion from 72e971e. Refs: #47001 (comment) PR-URL: #47035 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

PR-URL: #47001 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com>

Implements a review suggestion from 72e971e. Refs: #47001 (comment) PR-URL: #47035 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

blocked by #46736

PR-URL: nodejs#47001 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com>

Implements a review suggestion from 72e971e. Refs: nodejs#47001 (comment) PR-URL: nodejs#47035 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

PR-URL: #47001 Backport-PR-URL: #48345 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com>

Implements a review suggestion from 72e971e. Refs: #47001 (comment) PR-URL: #47035 Backport-PR-URL: #48345 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 7, 2023

url: fix array overrun in node:url::SetArgs()

4d5188a

anonrig force-pushed the improve-url-overrun branch from b099ef0 to 4d5188a Compare March 7, 2023 21:52

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2023

TimothyGu approved these changes Mar 8, 2023

View reviewed changes

bnoordhuis approved these changes Mar 8, 2023

View reviewed changes

anonrig added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Mar 8, 2023

TimothyGu removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 8, 2023

lemire approved these changes Mar 9, 2023

View reviewed changes

github-actions Bot mentioned this pull request Mar 9, 2023

CI Reliability 2023-03-09 nodejs/reliability#518

Open

21 tasks

RaisinTen approved these changes Mar 9, 2023

View reviewed changes

github-actions Bot mentioned this pull request Mar 10, 2023

CI Reliability 2023-03-10 nodejs/reliability#519

Open

22 tasks

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 10, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 10, 2023

nodejs-github-bot merged commit 72e971e into nodejs:main Mar 10, 2023

anonrig deleted the improve-url-overrun branch March 10, 2023 14:11

addaleax mentioned this pull request Mar 10, 2023

src: use std::array for passing argv in node::url #47035

Merged

addaleax added a commit to addaleax/node that referenced this pull request Mar 10, 2023

src: use std::array for passing argv in node::url

afd9550

Implements a review suggestion from 72e971e. Refs: nodejs#47001 (comment)

github-actions Bot mentioned this pull request Mar 13, 2023

CI Reliability 2023-03-13 nodejs/reliability#522

Open

20 tasks

targos pushed a commit that referenced this pull request Mar 13, 2023

url: fix array overrun in node:url::SetArgs()

43c2d41

PR-URL: #47001 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com>

targos pushed a commit that referenced this pull request Mar 14, 2023

url: fix array overrun in node:url::SetArgs()

e754277

PR-URL: #47001 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Darshan Sen <raisinten@gmail.com>

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

danielleadams added the backport-blocked-v18.x label Apr 3, 2023

anonrig mentioned this pull request Jun 5, 2023

[v18.x] backport Ada v2.x to Node.js v18 #48345

Closed

19 tasks

anonrig added backport-open-v18.x and removed backport-blocked-v18.x labels Jun 5, 2023

danielleadams mentioned this pull request Jul 12, 2023

v18.17.0 release proposal #48694

Merged

danielleadams added backported-to-v18.x and removed backport-open-v18.x labels Jul 12, 2023

-              void SetArgs(Environment* env, Local<Value> argv[10], const ada::result& url) {
+              void SetArgs(Environment* env,
+                           Local<Value> (*argv)[10],
+                           const ada::result& url) {

url: fix array overrun in node:url::SetArgs() by anonrig · Pull Request #47001 · nodejs/node · GitHub

Uh oh!

Uh oh!

url: fix array overrun in node:url::SetArgs() - #47001

url: fix array overrun in node:url::SetArgs()#47001nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom anonrig:improve-url-overrunanonrig/node:improve-url-overrunCopy head branch name to clipboard

Conversation

anonrig commented Mar 7, 2023

Uh oh!

nodejs-github-bot commented Mar 7, 2023

Uh oh!

nodejs-github-bot commented Mar 7, 2023

Uh oh!

nodejs-github-bot commented Mar 8, 2023

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

anonrig Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

TimothyGu Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

lemire Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

RaisinTen left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Mar 9, 2023

Uh oh!

joyeecheung commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Mar 9, 2023

Uh oh!

anonrig commented Mar 10, 2023

Uh oh!

nodejs-github-bot commented Mar 10, 2023

Uh oh!

danielleadams commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

url: fix array overrun in node:url::SetArgs()#47001
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:improve-url-overrun

joyeecheung commented Mar 9, 2023 •
edited
Loading