[reSIProcate] Bug in ClientRegistration::removeBinding

[Byron Campen <bcampen@xxxxxxxxxxxx>]

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

Attachment: smime.p7s
Description: S/MIME cryptographic signature