[reSIProcate] Bug in ClientRegistration::removeBinding

[Byron Campen]

	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/20070703/db953f01/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2423 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20070703/db953f01/attachment.bin>