Repository navigation
Conversation
SetLocalAddress() applied the first address to the c-ares channel with ares_set_local_ip4()/ares_set_local_ip6() before parsing and validating the second one. A call that then threw ERR_INVALID_ARG_VALUE, because both addresses were of the same family or the second address was invalid, left the first address in effect, so later queries were sent from an address the caller had been told was rejected. Parse and validate both arguments first and only then apply them. With a single argument the other family is still reset to "any", as before. Fixes: nodejs#67049 Signed-off-by: Martin Mirchev <mmirchev@comp.nus.edu.sg>
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
Contributor
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolver#setLocalAddress()applied the first address to the c-ares channel before it parsedand validated the second one. A call that then threw
ERR_INVALID_ARG_VALUE(two addresses ofthe same family, or an invalid second address) left the first address applied, so later queries
went out from an address the caller had been told was rejected.
This change parses and validates both arguments first and only then calls
ares_set_local_ip4()/ares_set_local_ip6(). In plain terms,SetLocalAddress()nowdecides whether the whole call is valid before it touches the channel, so a call that throws
changes nothing. The behaviour for valid calls is unchanged: the two setters are called in the
same order as before, and with a single argument the other family is still reset to "any".
The new test,
test/parallel/test-dns-setlocaladdress-rejected.js, runs a local UDP serveron 127.0.0.1 and records the source address of the DNS query. It calls
setLocalAddress('127.0.0.2'), then two calls that throw ('127.0.0.3', '127.0.0.4'and'127.0.0.5', 'bad'), and checks that the query still comes from 127.0.0.2. It needs nonetwork access. It is skipped on non-Linux platforms, where only 127.0.0.1 is bound to the
loopback interface by default.
Fixes: #67049
This bug was found by an LLM-assisted static analysis tool; the fix and test were drafted with
the help of a coding agent. I reproduced the bug and verified the fix on Linux (the test fails
before the change and passes after it), and ran the existing dns tests.