src: fix array overrun in node::url::SetArgs() #46541
Conversation
tniessen
added
whatwg-url
nodejs-github-bot
added
c++
|
Fast-track has been requested by @tniessen. Please 👍 to approve. |
github-actions
bot
removed
the
request-ci
| @@ -47,7 +47,7 @@ enum url_update_action { | |||
| kHref = 9, | |||
| }; | |||
|
|
|||
| void SetArgs(Environment* env, Local<Value> argv[12], const ada::result& url) { | |||
| void SetArgs(Environment* env, Local<Value> argv[13], const ada::result& url) { | |||
There was a problem hiding this comment.
Array arguments decay to pointers so this, in the abstract, doesn't fix anything. You could change it to:
| void SetArgs(Environment* env, Local<Value> argv[13], const ada::result& url) { | |
| void SetArgs(Environment* env, Local<Value> (*argv)[13], const ada::result& url) { |
That forces callers to pass a 13-element array by address, i.e.:
Local<Value> argv[13];
SetArgs(env, &argv, url);You'll need to update all the assignments from argv[0] to (*argv)[0].
There was a problem hiding this comment.
For education purposes: How did the code work before this change?
There was a problem hiding this comment.
It wrote past the end. Array arguments aren't length-checked; to the compiler int x[42] equals int x[] equals int *x.
|
cc @nodejs/url |
|
Seems like #46736 implicitly overwrote this. |
|
I believe @bnoordhuis's comment above is still relevant though. @anonrig is there any chance you could pick it up in a future PR? |
I agree. I'll open a PR. Thanks for the mention. |
Refs: #46410