url: disallow invalid IPv4 in IPv6 parser#12315
url: disallow invalid IPv4 in IPv6 parser#12315watilde wants to merge 1 commit intonodejs:masterfrom
Conversation
src/node_url.cc
Outdated
There was a problem hiding this comment.
Maybe numbers_seen < 4 is a bit more intuitive than 4 > numbers_seen?
There was a problem hiding this comment.
Sure thing. I've updated it :)
37de626 to
521926a
Compare
src/node_url.cc
Outdated
There was a problem hiding this comment.
Oh, I was just reading the spec from the top to the bottom. The order comes from it, but yeah it's better to not touch for the performance. I will update it :)
src/node_url.cc
Outdated
There was a problem hiding this comment.
This is covered by the if (numbers_seen > 0) { check at the start of the loop, isn't it?
There was a problem hiding this comment.
I think it could happen if the numbers_seen is increased in the loop in the loop that the top loop can't detect: https://github.com/watilde/node/blob/521926ae2f502759c5fc752c82a2661a3dbf419e/src/node_url.cc#L179
There was a problem hiding this comment.
I think I see what you mean but in that case it can be moved to right after the loop, right? And the ch == kEOL clause can be dropped because that's implied by while (ch != kEOL).
There was a problem hiding this comment.
Oh you're right! I just got what you meant of right after the loop. I will update and let's wait for the spec update at whatwg/url#292. Thanks :)
521926a to
a5786ac
Compare
|
Landed in 1b99d8f. Thanks! |
Fixes: #10655.
Checklist
make -j4 testAffected core subsystem(s)
url