Opened 11 years ago

Last modified 6 years ago

#2197 defect new

[SIP] wrong assertion in MessagesParser.dataDone

Reported by: Antoine Pitrou Owned by: washort
Priority: highest Milestone:
Component: core Keywords:
Cc: therve, Jean-Paul Calderone, Adam Branch:
Author:

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 11 years ago.

Download all attachments as: .zip

Change History (10)

Changed 11 years ago by therve

Attachment: sip_2197.diff added

comment:1 Changed 11 years ago by therve

Cc: therve added
Keywords: review added
Owner: Glyph deleted
Priority: normalhighest

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 Changed 11 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone 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 11 years ago by Antoine Pitrou

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 ; Changed 11 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 ; Changed 11 years ago by Jean-Paul Calderone

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 11 years ago by Antoine Pitrou

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 6 years ago by Adam

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

comment:8 Changed 6 years ago by Adam

Cc: Adam added

comment:9 Changed 6 years ago by Jean-Paul Calderone

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.