< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

Re: [reSIProcate] CSeq is incorrect in NOTIFY generated by DUM on SUBSCRIBE


Ah - I didn't realize that initial was different than max - that definitely helps.

On Tue, Feb 24, 2009 at 2:15 PM, Adam Roach <adam@xxxxxxxxxxx> wrote:
Hmmm... given that initial CSeq must be in the range of [0,2^31), and max CSeq is in the range of [0,2^32), you have somewhat more than 2 billion transactions before you roll the CSeq space. I know the US Congress has kinda numbed us to the real meaning of "billion," but that would take a long time... about 68 years, if you send one transaction per second.

/a

Scott Godin wrote:
I just did a quick scan of the code - it seems that the only time we check CSeq ordering is for ClientISubscriptions when receiving NOTIFY requests.

Perhaps we should add a CSeq order check to Dialog.cxx, instead of just removing mRemoteCSeq.  We will just need to be careful about being tolerant of CSeq roll overs.

Scott

On Tue, Feb 24, 2009 at 12:06 PM, Robert Sparks <rjsparks@xxxxxxxxxxx <mailto:rjsparks@xxxxxxxxxxx>> wrote:

   We don't check for out-of-order requests?


   On Feb 24, 2009, at 10:59 AM, Adam Roach wrote:

       I think (hope) Jason was talking about removing the "remote
       cseq" field. I'd wait for him to get to a non-virtual keyboard
       -- where he can examine the code himself -- for clarification.

       /a


       Robert Sparks wrote:

           Well, if there was to be a change, it would not be to use
           the CSeq from the other side, but rather to start at
           somewhere random instead of always at 1.

           While I don't really see a need to change to this random
           start (because I don't see any benefit), I'm really
           uncomfortable with the claim you're making here that it
           might do harm.

           (I agree with watching out for unintended consequences in
           general, but for this particular instance, if the change
           broke some peer, then introducing a challenging proxy, for
           instance,
           would break that same peer).

           RjS

           On Feb 24, 2009, at 9:52 AM, Jason Fischl wrote:

               My gut feeling here is to leave it unchanged. These
               kind of changes may have unintended consequences.

               Sent from my iPhone

               On Feb 24, 2009, at 6:55, Adam Roach <adam@xxxxxxxxxxx
               <mailto:adam@xxxxxxxxxxx>> wrote:

                   Volodymyr:

                   Thanks for the suggestion.

                   In terms of the local CSeq: According to RFC 3261,
                   The CSeq number space is unique in each direction.
                   In other words, the CSeq in the NOTIFY is
                   completely unrelated to the CSeq in the SUBSCRIBE.
                   The original code is correct.

                   I agree with you that the remote CSeq is not used,
                   and can likely be removed. I'd be interested to
                   have Scott, Jason, and Derek weigh in on whether
                   this can be safely removed, or if there are some
                   future plans for it.

                   /a

                   Volodymyr Tarasenko wrote:

                       Hi All,

                       As I see dum is incorrectly generates CSeq in
                       NOTIFY in case when SUBSCRIBE's SCeq was not
                       1. NOTIFY's CSeq always starts from 2 in spite
                       of CSeq in SUBSCRIBE was not 1.
                       The fast patch is very simple:

                       --- resip/dum/Dialog.cxx
                       +++ resip/dum/Dialog.cxx
                       @@ -136,7 +136,7 @@
                          }

                          mRemoteCSeq =
                       request.header(h_CSeq).sequence();
                       -      mLocalCSeq = 1;
                       +      mLocalCSeq =
                       request.header(h_CSeq).sequence();

                          DebugLog ( << "************** Created
                       Dialog as UAS **************" );
                          DebugLog ( << "mRemoteNameAddr: " <<
                       mRemoteNameAddr );

                       Also, after code review I've found that
                       mRemoteCSeq is never used and looks like it
                       could be safety removed.

                       Regards,
                       Volodymyr!


                         _______________________________________________
                       resiprocate-devel mailing list
                       resiprocate-devel@xxxxxxxxxxxxxxx
                       <mailto:resiprocate-devel@xxxxxxxxxxxxxxx>

                       https://list.resiprocate.org/mailman/listinfo/resiprocate-devel


                   _______________________________________________
                   resiprocate-devel mailing list
                   resiprocate-devel@xxxxxxxxxxxxxxx
                   <mailto:resiprocate-devel@xxxxxxxxxxxxxxx>

                   https://list.resiprocate.org/mailman/listinfo/resiprocate-devel

               _______________________________________________
               resiprocate-devel mailing list
               resiprocate-devel@xxxxxxxxxxxxxxx
               <mailto:resiprocate-devel@xxxxxxxxxxxxxxx>

               https://list.resiprocate.org/mailman/listinfo/resiprocate-devel




   _______________________________________________
   resiprocate-devel mailing list
   resiprocate-devel@xxxxxxxxxxxxxxx
   <mailto:resiprocate-devel@xxxxxxxxxxxxxxx>