Opened 6 years ago

Last modified 16 months ago

#3277 defect new

LineReceiver may drop a delimiter (newline) when calling lineLengthExceeded

Reported by: dz Owned by: therve
Priority: normal Milestone:
Component: core Keywords:
Cc: spiv, exarkun, ivank, jesstess, richard@… Branch:
Author: Launchpad Bug:

Description

In LineReceiver.dataReceived, if a line is received that is longer than MAX_LENGTH, it is concatenated with more data without putting the delimiter back:

basic.py:219 line, self.__buffer = self.__buffer.split(self.delimiter, 1)
basic.py:228 exceeded = line + self.__buffer
basic.py:230 return self.lineLengthExceeded(exceeded)

Line 228 should be changed to:

basic.py:228 exceeded = line + self.delimiter + self.__buffer

Attachments (4)

issue-3277.patch (5.1 KB) - added by spiv 6 years ago.
Fix for #3277
lr.patch (8.1 KB) - added by spiv 5 years ago.
Patch for inconsistencies between LineReceiver and LineOnlyReceiver (depends on testscenarios)
lineReceiver-bug-3277.patch (7.6 KB) - added by spiv 5 years ago.
lineReceiver-bug-3277-20100414.patch (10.0 KB) - added by spiv 4 years ago.
Updated patch for review

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by spiv

  • Cc spiv added

comment:2 Changed 6 years ago by spiv

I can confirm this bug report, and I agree with the diagnosis. I've attached a patch for review.

I'm not convinced that the semantics of how lineLengthExceeded is called are entirely sane. It's adequate for a simple "just give up and drop the connection" policy, but not much else. It doesn't seem to make it possible to sanely implement "discard long lines, but keep processing normal-length ones that may follow", for instance. Also, it can return a value that will be used instead of 'why', but this isn't documented. Basically, I'm not sure it's a fully-thought out API.

Also, the tests are excessively messy, and, as this bug highlights, inadequate.

So, the attached patch perhaps isn't quite right, but it's an incremental improvement on the status quo, and fixes this issue.

Changed 6 years ago by spiv

Fix for #3277

comment:3 Changed 6 years ago by spiv

  • Keywords review added

comment:4 Changed 6 years ago by exarkun

  • Cc exarkun added
  • Keywords review removed
  • Owner changed from glyph to spiv
  • The test method should be named test_foo instead of testFoo
  • Can you make the test method docstring more specific in its description of the behavior being tested? Also, it isn't the convention to link to a ticket number in test method descriptions. This can be somewhat interesting information, but it's available by looking at the svn revision log. Having just one test where the information is available in another way doesn't seem like much of a win, unless there's a particular reason you decided to include it?
  • it might be better to use ''.join((...)) instead of using concatenation twice; I'm thinking about the amount of memory that will be needed here, but maybe it's not any worse to concatenate twice than to do it once.

I agree about the weirdness/uselessness of the lineLengthExceeded API.

comment:5 Changed 5 years ago by ivank

The fix and tests for this were copied to #3803

exarkun's 2nd and 3rd points still apply to the implementation in #3803.

comment:6 Changed 5 years ago by ivank

  • Cc ivank added

Changed 5 years ago by spiv

Patch for inconsistencies between LineReceiver and LineOnlyReceiver (depends on testscenarios)

comment:7 Changed 5 years ago by spiv

That patch is a more elaborate version of my original one: it applies that test method, and some others, to both LineReceiver and LineOnlyReceiver. It turns out those two classes behave rather inconsistently. So this patch also addresses those inconsistencies.

Possibly this is too much scope creep, and deserves a new ticket...

Also, this test code depends on https://launchpad.net/testscenarios. I'm not sure what to do about that; I don't really like adding new dependencies, but the benefit it gives for the tests is pretty nice.

comment:8 Changed 5 years ago by exarkun

