t.p.basic.LineReceiver StackOverflow

[ghazel]

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.

[ghazel]

diff of the LineReceiver patch and unit test

[ivank]

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

[rwall]

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

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

[ghazel]

Fixed unittest, and added News file

[ghazel]

Here you go. Please review linereceiver_depth.2.diff

[ivank]

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.

[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'])

[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?

[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).

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

[exarkun]

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

[graham]

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

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

[therve]

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

[therve]

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

[graham]

Nice. Those changes seem entirely reasonable to me.

[glyph]

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

[therve]

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