Opened 5 years ago

Closed 4 years ago

#5992 defect closed fixed (fixed)

twistd dns without any options crashes when it is queried

Reported by: Richard Wall Owned by: Richard Wall
Priority: normal Milestone:
Component: names Keywords:
Cc: Thijs Triemstra Branch: branches/optionless-twistd-dns-5992-2
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

You can start twistd dns without any options but when you query it it crashes.

I think the default behaviour should be --recursive but see also #100 for ideas about alternative dns plugin names.

[richard@zorin ~]$ twistd -n dns --port 1053
2012-09-16 16:05:08+0100 [-] Log opened.
2012-09-16 16:05:08+0100 [-] twistd 12.2.0 (/usr/bin/python 2.7.3) starting up.
2012-09-16 16:05:08+0100 [-] reactor class: twisted.internet.epollreactor.EPollReactor.
2012-09-16 16:05:08+0100 [-] DNSServerFactory starting on 1053
2012-09-16 16:05:08+0100 [-] DNSDatagramProtocol starting on 1053
2012-09-16 16:05:08+0100 [-] Starting protocol <twisted.names.dns.DNSDatagramProtocol object at 0x188af90>
$ dig -p 1053 @127.0.0.1 localhost

; <<>> DiG 9.9.1-P1-RedHat-9.9.1-2.P1.fc17 <<>> -p 1053 @127.0.0.1 localhost
; (1 server found)
;; global options: +cmd
;; connection timed out; no servers could be reached
2012-09-16 16:05:13+0100 [DNSDatagramProtocol (UDP)] Unhandled Error
	Traceback (most recent call last):
	  File "/home/richard/projects/Twisted/trunk/twisted/python/log.py", line 73, in callWithContext
	    return context.call({ILogContext: newCtx}, func, *args, **kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/python/context.py", line 118, in callWithContext
	    return self.currentContext().callWithContext(ctx, func, *args, **kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/python/context.py", line 81, in callWithContext
	    return func(*args,**kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/internet/posixbase.py", line 601, in _doReadOrWrite
	    why = selectable.doRead()
	--- <exception caught here> ---
	  File "/home/richard/projects/Twisted/trunk/twisted/internet/udp.py", line 147, in doRead
	    self.protocol.datagramReceived(data, addr)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/dns.py", line 1838, in datagramReceived
	    self.controller.messageReceived(m, self, addr)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/server.py", line 192, in messageReceived
	    self.handleQuery(message, proto, address)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/server.py", line 136, in handleQuery
	    return self.resolver.query(query).addCallback(
	  File "/home/richard/projects/Twisted/trunk/twisted/names/common.py", line 57, in query
	    return self.typeToMethod[query.type](str(query.name), timeout)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/common.py", line 68, in lookupAddress
	    return self._lookup(name, dns.IN, dns.A, timeout)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/resolve.py", line 45, in _lookup
	    d = self.resolvers[0].query(q, timeout)
	exceptions.IndexError: list index out of range
	
2012-09-16 16:05:18+0100 [DNSDatagramProtocol (UDP)] Unhandled Error
	Traceback (most recent call last):
	  File "/home/richard/projects/Twisted/trunk/twisted/python/log.py", line 73, in callWithContext
	    return context.call({ILogContext: newCtx}, func, *args, **kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/python/context.py", line 118, in callWithContext
	    return self.currentContext().callWithContext(ctx, func, *args, **kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/python/context.py", line 81, in callWithContext
	    return func(*args,**kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/internet/posixbase.py", line 601, in _doReadOrWrite
	    why = selectable.doRead()
	--- <exception caught here> ---
	  File "/home/richard/projects/Twisted/trunk/twisted/internet/udp.py", line 147, in doRead
	    self.protocol.datagramReceived(data, addr)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/dns.py", line 1838, in datagramReceived
	    self.controller.messageReceived(m, self, addr)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/server.py", line 192, in messageReceived
	    self.handleQuery(message, proto, address)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/server.py", line 136, in handleQuery
	    return self.resolver.query(query).addCallback(
	  File "/home/richard/projects/Twisted/trunk/twisted/names/common.py", line 57, in query
	    return self.typeToMethod[query.type](str(query.name), timeout)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/common.py", line 68, in lookupAddress
	    return self._lookup(name, dns.IN, dns.A, timeout)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/resolve.py", line 45, in _lookup
	    d = self.resolvers[0].query(q, timeout)
	exceptions.IndexError: list index out of range
	
2012-09-16 16:05:23+0100 [DNSDatagramProtocol (UDP)] Unhandled Error
	Traceback (most recent call last):
	  File "/home/richard/projects/Twisted/trunk/twisted/python/log.py", line 73, in callWithContext
	    return context.call({ILogContext: newCtx}, func, *args, **kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/python/context.py", line 118, in callWithContext
	    return self.currentContext().callWithContext(ctx, func, *args, **kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/python/context.py", line 81, in callWithContext
	    return func(*args,**kw)
	  File "/home/richard/projects/Twisted/trunk/twisted/internet/posixbase.py", line 601, in _doReadOrWrite
	    why = selectable.doRead()
	--- <exception caught here> ---
	  File "/home/richard/projects/Twisted/trunk/twisted/internet/udp.py", line 147, in doRead
	    self.protocol.datagramReceived(data, addr)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/dns.py", line 1838, in datagramReceived
	    self.controller.messageReceived(m, self, addr)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/server.py", line 192, in messageReceived
	    self.handleQuery(message, proto, address)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/server.py", line 136, in handleQuery
	    return self.resolver.query(query).addCallback(
	  File "/home/richard/projects/Twisted/trunk/twisted/names/common.py", line 57, in query
	    return self.typeToMethod[query.type](str(query.name), timeout)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/common.py", line 68, in lookupAddress
	    return self._lookup(name, dns.IN, dns.A, timeout)
	  File "/home/richard/projects/Twisted/trunk/twisted/names/resolve.py", line 45, in _lookup
	    d = self.resolvers[0].query(q, timeout)
	exceptions.IndexError: list index out of range
	
^C2012-09-16 16:06:33+0100 [-] Received SIGINT, shutting down.
2012-09-16 16:06:33+0100 [-] (UDP Port 1053 Closed)
2012-09-16 16:06:33+0100 [-] Stopping protocol <twisted.names.dns.DNSDatagramProtocol object at 0x188af90>
2012-09-16 16:06:33+0100 [-] (TCP Port 1053 Closed)
2012-09-16 16:06:33+0100 [-] Main loop terminated.
2012-09-16 16:06:33+0100 [-] Server Shut Down.

Change History (14)

comment:1 Changed 5 years ago by Richard Wall

Author: rwall
Branch: branches/optionless-twistd-dns-5992

(In [38203]) Branching to 'optionless-twistd-dns-5992'

comment:2 Changed 5 years ago by Richard Wall

Keywords: review added

Ready for review in log:branches/optionless-twistd-dns-5992

  1. Added a postOptions check that at least one "operating mode" related option has been provided.
  2. Build Results: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/optionless-twistd-dns-5992

comment:3 Changed 5 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to Richard Wall
  1. When running the command I get this output:
bin/twistd: Unknown operating mode. Please provide at least one of the following operating mode options. (--recursive, --resolv-conf, --hosts-file, --secondary, --pyzone or --bindzone)

Why not:

bin/twistd: Unknown operating mode for [command/name here]. Please provide at least one of the following operating mode options: --recursive, --resolv-conf, --hosts-file, --secondary, --pyzone, or --bindzone

What I don't like about this change is the fact we would have to update this list of operating modes whenever something's added or removed, but I guess there's not an easy way around that.

  1. Instead of --{py,bind}zone in the docstring, why not be verbose about it; --pyzone, --bindzone

comment:4 Changed 5 years ago by Jean-Paul Calderone

Thanks for your work on this rwall, thijs.

One thing I notice is that this change doesn't do anything to the actual resolver implementation. It will still be quite possible to construct a Resolver which fails exactly as described in the ticket description.

So perhaps this isn't the complete fix?

What I don't like about this change is the fact we would have to update this list of operating modes whenever something's added or removed, but I guess there's not an easy way around that.

One way around it would be to implement the check in terms of resolvers that are constructed. If no resolvers are constructed (ie, if ca and cl are empty - or at another layer, if ResolverChain is constructed with an empty list of resolvers)), then you know you don't have a valid configuration.

comment:5 in reply to:  3 Changed 5 years ago by Richard Wall

Replying to thijs:

bin/twistd: Unknown operating mode for [command/name here]. Please provide at least one of the following operating mode options: --recursive, --resolv-conf, --hosts-file, --secondary, --pyzone, or --bindzone

I got rid of the brackets and raised #6460 for always including the subCommand name in twistd usage messages.

What I don't like about this change is the fact we would have to update this list of operating modes whenever something's added or removed, but I guess there's not an easy way around that.

I agree. I added the operating mode related options to a list and added a test that those option names are valid options. If someone adds a new mode related option without updating the list, they'll encounter the usage error when they attempt to run twistd dns --new_option.

  1. Instead of --{py,bind}zone in the docstring, why not be verbose about it; --pyzone, --bindzone

I now refer to the _operatingModeOptions list instead.

Thanks for the review.

comment:6 in reply to:  4 Changed 5 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted

Replying to exarkun:

One thing I notice is that this change doesn't do anything to the actual resolver implementation. It will still be quite possible to construct a Resolver which fails exactly as described in the ticket description. So perhaps this isn't the complete fix?

Thanks exarkun, it's a good point.

I've modified ResolverChain to raise a ResolverChainConstructionError if it's passed an empty resolver list.

I could also raise that exception if the resolvers list contains something that doesn't implement IResolver.

What I don't like about this change is the fact we would have to update this list of operating modes whenever something's added or removed, but I guess there's not an easy way around that.

One way around it would be to implement the check in terms of resolvers that are constructed. If no resolvers are constructed (ie, if ca and cl are empty - or at another layer, if ResolverChain is constructed with an empty list of resolvers)), then you know you don't have a valid configuration.

I've used ResolverChainConstructionError raised *via* DNSServerFactory to signal that the resolver isn't configured properly - which allowed me to replace the complicated / fragile series of configuration tests, but I'm not happy that Usage errors are now reported from makeService.

I think it would be better if I could construct a ResolverChain during Options.postOptions (catching the construction exception there) and then pass it directly ie DNSServerFactory(resolver=configresolver?).

Unfortunately DNSServerFactory.init does't have a resolver argument.

And it currently uses the caches argument (or the first cache) to write cache entries. But it seems wrong that you can supply a list of caches but only one of them will get written to.

And it currently uses the clients argument in deciding whether to set the recursion_available bit in responses.

Perhaps ResolverChain should have a cache() method and a recursionAvailable attribute which could then be used by DNSServerFactory.

I'd be interested to hear what you (or thijs) think.

  • Changes: log:branches/optionless-twistd-dns-5992

comment:7 Changed 5 years ago by Tom Prince

I've modified ResolverChain to raise a ResolverChainConstructionError if it's passed an empty resolver list.

Should it fail to be constructed, or simply create a resolver that can't resolve any names. I can't think of a use for it, but that doesn't seem like a reason to avoid it.

I think that might even allow the code in ResolverChain to be simplified slightly. Use defer.fail(dns.DomainError), and then loop over the entire list.

comment:8 Changed 4 years ago by Tom Prince

What if I want a dns server that doesn't know any domain names?

comment:9 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall

As I suggested in my previous two comments, I'm not sure that this is the correct approach to this issue.

Nevertheless

  1. test_operatingModeOptionsExist only tests that options aren't removed with updating _operatingModeOptions. I think the more likely case is actually things being added without the other being added.
  2. In the script, the reported error says "Unknown operating mode". It seems that an operating mode wasn't provide not that it isn't known.

Otherwise, the code looks good.

Please resubmit for review after addressing the suggestions in the previous comments.

comment:10 Changed 4 years ago by Richard Wall

Branch: branches/optionless-twistd-dns-5992branches/optionless-twistd-dns-5992-2

(In [38932]) Branching to 'optionless-twistd-dns-5992-2'

comment:11 in reply to:  9 Changed 4 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted

Ready for another review in log:branches/optionless-twistd-dns-5992-2

Replying to tom.prince:

As I suggested in my previous two comments, I'm not sure that this is the correct approach to this issue.

Yeah I think I agree after all.

I can think of testing situations where I'd want a nameserver to respond NXDOMAIN to all queries.

And I can imagine situations where I'd like to start with an empty ResolverChain and add resolvers dynamically later.

I've made ResolverChain return DomainError when its resolver list is empty.

Also filled in some missing docstrings for ResolverChain.

I've removed all the changes to twisted.names.tap.

-RichardW.

comment:12 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall
  1. Regarding lookupAllRecords, it appears twisted.names.cache defines it explictly.
    • But .query will correctly dispatch to it. (Are there tests for this?)
    • I guess whoever wrote that followed the first bit of reasoning, but not the second.
    • XXX comments needs a link to a ticket.

Please merge after addressing 1.

comment:13 in reply to:  12 Changed 4 years ago by Richard Wall

Status: newassigned

Replying to tom.prince:

  1. Regarding lookupAllRecords, it appears twisted.names.cache defines it explictly.
    • But .query will correctly dispatch to it. (Are there tests for this?)
    • I guess whoever wrote that followed the first bit of reasoning, but not the second.
    • XXX comments needs a link to a ticket.

I raised #6604.

I don't think there are explicit tests for t.n.common.ResolverBase.query dispatching to lookup methods, but it is tested indirectly by the t.n.test.test_names.ServerDNSTestCase.

comment:14 Changed 4 years ago by Richard Wall

Resolution: fixed
Status: assignedclosed

(In [38973]) Merge optionless-twistd-dns-5992-2

Author: rwall Reviewer: tom.prince, thijs, exarkun Fixes: #5992

twisted.names.resolve.ResolverChain now returns a twisted.names.error.DomainError failure if its resolvers list is empty.

Note: See TracTickets for help on using tickets.