[Fix] Preserve req->pos during reply validation to prevent packet truncation#5739
Merged
Conversation
…ncation on retransmit The rdns_request_reply_cmp function modifies req->pos as a side effect during reply validation. This caused packet truncation on retransmits because req->pos (which tracks the full packet length) was overwritten with the end position of the question section. On timeout/retry, rdns_send_request would use the corrupted req->pos value, resulting in truncated packets missing the OPT additional section. This made resolvers like Knot and PowerDNS unable to parse retransmitted packets. Fix by saving and restoring req->pos around the reply comparison logic.
1 task
1 task
Member
Author
|
@droid review |
| } | ||
| } | ||
|
|
||
| /* Switch IO channel from UDP to TCP */ |
Contributor
There was a problem hiding this comment.
Bug: by moving the channel switch before allocating oc, the failure path returns false after we have already removed the request from the UDP hash, changed req->io, and potentially regenerated the id. Because we exit before assigning kh_value, re-arming the timer, or releasing old_ioc, the request is left detached while the old channel keeps its ref. Please defer the switch until after calloc succeeds (as before) or add explicit rollback on error.
When switching from UDP to TCP (e.g., on truncated response), the code was copying the packet to the TCP output buffer before regenerating the transaction ID to avoid collisions. This resulted in both UDP and TCP packets having the same transaction ID. Fix by moving the ID regeneration and IO channel switch logic before the memcpy to the TCP output buffer, ensuring the TCP packet contains the updated transaction ID.
vstakhov
force-pushed
the
vstakhov-rdns-fix-truncation
branch
from
November 10, 2025 09:39
4f5c277 to
c774de1
Compare
Member
Author
|
@droid review |
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
1 participant
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.
Problem
The rdns library was truncating DNS packets on retransmit, causing the OPT additional section to be missing. This made resolvers like Knot-Resolver and PowerDNS-Recursor unable to parse retransmitted packets.
Root Cause
The
rdns_request_reply_cmpfunction modifiesreq->posas a side effect during reply validation. Thereq->posfield serves dual purposes:When a request times out and needs retransmit:
req->pos(includes OPT section)req->poswith end of question sectionreq->pos, truncating the packetSolution
Save and restore
req->posaround the reply comparison logic inrdns_parse_reply. This ensures the original packet length is preserved for potential retransmits while still allowing the comparison function to work as designed.The fix avoids adding new struct fields and has minimal performance impact (one variable save/restore).