Opened 7 years ago

Closed 23 months ago

#3050 defect closed fixed (fixed)

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 (3)

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

Download all attachments as: .zip

Change History (24)

Changed 7 years ago by ghazel

diff of the LineReceiver patch and unit test

comment:1 Changed 5 years ago by ivank

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

comment:2 Changed 5 years ago by ivank

  • Cc ivank added

comment:3 Changed 4 years ago by ghazel

  • Keywords review added

comment:4 Changed 4 years ago by ghazel

  • Owner teratorn deleted

comment: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

comment: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

comment:7 Changed 4 years ago by ghazel

  • Keywords review added
  • Owner ghazel deleted

Here you go. Please review linereceiver_depth.2.diff

comment: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.

comment: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'])

comment: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?

comment: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).

comment: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.

comment:13 Changed 4 years ago by <automation>

comment:14 Changed 2 years ago by exarkun

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

Changed 23 months ago by graham

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

comment:15 Changed 23 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.

comment:16 Changed 23 months ago by graham

  • Keywords review added

comment:17 Changed 23 months ago by therve

  • Author changed from ghazel to therve, ghazel
  • Branch set to branches/linereceiver-stack-3050

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

comment:18 Changed 23 months ago by therve

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

comment:19 Changed 23 months ago by graham

Nice. Those changes seem entirely reasonable to me.

comment:20 Changed 23 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 :).

comment:21 Changed 23 months ago by therve

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

(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.