Opened 10 years ago

Closed 8 years ago

#2194 defect closed fixed (fixed)

small bug in SIP Via header generation

Reported by: antoine 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 10 years ago by antoine

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 10 years ago by therve

AFAIK, 1 is the default value for ttl.

comment:3 Changed 10 years ago by antoine

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 10 years ago by exarkun

  • Owner changed from glyph to washort

comment:5 Changed 8 years ago by washort

  • Author set to washort
  • Branch set to branches/via-parameters-2194

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

comment:6 Changed 8 years ago by washort

  • Keywords review added
  • Owner washort deleted

comment:7 Changed 8 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:8 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to washort
  • Status changed from assigned to new
  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 8 years ago by washort

  • Branch changed from branches/via-parameters-2194 to branches/via-parameters-2194-2

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

comment:10 Changed 8 years ago by washort

  • Keywords review added
  • Owner washort deleted
  • Priority changed from normal to highest

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

comment:11 Changed 8 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:12 Changed 8 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to dash
  • Status changed from assigned to new
  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 8 years ago by washort

  • Keywords review added
  • Owner dash deleted

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

comment:14 Changed 8 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 8 years ago by washort

  • Resolution set to fixed
  • Status changed from new to closed

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