Opened 10 years ago

Last modified 5 years ago

#2513 defect new

Infinite recursion in udp write() function

Reported by: p1mrx Owned by: p1mrx
Priority: normal Milestone:
Component: core Keywords:
Cc: therve, jesstess Branch:
Author:

Description (last modified by therve)

I received this traceback from a user of my program. It looks like, in certain cases, twisted can go into an infinite recursive loop inside the UDP write() function. Maybe it should just fail after a certain number of retries?

Traceback (most recent call last):
 File "dtella.py", line 446, in <module>
 File "dtella.py", line 419, in run
 File "twisted\internet\posixbase.pyo", line 220, in run
 File "twisted\internet\posixbase.pyo", line 228, in mainLoop
--- <exception caught here> ---
 File "twisted\internet\base.pyo", line 561, in runUntilCurrent
 File "dtella_core.pyo", line 3294, in cb
 File "dtella_core.pyo", line 258, in sendPacket
 File "twisted\internet\udp.pyo", line 160, in write
 File "twisted\internet\udp.pyo", line 160, in write
 File "twisted\internet\udp.pyo", line 160, in write
 File "twisted\internet\udp.pyo", line 160, in write
 File "twisted\internet\udp.pyo", line 160, in write
 File "twisted\internet\udp.pyo", line 160, in write

(a couple thousand times)

 File "twisted\internet\udp.pyo", line 160, in write
 File "twisted\internet\udp.pyo", line 160, in write
exceptions.RuntimeError: maximum recursion depth exceeded

Attachments (1)

udp-infinite-eintr.patch (1.6 KB) - added by p1mrx 9 years ago.
Proposed patch

Download all attachments as: .zip

Change History (15)

comment:1 Changed 10 years ago by kbrooks

Owner: changed from Glyph to kbrooks
Status: newassigned

confirmed in trunk

comment:2 Changed 10 years ago by kbrooks

Owner: changed from kbrooks to Glyph
Status: assignednew

oops, i did not mean to change the owner

Changed 9 years ago by p1mrx

Attachment: udp-infinite-eintr.patch added

Proposed patch

comment:3 Changed 9 years ago by jknight

"confirmed in trunk" -- confirmed how? What causes this?

comment:4 Changed 9 years ago by Glyph

Owner: changed from Glyph to p1mrx

Patch has no tests. I don't quite understand what's causing this either. Please add some and expound.

comment:5 Changed 9 years ago by therve

Description: modified (diff)

comment:6 Changed 9 years ago by therve

Cc: therve added

We should first thank you for the report :). As glyph spotted earlier, this has few chances to be in if there is no test, and absolutely no chance if we don't understand what's happening.

First, it seems it happens only under win32? Do you have some details on your application? Does it happen in a particular situation (closing the app,..).? Every feedback will be welcome.

comment:7 Changed 9 years ago by p1mrx

This was an intermittent failure that a few people sent me tracebacks for. Most people are running it on Windows, but I can't be certain that this was a Windows-only bug. It did occur on more than one computer though. That's all I know about it.

My program now just catches RuntimeErrors when doing a UDP write, and treats it like packet loss.

comment:8 Changed 9 years ago by Jean-Paul Calderone

Milestone: Core-2.6

Milestones are used for something different.

comment:9 Changed 5 years ago by jesstess

Cc: jesstess added

p1mrx, thanks again for the initial bug report. Do you know if this an issue that still exists under Twisted 11.0? If we can get a reproducer we'd like to investigate it, but if we can't get some more concrete details it may be time to close the ticket.

comment:10 Changed 5 years ago by p1mrx

Sorry, I don't have any information beyond what I've provided. Dtella still contains the "except RuntimeError" hack, but I don't know if it's still required, and it wouldn't be in the users' interest to remove it.

comment:11 in reply to:  10 Changed 5 years ago by Glyph

Replying to p1mrx:

Sorry, I don't have any information beyond what I've provided. Dtella still contains the "except RuntimeError" hack, but I don't know if it's still required, and it wouldn't be in the users' interest to remove it.

Can you try removing that except: just on a test system? I realize that Dtella doesn't stand to benefit much from doing this, but it would really be helpful to Twisted if you could let us know whether we still need to fix this issue, especially since we don't have a simple case that reproduces it.

comment:12 Changed 5 years ago by p1mrx

I can't just reproduce the error at will; the traceback was sent to me by a random user, and I have no idea what conditions triggered it.

comment:13 in reply to:  12 Changed 5 years ago by Glyph

Replying to p1mrx:

I can't just reproduce the error at will; the traceback was sent to me by a random user, and I have no idea what conditions triggered it.

My hypothesis would be "firewall" software like AVG which randomly changes the behavior of sockets (c.f. #3831). Although not necessarily AVG itself, in this case.

I'm sorry we haven't up with a better solution for you yet, but I really appreciate your extreme responsiveness to all the feedback on the ticket. Thank you.

comment:14 Changed 5 years ago by Glyph

Whether or not it is easy to produce repeatedEINTRs from a UDP socket.send, the fact remains that this has the potential for infinite recursion since the socket implementation is totally within its rights to raise that errno at any time if it feels like it. Maybe another process is anxiously trying to kill it; maybe some syscall interception library is messing with it. It doesn't matter, we should handle it better, and the tests for this case are going to use a synthetic socket anyway; producing it from the real implementation is not realistic.

I think a 'pass' might be a more legitimate implementation for this case anyway. Then we'd be dropping a packet knowingly without telling application code, which means that this is also potentially related to #2790. (Perhaps somebody could just re-write udp.Port.write completely, with tests, because the current behavior is accidental and gross and obviously wrong in several cases.)

Note: See TracTickets for help on using tickets.