dns: fix crash on setServers with port 0 - #65009
Conversation
Sorry, something went wrong.
Signed-off-by: Lazizbek Ergashev <lazerg2@gmail.com>
|
Review requested:
|
Sorry, something went wrong.
There was a problem hiding this comment.
👍
Note that this preserves the previous behaviour of crashing the process if the port was greater than INT_MAX. Indeed, even below that point, any port value greater than 65535 overflows:
> dns.setServers(['1.1.1.1:65535', '1.1.1.1:65536', '1.1.1.1:65537'])
> dns.getServers()
[ '1.1.1.1:65535', '1.1.1.1', '1.1.1.1:1' ]If you're happy, I think it would be a worthwhile addition to add validation to the port values, either as part of this PR or as a follow-up PR?
Sorry, something went wrong.
|
Thanks for the review. I couldn't reproduce the wraparound though — on this branch (c-ares 1.34.8, bundled, untouched by this diff): > dns.setServers(['1.1.1.1:65535', '1.1.1.1:65536', '1.1.1.1:65537']) Uncaught Error [ERR_DNS_SET_SERVERS_FAILED]: c-ares failed to set servers: "Misformatted string" [...] ares_parse_port() (deps/cares/src/lib/str/ares_str.c) already caps at 65535 via ares_str_parse_uint(str, 65535UL, &val), so the CSV parse fails and the whole call throws, restoring the previous server list. That check is unrelated to this PR's diff, so it should behave the same on main. Might be version-dependent on your end. Since I can't reproduce silent corruption on the current bundled c-ares, I'd rather not scope-creep this PR with validatePort() unless you still want the earlier/clearer JS-side error as its own improvement — happy to open that as a follow-up if so. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
This comment was marked as outdated.
Codecov Report❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #65009 +/- ##
==========================================
- Coverage 90.29% 90.29% -0.01%
==========================================
Files 762 759 -3
Lines 247646 247605 -41
Branches 46709 46672 -37
==========================================
- Hits 223619 223564 -55
- Misses 15485 15516 +31
+ Partials 8542 8525 -17
... and 54 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
Sorry, something went wrong.
dns.setServers(['1.1.1.1:0']) aborts the process:
SetServers() in src/cares_wrap.cc checked the port with CHECK(portValue->Int32Value(env->context()).FromJust()). That asserts the port value is non-zero, not that the conversion succeeded, so a port of 0 trips it. The family value had the same problem. Both now use Maybe::To(), like the rest of the file.
c-ares already treats port 0 as "use the default port", so 1.1.1.1:0 becomes 1.1.1.1:53. [::1]:0 behaved that way already, since the bracketed-IPv6 branch in lib/internal/dns/utils.js coerces 0 to 53 before it ever reaches C++.
Fixes: #65006