Opened 16 years ago

Closed 16 years ago

#1708 enhancement closed fixed (fixed)

twisted.names shouldn't stop listening when crappy data is sent

Reported by: radix Owned by:
Priority: highest Milestone:
Component: names Keywords:
Cc: Branch:


It should catch the exception and log it.

2006/04/30 19:10 CDT [twisted.names.dns.DNSDatagramProtocol (UDP)] Traceback (most recent call last):
          File "/home/t-names/Twisted/twisted/python/", line 53, in callWithLogger
            return callWithContext({"system": lp}, func, *args, **kw)
          File "/home/t-names/Twisted/twisted/python/", line 38, in callWithContext
            return{ILogContext: newCtx}, func, *args, **kw)
          File "/home/t-names/Twisted/twisted/python/", line 59, in callWithContext
            return self.currentContext().callWithContext(ctx, func, *args, **kw)
          File "/home/t-names/Twisted/twisted/python/", line 37, in callWithContext
            return func(*args,**kw)
        --- <exception caught here> ---
          File "/home/t-names/Twisted/twisted/internet/", line 139, in _doReadOrWrite
            why = getattr(selectable, method)()
          File "/home/t-names/Twisted/twisted/internet/", line 126, in doRead
            self.protocol.datagramReceived(data, addr)
          File "/home/t-names/Twisted/twisted/names/", line 1088, in datagramReceived
          File "/home/t-names/Twisted/twisted/names/", line 1047, in fromStr
          File "/home/t-names/Twisted/twisted/names/", line 992, in decode
            header = readPrecisely(strio, self.headerSize)
          File "/home/t-names/Twisted/twisted/names/", line 153, in readPrecisely
            raise EOFError
2006/04/30 19:10 CDT [twisted.names.dns.DNSDatagramProtocol (UDP)] (Port 53 Closed)
2006/04/30 19:10 CDT [twisted.names.dns.DNSDatagramProtocol (UDP)] Stopping protocol <twisted.names.dns.DNSDatagramProtocol instance at 0xb798580c>

Change History (8)

comment:1 Changed 16 years ago by itamarst

Is this fixed now, or do you want separate t.names fix too?

comment:2 Changed 16 years ago by Jean-Paul Calderone

It should probably be fixed in names too.

comment:3 Changed 16 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Priority: normalhighest

Fixed in source:branches/names-eof-1708 - please review.

comment:4 Changed 16 years ago by Glyph

Owner: set to Stephen Thorne

comment:5 Changed 16 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from Stephen Thorne to Jean-Paul Calderone

Tests pass, the change is a good change to make. I'm a little about there being two error paths, and only one being tested. Please add tests for 'Message.fromStr', testing that object in isolation. Verify that it decodes a DNS packet, and verify that it throws an EOFError error on truncated packets. Knowing what other exceptions can be thrown (instead of using a bare 'except:') would be nice, but not entirely necessery.

The API for Message() looks fugly here. This pattern:

m = Message()

is better expressed by implementing 'fromStr' as a classmethod, making the api:

m = Message.fromStr(mystr)

The function would go from looking like:

    def fromStr(self, str):     
        strio = StringIO.StringIO(str)

to looking like:

    def fromStr(cls, str):     
        strio = StringIO.StringIO(str)
        self = cls()
        return self
    fromStr = classmethod(fromStr)

This is a reasoanbly large change. Only persue this if you think it's worth doing. Create a new ticket maybe?

pyflakes really hates We use exec when importing in order to insert query types into the module namespace. This is yuck, but I don't consider it an issue.

comment:6 Changed 16 years ago by Jean-Paul Calderone

Added a real unit test for Message.fromStr behavior. Didn't add any test for the non-EOFError exception branch, since no code should actually trigger that (and afaik none does).

fromStr predates classmethod, which is why it isn't one. Changing it to one would be incompatible, although I can imagine another kind of descriptor as which it would retain compatibility while also acting as a classmethod. It's mostly internal though so I'm not too worried about it.

The exec's in should really go away, I guess. I'll make a ticket for that.

comment:7 Changed 16 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [17096]) Merge names-eof-1708

Author: exarkun Reviewer: Jerub Fixes #1708

This adds several simple tests for the case of receiving a truncated DNS messages and fixes the code so that it does not raise an exception all the way to the reactor for these cases.

comment:8 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.