-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ignore responses with unexpected IDs #1155
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1155 +/- ##
=======================================
Coverage 55.89% 55.89%
=======================================
Files 41 41
Lines 10141 10142 +1
=======================================
+ Hits 5668 5669 +1
Misses 3468 3468
Partials 1005 1005
Continue to review full report at Codecov.
|
[ Quoting <[email protected]> in "[miekg/dns] Ignore responses with u..." ]
This fixes the following problem:
ErrID is returned in this case and the caller needs to check for this. There are other
checks that can be done as well, such as double checking if the case of the domain names
matches what was sent.
nacking this pr, as it may be useful for debug programs to actually ignore the ID.
/close
|
Codecov Report
@@ Coverage Diff @@
## master #1155 +/- ##
==========================================
- Coverage 55.89% 55.12% -0.77%
==========================================
Files 41 42 +1
Lines 10141 9581 -560
==========================================
- Hits 5668 5282 -386
+ Misses 3468 3274 -194
- Partials 1005 1025 +20
Continue to review full report at Codecov.
|
I've made the requested changes. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty much perfect to me and definitely the right approach IMO. LGTM.
|
this needs a rebase to pick up the travis changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebase + fix minor nit please.
This fixes the following problem: At time 0, we send a query with ID X from port P. At time T, we time out the query due to lack of response, and then send a different query with ID Y. By coincidence, the new query is sent from the same port number P (since port numbers are only 16 bits, this can happen with non-negligible probability when making queries at a high rate). At time T+epsilon, we receive a response to the original query. Since the ID in this response is X, not Y, we would previously return ErrId, preventing the second query from succeeding. With this commit, we simply ignore the response with the mismatched ID and return once we receive the response with the correct ID.
The new test sends two replies: the first one has a bad ID, which should be ignored, and the second one has the correct ID.
Should be good now. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
* Ignore replies with unexpected IDs This fixes the following problem: At time 0, we send a query with ID X from port P. At time T, we time out the query due to lack of response, and then send a different query with ID Y. By coincidence, the new query is sent from the same port number P (since port numbers are only 16 bits, this can happen with non-negligible probability when making queries at a high rate). At time T+epsilon, we receive a response to the original query. Since the ID in this response is X, not Y, we would previously return ErrId, preventing the second query from succeeding. With this commit, we simply ignore the response with the mismatched ID and return once we receive the response with the correct ID. * Update test for bad ID The new test sends two replies: the first one has a bad ID, which should be ignored, and the second one has the correct ID. * Add test to ensure query times out when server returns bad ID * Avoid use of error string matching in test case * Check for mismatched query IDs when using TCP * Reduce timeout in TestClientSyncBadID
This fixes the following problem:
At time 0, we send a query with ID X from port P.
At time T, we time out the query due to lack of response, and then send a different query with ID Y. By coincidence, the new query is sent from the same port number P (since port numbers are only 16 bits, this can happen with non-negligible probability when making queries at a high rate).
At time T+epsilon, we receive a response to the original query. Since the ID in this response is X, not Y,
exchange
returnsErrId
, preventing the second query from succeeding.Since UDP does not have connections, there's no guarantee that a response that we receive is actually associated with the query that we sent - the ID is needed to make the association. Therefore, it's more appropriate to ignore replies with unexpected IDs that to consider them an error. Accordingly, this pull request changes
exchange
to ignore responses with mismatched IDs.exchange
continues waiting for a response with the expected ID and returns it when it arrives.