Opened 5 years ago

Closed 4 years ago

#3998 regression closed fixed (fixed)

Names module DNS getHostByName error on certain sites

Reported by: pythonologist Owned by:
Priority: normal Milestone:
Component: names Keywords:
Cc: smcq, exarkun, jesstess, Scramblejams Branch: branches/names-no-transport-3998
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

With the attached program I get this output:

D:\software\Twisted-trunk\twisted\names\dns.py:1662: DeprecationWarning: Please only pass IPs to write(), not hostnames
  self.transport.write(message.toStr(), address)
result: [Failure instance: Traceback: <type 'exceptions.AttributeError'>: 'NoneType' object has no attribute 'stopListening'
D:\software\Twisted-trunk\twisted\internet\udp.py:126:doRead
D:\software\Twisted-trunk\twisted\names\dns.py:1693:datagramReceived
D:\software\Twisted-trunk\twisted\internet\defer.py:238:callback
D:\software\Twisted-trunk\twisted\internet\defer.py:307:_startRunCallbacks
--- <exception caught here> ---
D:\software\Twisted-trunk\twisted\internet\defer.py:323:_runCallbacks
D:\software\Twisted-trunk\twisted\names\common.py:206:<lambda>
]

Certain domain names appear to return a particular set of DNS records requiring recursion, which does not appear to work properly: one example is www.bbc.co.uk - note that plain bbc.co.uk works fine. Possibly related to #2850.

Attachments (3)

dns_test_2.py (396 bytes) - added by pythonologist 5 years ago.
simple dns test program
gethostbyname-dns.pcap (738 bytes) - added by exarkun 5 years ago.
3998.diff (842 bytes) - added by smcq 5 years ago.
Patch that fixes problem. extractRecord incorrectly attempted to close it's resolver's connection.

Download all attachments as: .zip

Change History (23)

Changed 5 years ago by pythonologist

simple dns test program

comment:1 Changed 5 years ago by exarkun

  • Branch trunk deleted
  • Cc exarkun added
  • Owner changed from exarkun to pythonologist

I cannot reproduce this. Please attach a DNS traffic capture.

comment:2 Changed 5 years ago by exarkun

smcq ran into this error, apparently, and brought it up on IRC. On taking a second look, I was able to reproduce this failure after changing dns_test_2.py to use twistedmatrix.com instead of www.bbc.co.uk. This difference is probably just due to differences in the networks pythonologist and I were running the example on, but I haven't dug into the problem yet so that's just a guess.

I am attaching a pcap file of the dns traffic generated by the script when I run it. Examination (or use, in the worst case) of this should be enough to reproduce the issue in a test case.

Changed 5 years ago by exarkun

comment:3 Changed 5 years ago by exarkun

A guess I might make is that #3342 introduced this bug (that would make this a regression :/).

comment:4 Changed 5 years ago by smcq

  • Owner changed from pythonologist to smcq
  • Status changed from new to assigned

I have tracked this bug to the NS followup query done by extractRecord in common.py.

return r.lookupAddress(str(name)

).addCallback(lambda (ans, auth, add): extractRecord(r, name, ans + auth + add, level - 1)
).addBoth(lambda passthrough: (r.protocol.transport.stopListening(), passthrough)[1])

Simply removing the addBoth (which does appear entirely unnecessary by my reading) will fix the bug.

To duplicate, use a NS record, I was able to successfully duplicate it with saucelabs.com. You may need to set retries to two in order to cause the bug (unsure as to reason, and you may want to dig into that before patching).

With the addBoth removed the following commands successfully run, with it enabled the following commands will cause the server to crash with NoneType not having stopListening.

If you run the following you should be able to reproduce the error against a trivial nameserver implementation:

dig +tries=2 @localhost -p 4053 saucelabs.com

comment:5 Changed 5 years ago by smcq

Sorry, I didn't mean to accept this, however I can submit a patch in the morning for my resolution.

Changed 5 years ago by smcq

Patch that fixes problem. extractRecord incorrectly attempted to close it's resolver's connection.

comment:6 Changed 5 years ago by jesstess

  • Cc smcq jesstess added
  • Status changed from assigned to new

Thanks for the patch smcq!

Can you submit a test case as part of this patch? Also, once you're ready for someone to review a patch, please add 'review' to the keywords property and reassign to nobody so it doesn't get lost in the shuffle.

comment:7 Changed 5 years ago by jesstess

Oh trac thou dost protesteth too much

comment:8 Changed 4 years ago by exarkun

  • Milestone set to Twisted-10.0
  • Type changed from defect to regression

comment:9 Changed 4 years ago by exarkun

#4210 was a duplicate of this.

comment:10 Changed 4 years ago by jml

  • Milestone Twisted-10.1 deleted

Twisted 10.1.0 has been released. Removing this ticket from the 10.1 milestone. If you think it's severe enough to demand a follow-up release, please say so and add this ticket back to the 10.1 milestone.

comment:11 Changed 4 years ago by Scramblejams

  • Cc Scramblejams added

This was a blocker for me, now I have to manually patch all my Twisted installations. And patch them again whenever a security update comes out... Are we just waiting on a test case?

comment:12 Changed 4 years ago by therve

  • Owner changed from smcq to therve

comment:13 Changed 4 years ago by therve

  • Author set to therve
  • Branch set to branches/names-no-transport-3998

(In [29989]) Branching to 'names-no-transport-3998'

comment:14 Changed 4 years ago by therve

  • Keywords review added
  • Owner therve deleted

OK, I did the bare minimum, adding a test failing before and not after. Considering the fix is obvious, I'll it's good enough.

comment:15 Changed 4 years ago by jml

  • Keywords review removed
  • Owner set to therve

Thanks for adding the test therve.

I'm a little confused though as to why the test builds a messages list only to have it returned from query. Wouldn't it be simpler to have query define and return m?

comment:16 Changed 4 years ago by jml

<therve> jml, re your comment, I want it to blow up if another query is made
<therve> I guess it doesn't matter that much if the test is passing
<jml> there are eight tickets marked to review, hard to say if they've got patches or not.
<jml> therve, maybe add a comment saying "Blow up if another query is made", as well as saying why you want to do that.
<therve> jml, sounds good

comment:17 Changed 4 years ago by therve

  • Keywords review added
  • Owner therve deleted

Thanks, I added a comment in r29999.

comment:18 Changed 4 years ago by jml

  • Keywords review removed
  • Owner set to therve

Thanks. Please merge.

comment:19 Changed 4 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(In [30001]) Merge names-no-transport-3998

Author: therve
Reviewer: jml
Fixes: #3998

Fix a regression twisted.names.common.extractRecords where it tried to close a
non-present transport.

comment:20 Changed 3 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.