Opened 12 years ago

Closed 9 years ago

#2194 defect closed fixed (fixed)

small bug in SIP Via header generation

Reported by: Antoine Pitrou Owned by: washort
Priority: highest Milestone:
Component: core Keywords:
Cc: Branch: branches/via-parameters-2194-2
branch-diff, diff-cov, branch-cov, buildbot
Author: washort


There is a small bug in the Via.toString() method. Perhaps it can only occur in theoretical conditions (I don't know enough about the SIP protocol to know for sure).

However, here it is:

>>> from twisted.protocols import sip
>>> via = sip.Via("localhost", ttl=2)
>>> via.toString()
'SIP/2.0/UDP localhost:5060;ttl=2'
>>> via = sip.Via("localhost", ttl=1)
>>> via.toString()
'SIP/2.0/UDP localhost:5060;ttl'

As you see, if a value is exactly 1, it isn't output. This is because of the test "if value == True" at line 207. Replacing it with "if value is True" fixes the bug.

Change History (15)

comment:1 Changed 12 years ago by Antoine Pitrou

And here is a test to be added in test_sip:ViaTestCase.

    def testOneIsNotTrue(self):
        s = "SIP/2.0/UDP;ttl=1"
        v = sip.parseViaHeader(s)
        self.assertEquals(v.toString().rsplit(';')[1], "ttl=1")

comment:2 Changed 12 years ago by therve

AFAIK, 1 is the default value for ttl.

comment:3 Changed 12 years ago by Antoine Pitrou

The bug can happen with any parameter, I just took "ttl" as an example. Also, RFC 3261 says (section 18.2.2):

If the address is a multicast address, the response SHOULD be sent using the TTL indicated in the "ttl" parameter, or with a TTL of 1 if that parameter is not present.

It does not say there is a default value if the parameter is present but does not specify a value.

comment:4 Changed 12 years ago by Jean-Paul Calderone

Owner: changed from Glyph to washort

comment:5 Changed 10 years ago by washort

Author: washort
Branch: branches/via-parameters-2194

(In [25676]) Branching to 'via-parameters-2194'

comment:6 Changed 10 years ago by washort

Keywords: review added
Owner: washort deleted

comment:7 Changed 10 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:8 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to washort
Status: assignednew
  1. twisted/protocols/
    1. In the class docstring for Via several attributes are documented as Optional or Required. I see that this is information from the spec, but I'm not sure what it means for the attribute. This should be converted into information which can more easily be used by a developer likely to interact with this class (for example, perhaps the optional attributes should be documented as possibly having a None value if the information isn't present).
    2. The types of most of the attributes are not documented but should be.
    3. The enormous try/except in parseViaHeader doesn't seem ideal to me. One specific problem it has is that some of the parsing code raises SIPError with detailed information about how the parsing failed (the wrong version number case), but the except suite squashes everything into a generic SIPError. Another specific problem is the log.err call in the except suite; low-level parsing functions shouldn't log errors, they should raise exceptions indicating how parsing failed. Another problem is that there seems to be no test coverage for the exception handling in this function; it's hard to really decide if the implementation strategy is appropriate without tests. :)
  2. The test suite hangs at testAmoralRPort for me. A parse error shows up in the logs. This may indicate that the Via rport handling has changed incompatibly...
  3. Why was testExtraWhitespace deleted?

comment:9 Changed 9 years ago by washort

Branch: branches/via-parameters-2194branches/via-parameters-2194-2

(In [26100]) Branching to 'via-parameters-2194-2'

comment:10 Changed 9 years ago by washort

Keywords: review added
Owner: washort deleted
Priority: normalhighest

Fixed. The try/except turned out to be useless since nothing catches any exceptions at the call sites for parseViaHeader.

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

Owner: set to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to dash
Status: assignednew
  1. It seems that there's still an incompatibility in the rport change. Old code passing True will get inexpected behavior (rport=True, I think). Can you deprecate this use of the Via.__init__ rport parameter and restore the old behavior for that case?
  2. Before testExtraWhitespace was deleted. Now it's duplicated. Er?
  3. I don't think _Absent.__nonzero__ quite does the trick. Even if the deprecated code isn't being edited, its behavior is potentially being changed. That's fine, if it has tests, but it doesn't appear to have tests. So it probably needs to have tests added. It also looks like some of it is broken by this change - for example, Base._fixupNAT compares the value of rport to True. That case never happens anymore, right? It's now the None case.

comment:13 Changed 9 years ago by washort

Keywords: review added
Owner: dash deleted

OK, new scheme for compatibility added (and other stuff fixed).

comment:14 Changed 9 years ago by Glyph

Keywords: review removed
Owner: set to washort
  1. flakes: Not your fault, but in test_sip, twisted.cred.checkers and twisted.cred.portal are apparently unused. Also, twisted.cred.error is imported in a weird way.
  2. getWarningMethod() should really be deprecated. It's no longer necessary. just use warnings.warn.
  3. BAD_REQUEST is apparently never used. Neither is a "400" used anywhere in the diff. I guess it should just be deleted?
  4. I think it would be clearer to call the getter _getrport, rather than the ambiguous rport. (Also the setter should be _setrport, since I find the case difference is a bit confusing.)
    1. also please two blank lines after the rport method, when you change its name.
  5. Given that rportRequested is a boolean, the "== True" crud is just noise. Delete it.
  6. Docstrings should use L{} more often. For example, instead of using a sentence fragment, Via's docstring could begin with: "A L{Via} is a SIP 'Via' header...". parseViaHeader should say "Parse a 'via' header, returning a L{Via}." Also it could say "@return" and "@rtype".

This is all pretty cosmetic, so please address and then merge.

comment:15 Changed 9 years ago by washort

Resolution: fixed
Status: newclosed

(In [26236]) Merge via-parameters-2194-2. Fix a defect in SIP Via header parameter generation.

Author: washort Reviewers: exarkun, glyph Closes #2194.

Note: See TracTickets for help on using tickets.