Ticket #3050 defect closed fixed

Opened 6 years ago

Last modified 18 months ago

t.p.basic.LineReceiver StackOverflow

Reported by: ghazel Owned by: therve
Priority: normal Milestone:
Component: core Keywords:
Cc: spiv, ivank Branch: branches/linereceiver-stack-3050
(diff, github, buildbot, log)
Author: therve, ghazel Launchpad Bug:

Description

If a LineReceiver-based protocol switches back and forth between line and data modes several times on the same packet, it can hit the maximum stack recursion depth.

Attached is a patch to fix that in LineReceiver, with a unit test that fails with trunk and passes with the patch applied.

Attachments

linereceiver_depth.diff Download (4.3 KB) - added by ghazel 6 years ago.
diff of the LineReceiver patch and unit test
linereceiver_depth.2.diff Download (4.7 KB) - added by ghazel 4 years ago.
Fixed unittest, and added News file
linereceiver_depth.3.diff Download (4.9 KB) - added by graham 19 months ago.
Updated version of the previous patch that accommodates calling setLineMode from outside a rawDataReceived method.

Change History

Changed 6 years ago by ghazel

diff of the LineReceiver patch and unit test

1

Changed 5 years ago by ivank

There is a new attempt to fix this, among other things, in #3803

2

Changed 5 years ago by ivank

  • cc ivank added

3

Changed 4 years ago by ghazel

  • keywords review added

4

Changed 4 years ago by ghazel

  • owner teratorn deleted

5

Changed 4 years ago by rwall

  • keywords review removed
  • owner set to ghazel

ghazel, did you mean to put this up for review. Your patch was added 2 years ago and your comment 15 months ago mentions a new attempt to fix this problem in #3803.

Did you mean to mark this as a duplicate of #3803?

If existing patch _is_ up for review, it'll need a news file ( http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles) and tests fixing.

trial twisted/test/test_protocols.py
...
[ERROR]
Traceback (most recent call last):
  File "/home/richard/Projects/Twisted/trunk/twisted/test/test_protocols.py", line 338, in testStackRecursion
    t = StringIOWithoutClosing()
exceptions.NameError: global name 'StringIOWithoutClosing' is not defined

6

Changed 4 years ago by ghazel

Yes, I did mean to mark this for review. #3803 is not functional or recommended by the author.

I'll fix the test and add a NEWS item.

Changed 4 years ago by ghazel

Fixed unittest, and added News file

7

Changed 4 years ago by ghazel

  • keywords review added
  • owner ghazel deleted

Here you go. Please review linereceiver_depth.2.diff

8

Changed 4 years ago by ivank

  • keywords review removed

I believe this changes the behavior of LineReceiver.setLineMode when it is called not under LineReceiver.dataReceived (i.e. called by some application code when it feels like it). There are several places in Twisted that call setLineMode like this.

I think the setLineMode implementation should look more like this:

        if self._busy:
            self.__buffer += extra
        else:
            self.dataReceived(extra)

(and of course LineReceiver has to set _busy when appropriate; something like this, and also when calling self.lineReceived(...))

                self._busy = True
                try:
                    why = self.rawDataReceived(line)
                finally:
                    self._busy = False

I did not review the rest of the change.

9

Changed 4 years ago by ivank

Here's a new test for the above problem I described:

    def test_setLineModeWhenNotBusyCausesDelivery(self):
        """
        If setLineMode is called *not* under a rawDataReceived or lineReceived
        callback, the data passed to extra= is immediately delivered (if possible).
        """
        class Receiver(basic.LineReceiver):
            MAX_LENGTH = 4
            def lineReceived(self, line):
                #print 'lines was', self.lines
                lines.append(line)

        for setRawFirst in (True, False):
            lines = []
            protocol = Receiver()
            if setRawFirst:
                protocol.setRawMode()
            protocol.setLineMode(extra='test\r\nthis')
            self.assertEqual(lines, ['test'])

10

Changed 4 years ago by ghazel

Does anything actually call setLineMode with extra data outside of a rawDataReceived callback and expect it to work? The comment says:

"If you are calling this from a rawDataReceived callback,
 you can pass in extra unhandled data, and that data will
 be parsed for lines.  Further data received will be sent
 to lineReceived rather than rawDataReceived.


 Do not pass extra data if calling this function from
 within a lineReceived callback."

It even says to *not* call this function from within a lineReceived callback. Could we simply extend the comment to say not to call it from outside a rawDataReceived callback at all?

11

Changed 4 years ago by ivank

I don't think the compatibility policy allows a change to its non-buggy behavior, even though it's a subtle change that doesn't cause any of Twisted's tests to fail (last I checked).

12

Changed 4 years ago by ghazel

Well, I disagree. I'm abandoning this effort. If someone else would like to try to fix it to handle imaginary, undocumented cases that's fine with me.

13

Changed 3 years ago by <automation>

14

Changed 20 months ago by exarkun

#5898 was a duplicate of this. There's also a patch attached there.

Changed 19 months ago by graham

Updated version of the previous patch that accommodates calling setLineMode from outside a rawDataReceived method.

15

Changed 19 months ago by graham

I hit this in production and was amazed to find this patch languishing for 5 years when it would have saved me (and probably a lot of other people) a huge headache. I agree with ghazel that the scenario presented for rejecting his patch is unlikely, but frankly I'm not willing to leave a monkey patch in production code just so it doesn't blow up under load. So here's a patch that might appeal to you better.

I feel like the discussion on this patch to date treated it as an academic issue, but the reality is that I doubt you could even use the memcache driver included with twisted under load without hitting this problem, and it's a bit of a pain to diagnose. It's also a genuine DoS target, since it'd be fairly easy to craft packet patterns that would trigger this behaviour on any protocol that uses linemode switching.

16

Changed 19 months ago by graham

  • keywords review added

17

Changed 19 months ago by therve

  • branch set to branches/linereceiver-stack-3050
  • branch_author changed from ghazel to therve, ghazel

(In [36007]) Branching to 'linereceiver-stack-3050'

18

Changed 19 months ago by therve

I applied latest patch in a branch, with some minor fixes like py3 handling and coding standard.

19

Changed 19 months ago by graham

Nice. Those changes seem entirely reasonable to me.

20

Changed 18 months ago by glyph

  • keywords review removed
  • owner set to therve

These changes look basically good to me. However, the diff covers all the lines in dataReceived, but there are still a bunch of lines - mostly the line-length-exceeded case - that are still un-covered. Can you write a test or two to hit those lines, then commit? It should be pretty straightforward, so no need for a re-review.

Thanks to everybody who kept this ticket alive and got it to the finish line :).

21

Changed 18 months ago by therve

  • status changed from new to closed
  • resolution set to fixed

(In [36239]) Merge linereceiver-stack-3050

Authors: graham, ghazel, therve Reviewers: glyph, ivank Fixes: #3050

Make LineReceiver consume its buffer iteratively to not hit stack errors when it switches back and forth between line and raw modes.

Note: See TracTickets for help on using tickets.