Re: [reSIProcate] Bug in ClientRegistration::removeBinding

["Scott Godin" <slgodin@xxxxxxxxxxxx>]

I think either option is feasible, but your first option (storing default expires and using myContacts) will fix more issues.  Ie.  If the user calls removeMyBindings or removeAll then we lose our default expires and contact, so calling addBinding later will not work – this makes setting the stopRegisteringWhenDone flag to false, useless.

 

Scott

 

From: resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Byron Campen
Sent: Tuesday, July 03, 2007 5:22 PM
To: resiprocate-devel
Cc: Jason Fischl
Subject: [reSIProcate] Bug in ClientRegistration::removeBinding

 

          Here's the code in question:

 

*snip*

void

ClientRegistration::removeBinding(const NameAddr& contact)

{

   if (mState == Removing)

   {

      WarningLog (<< "Already removing a binding");

      throw UsageUseException("Can't remove binding when already removing registration bindings", __FILE__,__LINE__);

   }

 

   SharedPtr<SipMessage> next = tryModification(Removing);

   for (NameAddrs::iterator i=mMyContacts.begin(); i != mMyContacts.end(); i++)

   {

      if (i->uri() == contact.uri())

      {

         mMyContacts.erase(i);

 

         next->header(h_Contacts) = mMyContacts;

         next->header(h_Expires).value() = 0;

         next->header(h_CSeq).sequence()++;

 

         if (mQueuedState == None)

         {

            send(next);

         }

 

*snip*

 

          This appears to unregister all bindings _except_ contact, and remove contact from the set of contacts that we're maintaining. Surely this is not what this function is supposed to do? Fixing this does not look as simple as setting next->header(h_Contacts)=contact, because next is an alias of mLastRequest, and when it comes time for a refresh, we seem to just increment the CSeq and kick mLastRequest out on the wire.

 

*snip*

void

ClientRegistration::internalRequestRefresh(UInt32 expires)

{

   InfoLog (<< "requesting refresh of " << *this);

  

   assert (mState == Registered);

   mState = Refreshing;

   mLastRequest->header(h_CSeq).sequence()++;

   if(expires > 0)

   {

      mLastRequest->header(h_Expires).value() = expires;

   }

 

   send(mLastRequest);

}

*snip*

 

          If we just modified the code in removeBinding(), this would lead to refreshes having just the removed contact and an Expires of 0, which isn't right either. Furthermore, it appears to me that our default expires value is _gone_ at this point (it only existed in mLastRequest, and we overwrote it with 0 when we called removeBinding()).

 

          It seems to me that storing our default Expires value in mLastRequest is wrong, and we need another member. Additionally, it seems like a bad idea to rely on contacts that are left sitting around in mLastRequest (we really should be using mMyContacts every time, right?).

 

          Our alternative is to forge the unregister request from whole cloth, so we don't end up stomping on the stuff in mLastRequest (we would have to remember to remove the contact from mLastRequest->header(h_Contacts), lest it be re-registered on the next refresh).

 

          Opinions?

 

Best regards,

Byron Campen