src: fix array overrun in node::url::SetArgs()#46541
Uh oh!
There was an error while loading. Please reload this page.
Conversation
tniessen added whatwg-url 
nodejs-github-bot added c++ 
Fast-track has been requested by @tniessen. Please 👍 to approve. |
github-actionsbot removed the request-ci 
nodejs-github-bot commented Feb 7, 2023
| }; | ||
| voidSetArgs(Environment* env, Local<Value> argv[12], const ada::result& url){ | ||
| voidSetArgs(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:
| voidSetArgs(Environment* env, Local<Value> argv[13], const ada::result& url){ | |
| voidSetArgs(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.
anonrig commented Feb 7, 2023
cc @nodejs/url |
tniessen commented Mar 2, 2023
Seems like #46736 implicitly overwrote this. |
TimothyGu commented Mar 7, 2023
I believe @bnoordhuis's comment above is still relevant though. @anonrig is there any chance you could pick it up in a future PR? |
anonrig commented Mar 7, 2023
I agree. I'll open a PR. Thanks for the mention. |
6 participants
Refs: #46410