[SIP] wrong assertion in MessagesParser.dataDone

[antoine]

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)

[therve]

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.

[exarkun]

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.

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

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

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

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

[awclin]

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

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