Re: [reSIProcate-users] reINVITE - BYE memory leak
I think Byron checked in a fix in May 2008 to the memory leak issue in the Transaction Map (rev 7666) - ie. eventually abandon transactions that the TU (DUM) fails to respond to. But it's not good that we would get into a situation where we are not responding at all to a request. I quickly scanned what you are proposing, and it makes sense to me so far. Note: It seems this would only be an issue for applications that do not respond immediately to a re-invite, or info/message request. I hope to get more time later this week, or next to scan in more detail and commit a fix to SVN.
Thanks for noticing this and posting such a detailed report.
Scott
On Tue, Mar 31, 2009 at 11:12 AM, Gerard Blais
<blais_gerard@xxxxxxxxxxx> wrote:
Hi, this email is a bit long, sorry in advance. During testing I found that the following scenario was causing a memory leak:
UAS NETWORK
| |
|<-------------------------reINVITE-| a
|<------------------------------BYE-| b
|-200 OK--------------------------->|
| |
|-100 TRYING----------------------->| c
| |
a - TransactionMap.cxx: a new TransactionState is created and inserted in TMAP.
b - InviteSession.cxx: InviteSession::dispatchBye is called, 200 OK is sent, InviteSessionHander onTerminated is called, InviteSession is destroyed (mDum.destroy(this)). At this point the application can no longer respond to the reINVITE. The allocated TransactionState stays in the TMAP forever.
c - Sent when Timer Trying 80 has elapsed.
Has anyone seen this already?
I think the stack should send a 487 for the reINVITE when it gets the BYE request:
UAS NETWORK
| |
|<-------------------------reINVITE-|
|<------------------------------BYE-|
|-487 Request Terminated----------->| a
|-200 OK--------------------------->|
| |
a - Send a 487 Request Terminated response for the reINVITE.
I believe the same scenario applies to a BYE received after an INFO request (or any non-INVITE server transaction). The INFO transaction will not be canceled and will remain unanswered. The current code does not seem to keep a state for non-invite server transactions. Therefore when the BYE is receive we cannot tell if such a transaction needs to be canceled.
Another related issue is what happens if a misbehaved remote client sends two (or more) consecutive INFO requests. The last INFO message received always overwrites the current one (stored in mLastNitResponse in InviteSession::dispatchInfo) so that when a response is made the wrong INFO message is responded and the previous transactions are lost.
There is already a state for non-invite client transactions (mNitState), but to fix this a state would be needed for server transactions as well.
I have made changes for the reINVITE leak, see below. For the non-INVITE transaction issue changes are necessary also in InviteSession.cxx. I do not show the change in context for this below but I included a diff output of InviteSession.cxx .hxx at the end. Could someone confirm that this is ok? Thanks.
Gerard
--
Here is the main change shown in context:
---------------------------------------------------------------------------
From resip/dum/InviteSession.cxx, current code (resiprocate version 1.4.1):
---------------------------------------------------------------------------
void
InviteSession::dispatchReceivedUpdateOrReinvite(const SipMessage& msg) {
MethodTypes method = msg.header(h_CSeq).method();
if (method == INVITE || method == UPDATE)
{
// Means that the UAC has sent us a second reINVITE or UPDATE before we
// responded to the first one. Bastard!
SharedPtr<SipMessage> response(new SipMessage);
mDialog.makeResponse(*response, msg, 500);
response->header(h_RetryAfter).value() = Random::getRandom() % 10;
send(response);
}
else
{
dispatchOthers(msg);
}
}
---------------------------------------------------------------------------
Suggested fix:
---------------------------------------------------------------------------
void
InviteSession::dispatchReceivedUpdateOrReinvite(const SipMessage& msg) {
InviteSessionHandler* handler = mDum.mInviteSessionHandler;
std::auto_ptr<SdpContents> sdp = InviteSession::getSdp(msg);
switch (toEvent(msg, sdp.get()))
{
case OnInvite:
case OnInviteReliable:
case OnInviteOffer:
case OnInviteReliableOffer:
case OnUpdate:
case OnUpdateOffer:
{
// Means that the UAC has sent us a second reINVITE or UPDATE before we
// responded to the first one. Bastard!
SharedPtr<SipMessage> response(new SipMessage);
mDialog.makeResponse(*response, msg, 500);
response->header(h_RetryAfter).value() = Random::getRandom() % 10;
send(response);
break;
}
case OnBye:
{
// BYE received after a reINVITE, terminate the reINVITE transaction.
SharedPtr<SipMessage> response(new SipMessage);
mDialog.makeResponse(*response, *mLastRemoteSessionModification, 487); // Request Terminated
handleSessionTimerRequest(*response, *mLastRemoteSessionModification);
send(response);
dispatchBye(msg);
break;
}
default:
dispatchOthers(msg);
break;
}
}
Here is a diff for all the changes:
---------------------------------------------------------------------------
diff output for InviteSession.hxx
---------------------------------------------------------------------------
327a328
> NitState mServerNitState;
---------------------------------------------------------------------------
diff output for InviteSession.cxx
---------------------------------------------------------------------------
59a60
> mServerNitState(NitComplete),
1724,1734c1725,1728
< MethodTypes method = msg.header(h_CSeq).method();
< if (method == INVITE || method == UPDATE)
< {
< // Means that the UAC has sent us a second reINVITE or UPDATE before we
< // responded to the first one. Bastard!
< SharedPtr<SipMessage> response(new SipMessage);
< mDialog.makeResponse(*response, msg, 500);
< response->header(h_RetryAfter).value() = Random::getRandom() % 10;
< send(response);
< }
< else
---
> InviteSessionHandler* handler = mDum.mInviteSessionHandler;
> std::auto_ptr<SdpContents> sdp = InviteSession::getSdp(msg);
>
> switch (toEvent(msg, sdp.get()))
1736c1730,1758
< dispatchOthers(msg);
---
> case OnInvite:
> case OnInviteReliable:
> case OnInviteOffer:
> case OnInviteReliableOffer:
> case OnUpdate:
> case OnUpdateOffer:
> {
> // Means that the UAC has sent us a second reINVITE or UPDATE before we
> // responded to the first one. Bastard!
> SharedPtr<SipMessage> response(new SipMessage);
> mDialog.makeResponse(*response, msg, 500);
> response->header(h_RetryAfter).value() = Random::getRandom() % 10;
> send(response);
> break;
> }
> case OnBye:
> {
> // BYE received after a reINVITE, terminate the reINVITE transaction.
> SharedPtr<SipMessage> response(new SipMessage);
> mDialog.makeResponse(*response, *mLastRemoteSessionModification, 487); // Request Terminated
> handleSessionTimerRequest(*response, *mLastRemoteSessionModification);
> send(response);
>
> dispatchBye(msg);
> break;
> }
> default:
> dispatchOthers(msg);
> break;
1740d1761
<
1987a2009,2018
> // Check for any non-invite server transactions (e.g. INFO)
> // that have not been responded yet and terminate them.
> if (mServerNitState == NitProceeding)
> {
> mLastNitResponse->header(h_StatusLine).statusCode() = 487;
> mLastNitResponse->setContents(0);
> Helper::getResponseCodeReason(487, mLastNitResponse->header(h_StatusLine).reason());
> send(mLastNitResponse);
> mServerNitState = NitComplete;
> }
2019,2021c2050,2064
< InfoLog (<< "Received " << msg.brief());
< mDialog.makeResponse(*mLastNitResponse, msg, 200);
< handler->onInfo(getSessionHandle(), msg);
---
> if (mServerNitState == NitProceeding)
> {
> // Means that the UAC has sent us a second INFO before we
> // responded to the first one.
> SharedPtr<SipMessage> response(new SipMessage);
> mDialog.makeResponse(*response, msg, 500);
> send(response);
> }
> else
> {
> InfoLog (<< "Received " << msg.brief());
> mServerNitState = NitProceeding;
> mDialog.makeResponse(*mLastNitResponse, msg, 200);
> handler->onInfo(getSessionHandle(), msg);
> }
2046a2090,2094
> if (mServerNitState != NitProceeding )
> {
> throw UsageUseException("No transaction to accept", __FILE__, __LINE__);
> }
>
2050c2098,2099
< send(mLastNitResponse);
---
> send(mLastNitResponse);
> mServerNitState = NitComplete;
2091a2141,2146
>
> if (mServerNitState != NitProceeding )
> {
> throw UsageUseException("No transaction to reject", __FILE__, __LINE__);
> }
>
2095a2151
> mServerNitState = NitComplete;
2133,2136c2189,2204
< InfoLog (<< "Received " << msg.brief());
< mDialog.makeResponse(*mLastNitResponse, msg, 200);
< mLastNitResponse->header(h_Contacts).clear();
< handler->onMessage(getSessionHandle(), msg);
---
> if (mServerNitState == NitProceeding)
> {
> // Means that the UAC has sent us a second NIT message before we
> // responded to the first one.
> SharedPtr<SipMessage> response(new SipMessage);
> mDialog.makeResponse(*response, msg, 500);
> send(response);
> }
> else
> {
> InfoLog (<< "Received " << msg.brief());
> mServerNitState = NitProceeding;
> mDialog.makeResponse(*mLastNitResponse, msg, 200);
> mLastNitResponse->header(h_Contacts).clear();
> handler->onMessage(getSessionHandle(), msg);
> }
Communicate, update and plan on Windows Live Messenger.
Get started today.
_______________________________________________
resiprocate-users mailing list
resiprocate-users@xxxxxxxxxxxxxxx
List Archive: http://list.resiprocate.org/archive/resiprocate-users/