[reSIProcate] Bug in ClientRegistration::removeBinding

[Scott Godin]

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 at list.resiprocate.org
[mailto:resiprocate-devel-bounces at list.resiprocate.org] 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

 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20070705/165a0857/attachment.htm>