| < Previous by Date | Date Index | Next by Date > |
| Thread Index | Next in Thread > |
|
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. |