Opened 8 years ago

Last modified 4 years ago

#2197 defect new

[SIP] wrong assertion in MessagesParser.dataDone

Reported by: antoine Owned by: washort
Priority: highest Milestone:
Component: core Keywords:
Cc: therve, exarkun, awclinford@… Branch:
Author: Launchpad Bug:

Description

Hi,

At the end of the method twisted.protocols.sip:MessagesParser.dataDone(), the number of currently received bytes is compared to the declared content-length. The stated aim is to distinguish "aborted" packets/datagrams (received bytes < content length) from internal errors.
However, the comparison in line 533 does exactly the reverse, and raises a RuntimeError when an "aborted" packet is received.

To fix the bug, "self.length < self.bodyReceived" in that line must be changed to "self.length > self.bodyReceived".

To test for this behaviour, add the following method to MessageParsingTestCase in test_sip.py:

    def testIncomplete(self):
        # test for "aborted" request (body shorter than content-length)
        l = self.l
        self.feedMessage(request4[:-1])
        self.assertEquals(len(l), 2)

Attachments (1)

sip_2197.diff (19.8 KB) - added by therve 8 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by therve

comment:1 Changed 8 years ago by therve

  • Cc therve added
  • Keywords review added
  • Owner glyph deleted
  • Priority changed from normal to highest

Good catch, attached the patch for easier integration. I couldn't prevent me from cleaning the files, I hope it won't mess the review.

comment:2 follow-ups: Changed 8 years ago by exarkun

  • Cc exarkun added
  • Keywords review removed
  • Owner set to washort

sip.py is primarily maintained in Divmod Sine. Patches should probably go there, not here, to avoid having the two diverge in an unmaintainable way.

comment:3 in reply to: ↑ 2 Changed 8 years ago by antoine

Replying to exarkun:

sip.py is primarily maintained in Divmod Sine.

Ok, I'll take the Tangent (haha).
Which in French means to slip away.

comment:4 in reply to: ↑ 2 ; follow-up: Changed 8 years ago by therve

Replying to exarkun:

sip.py is primarily maintained in Divmod Sine.

Does that mean we should use the version in Sine ? Are bugs resolved both in Sine and Twisted version ? The sip.py in Sine has much more dependances (shtoom, epsilon, axiom), so it could be a pain to use.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by exarkun

Replying to therve:

Replying to exarkun:

sip.py is primarily maintained in Divmod Sine.

Does that mean we should use the version in Sine ? Are bugs resolved both in Sine and Twisted version ? The sip.py in Sine has much more dependances (shtoom, epsilon, axiom), so it could be a pain to use.

No one has ever reported a bug or submitted a patch against sip.py before, afaik, so this is the first time it has come up. :P washort has done a huge amount of work on the version of sip.py in Sine, though, and those changes have yet to make their way back into Twisted. They should at some point, but afaik it isn't high on anyone's list of priorities right now. If someone wanted to take that reponsibility upon themselves, I don't think anyone would object. Now, I don't know how compatible Sine's sip.py is with Twisted's sip.py. I expect there are some incompatibilities (even after the various Divmod-specific dependencies are removed, as they should be before anything moves back into Twisted). So moving this code might break things using Twisted's sip.py.

comment:6 in reply to: ↑ 5 Changed 8 years ago by antoine

Replying to exarkun:

Now, I don't know how compatible Sine's sip.py is with Twisted's sip.py. I expect there are some incompatibilities (even after the various Divmod-specific dependencies are removed, as they should be before anything moves back into Twisted). So moving this code might break things using Twisted's sip.py.

I've just tried to backport the message parsing code, it freezes the test suite (yes, freezes) after a few failures. Haven't bothered to try further.

comment:7 Changed 4 years ago by awclin

is it still the case we should merge sip.py fixes back to Sine?

comment:8 Changed 4 years ago by awclin

  • Cc awclinford@… added

comment:9 Changed 4 years ago by exarkun

dash has one (or more?) big branches outstanding to improve twisted.protocols.sip. Someone should try to push those forward, I think. Maybe by finishing them, maybe by pulling pieces of them out into simpler branches (but it's not clear that "implement SIP transactions" can be split into simpler pieces).

Note: See TracTickets for help on using tickets.