Fix two crashes in the default DNS forward/cache path - #21784
Open
Pushpenderrathore wants to merge 1 commit into
Open
Fix two crashes in the default DNS forward/cache path#21784Pushpenderrathore wants to merge 1 commit into
Pushpenderrathore wants to merge 1 commit into
Conversation
Rex::Proto::DNS::Server#default_dispatch_request crashed with a NoMethodError the moment it had to finalize an empty response, which happens on any query that misses the cache and comes back from the resolver with no answers - an out-of-scope query being forwarded, for instance, or any query with a genuinely empty result. The exception was uncaught, so it killed the listener thread and stopped the whole server. Two mistakes, both on code Packet.encode_drb documents as returning a Dnsruby::Message: - req.header.rCode= does not exist on Dnsruby::Header; the real setter is rcode= (lowercase). The class does define an rCode-cased method elsewhere as a getter alias, which is presumably what led to the wrong casing here. - req.data does not exist on Dnsruby::Message either; the real serializer is #encode. Packet.encode_raw, a few lines above in the same file, already handles this correctly by checking respond_to?(:data) for a legacy Net::DNS::Packet and falling back to #encode otherwise - default_dispatch_request just used the wrong branch of that same distinction. No existing spec covered this path, so both bugs shipped and stayed live. Reproduced against a real client. Fixed and added coverage for the empty answer case that crashed.
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.
Description
Rex::Proto::DNS::Server#default_dispatch_requestcrashes the moment it has to finalize an empty response. That happens on any query that misses the cache and gets forwarded to the resolver with no answer coming back, which in practice includes any out-of-scope query being forwarded through unchanged, such as the PTR reverse lookup a real client sends before its actual query. The exception is uncaught, so it kills the listener thread and stops the whole server, silently, on the very first such query.Two mistakes in the same two lines, both against
Dnsruby::Message, the typePacket.encode_drbdocumentsreqas being:req.header.rCode=does not exist onDnsruby::Header. The real setter isrcode=(lowercase). The class does expose anrCode-cased reader alias elsewhere in its API, which is presumably where the casing came from.req.datadoes not exist onDnsruby::Message. The real serializer is#encode.Packet.encode_raw, a few lines above in this same file, already gets this right:default_dispatch_requestjust used the wrong branch of the samedata/encodedistinction this file already knows how to make.No spec covered this path, so both shipped and stayed live.
Live reproduction
I hit this while building an IPv6 DNS-takeover module. Once the crash was fixed I could see it happening: with only the first fix applied, the second line raised immediately on the next real query.
With both fixed, the same real client's query is handled and forwarded correctly, and the server survives.
Testing
Added
spec/lib/rex/proto/dns/server_spec.rb, covering the path that crashed: a forwarded query whose response carries no answers. Confirmed the new spec fails against the pre-fix code with the exact errors above, and passes with the fix.Breaking Changes
None. Both lines were unreachable without raising; nothing depended on the old behavior.