As far as the testscenarios dependency goes, I think we need to avoid introducing a dependency on it:

  • It seems to be unreleased (ie, the only way to get it is to do a bzr checkout)
  • The launchpad project page claims it is licensed GPLv2

It seems like a mixin defining some test methods and a couple trivial subclasses will accomplish the same goal without any additional effort (I expect it will be less code, actually, since you won't have to define test_suite anymore).

Changed 5 years ago by spiv

comment:9 Changed 5 years ago by spiv

  • Keywords review added

Ok, here's a patch using a mixin rather testscenarios.

comment:10 Changed 5 years ago by therve

  • Keywords LineReceiver MAX_LENGTH lineLengthExceeded review removed
  • -                return self.lineLengthExceeded(line)
    +                return self.lineLengthExceeded(dataSoFar)
    

I think the new behavior is wrong too: if the buffer contains 2 lines, the second being too long, self.lineLengthExceeded will be called with the first line as well.

Everything else looks fine, thanks!

comment:11 Changed 5 years ago by jesstess

  • Cc jesstess added

comment:12 follow-up: Changed 4 years ago by spiv

therve says:
"I think the new behavior is wrong too: if the buffer contains 2 lines, the second being too long, self.lineLengthExceeded will be called with the first line as well."

This is true, and I agree it's odd. It's also what LineReceiver in trunk was trying to do, but it got it wrong and that's not worth preserving. Fixed.

I also made lineLengthExceeded be treated the same as lineReceiver in terms of checking for a 'why' return value or self.transport.disconnecting. The consistency can't hurt, I hope...

Changed 4 years ago by spiv

Updated patch for review

comment:13 Changed 4 years ago by spiv

  • Keywords review added

comment:14 Changed 4 years ago by spiv

  • Owner spiv deleted

comment:15 in reply to: ↑ 12 Changed 4 years ago by rwall

  • Cc richard@… added
  • Keywords review removed
  • Owner set to spiv

Spiv,

This is a general code review - I'm not that familiar with this code and I may have misunderstood parts of it. Here are my notes:

  • Update the docstrings of all changed functions classes to meet coding standard.
  • Update the copyright dates of all changed files
  • New tests are much more thorough, but some of the test methods need docstrings.

source:twisted/protocols/basic.py (LineOnlyReceiver)

  1. LineOnlyReceiver.lineLengthExceeded always returns an Exception so why the added check? I guess it's to allow for overriding in subclasses. It would be good to add documentation about what should be returned from lineLengthExceeded and what effect it will have.
    	147                why = self.lineLengthExceeded(line) 
     	148	                if why: 
     	149	                    return why 
    

source:twisted/protocols/basic.py (LineReceiver)

  1. LineReceiver.dataReceived: The duplication of "disconnecting" check here and later in the method looks wrong. If the intent is to reconcile the differences between LineOnlyReceiver and LineReceiver - I think you should copy the LineOnlyReceiver code - ie move the disconnecting check to the top of the while loop, return None and add a comment to explain why its needed. Hope that makes sense.
    	234	                    why = self.lineLengthExceeded(line) 
     	235	                    if why or self.transport and self.transport.disconnecting: 
     	236	                        return why 
    
                if self.transport.disconnecting:
                    # this is necessary because the transport may be told to lose
                    # the connection by a line within a larger packet, and it is
                    # important to disregard all the lines in that packet following
                    # the one that told it to close.
                    return
    
  2. Also why does LineReceiver.dataReceived check for self.transport and self.transport.disconnecting? Under what circumstances will self.transport be None?
  3. In comparison, IntNStringReceiver.dataReceived doesn't attempt to use the return value of IntNStringReceiver.lengthLimitExceeded, nor does it check for disconnecting.

comment:16 Changed 3 years ago by <automation>

  • Owner spiv deleted

comment:17 Changed 16 months ago by therve

  • Owner set to therve
Note: See TracTickets for help on using tickets.