[asterisk-dev] zero is a legitimate value for SIP CSeq numbers, right ?

[Luigi Rizzo]

On Thu, Oct 12, 2006 at 11:21:52AM -0500, Kevin P. Fleming wrote:
> ----- Luigi Rizzo <rizzo at icir.org> wrote:
> > according to the RFC and a bit of googling on mailing lists,
> > zero (0) seems to be a legitimate value for SIP CSeq numbers,
> > so unless there are objections or other reasons i would like
> > to remove the special handling of 0 for ocseq/icseq in chan_sip.c
> > 
> > Otherwise, if we need to keep it, i would like at least to
> > document why.
> 
> Unless you know of a specific interoperability or performance issue associated with this change, I'm hesitant to see it get changed.

the issue is code obfuscation, which means we don't know
what the current handling of 0 does, which means trouble :)

I see two different issues (refer to SVN44910 of chan_sip.c):

  + ocseq is supposed to be initialized to INITIAL_CSEQ (non-zero in asterisk),
    so ocseq = 0 is an error condition (if i understand well).

    Unfortunately the code near line 14101 does not detect it
    as an error, just accepts incoming packets irrespective
    of the seqno they carry. I think this is a bug.

    Since we generally don't checks that variables are correctly
    initialized (we try to guarantee it when we create the
    data structure), i think we should just remove this zero check.

  + icseq is initialized with whatever CSeq we get in the first msg.
    Apparently, in chan_sip, icseq == 0 is used as "not initialized" marker
    (this is in the check for duplicate packets near line 14134).
    However CSeq == 0 is a legal initial value for CSeq
    (coming from a third party we have no control over it),
    and if such a message comes in, we would process retransmissions
    of CSeq 0 multiple times because we do not detect it as a dup.
    Unlikely but possibly a bug as well.

    Other than using an external flag, i think a better
    choice for "not initialized" is 0xffffffff, which is not
    a valid initial CSeq for a dialogue.

Anyways the special handling is only in the two places above,
and my inclination would be to fix them as indicated above.

more convincing ?

	cheers
	luigi