Ticket #3277 defect new

Opened 6 years ago

Last modified 13 months ago

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

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

Change History

1

  Changed 6 years ago by spiv

  • cc spiv added

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

3

  Changed 6 years ago by spiv

  • keywords review, added

4

  Changed 6 years ago by exarkun

  • owner changed from glyph to spiv
  • keywords review, removed
  • cc exarkun added
  • 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.

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.

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)

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.

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

9

  Changed 5 years ago by spiv

  • keywords lineLengthExceeded, review added; lineLengthExceeded removed

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

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!

11

  Changed 4 years ago by jesstess

  • cc jesstess added

12

follow-up: ↓ 15   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

13

  Changed 4 years ago by spiv

  • keywords review added

14

  Changed 4 years ago by spiv

  • owner spiv deleted

15

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

  • cc richard@… added
  • owner set to spiv
  • keywords review removed

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.

16

  Changed 3 years ago by <automation>

  • owner spiv deleted

17

  Changed 13 months ago by therve

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