Re: [reSIProcate] ClientPublication bug with 401/auth and queued changes

[Scott Godin <sgodin@xxxxxxxxxxxxxxx>]

Looks good to me.  Thanks Aron!

On Wed, Sep 22, 2010 at 7:54 PM, <aron_rosenberg@xxxxxxxxxxxx> wrote:

I found a bug in ClientPublication which is causing it to fail a transaction if an update request is made while an existing request is on the wire that will result in a 401. A very simple scenario that can cause this is if the server has nonce re-use disabled (or lifetime of 0) and an updated PUBLISH is requested by the user while a current PUBLISH is on the wire.

Scenario

Client Server
PUBLISH 1 ---------------------->
<----------------401----PUB1-----

PUBLISH 2 with auth -------->
<----------------200 ok------------

PUBLISH 3 with auth ------->
PUBLISH 4 requested (gets queued in ClientPublication)
<--------------401--PUB3-------


When the 401 for PUB3 arrives, the stack doesn't do anything with it and dum calls ClientPublicationHandler::onFailure with a 401

I believe the bug is that ClientPublication.cxx is incrementing the CSeq number on the shared_ptr request even in the case where it is queuing the new PUBLISH rather than sending it right away. This is causing the CSeq test in DialogSet::handledByAuthOrRedirect(const SipMessage& msg) to fail when the 401 arrives becuase the CSeq is now greater in the stored / shared_ptr request than what just came on the wire.

You can force this using the new resip/dum/test/basicPublication.cxx if you define PUB_REALLY_FAST and have a server which doesn't allow nonce re-use.

The following patch fixes the issue and I will commit it if nobody says otherwise. All it basically does is only increment the CSeq number when the request is actually sent to the wire, rather than when a request could be queued up.


Index: resip/dum/ClientPublication.cxx
===================================================================
--- resip/dum/ClientPublication.cxx (revision 8815)
+++ resip/dum/ClientPublication.cxx (working copy)
@@ -45,7 +45,6 @@
ClientPublication::end()
{
InfoLog (<< "End client publication to " << mPublish->header(h_RequestLine).uri());
- mPublish->header(h_CSeq).sequence()++;
mPublish->header(h_Expires).value() = 0;
send(mPublish);
}
@@ -230,7 +229,6 @@
{
expiration = mPublish->header(h_Expires).value();
}
- mPublish->header(h_CSeq).sequence()++;
send(mPublish);
}

@@ -283,7 +281,6 @@
}
}

- mPublish->header(h_CSeq).sequence()++;
mPublish->setContents(mDocument);
send(mPublish);
}
@@ -328,6 +325,7 @@
}
else
{
+ request->header(h_CSeq).sequence()++;
mDum.send(request);
mWaitingForResponse = true;
mPendingPublish = false;


Aron Rosenberg
Logitech Inc, (SightSpeed Group)

_______________________________________________
resiprocate-devel mailing list
resiprocate-devel@xxxxxxxxxxxxxxx
https://list.resiprocate.org/mailman/listinfo/resiprocate-devel