< Previous by Date Date Index Next by Date >
  Thread Index Next in Thread >

[reSIProcate] Memory leak in case of some badly formed headers in sip message


Hi,

As I see there is a memory leak in case reciprocate receives a sip message with 
some badly formed headers.

For example, if From is something like:
From: "Test" <sip:12345678 @my.test.com >
                                      ^^^^
an exception is throwing in TransactionState::process() and this will lead to 
memory leak: sip  message which is retrieved from Fifo will not be deleted.

I've attached simplest patch for this, but I think it should checked more 
carefully... 

Regards,
Volodymyr!


      
diff -Naur resip/stack/TransactionState.cxx resip/stack/TransactionState.cxx
--- resip/stack/TransactionState.cxx
+++ resip/stack/TransactionState.cxx
@@ -140,6 +140,199 @@
    mState = Bogus;
 }
 
+bool
+TransactionState::processSipMessageAsNew(SipMessage* sip, TransactionController& controller, const Data& tid)
+{
+   StackLog (<< "No matching transaction for " << sip->brief());
+   TransactionUser* tu = 0;      
+   if (sip->isExternal())
+   {
+      if (controller.mTuSelector.haveTransactionUsers() && sip->isRequest())
+      {
+         tu = controller.mTuSelector.selectTransactionUser(*sip);
+         if (!tu)
+         {
+            //InfoLog (<< "Didn't find a TU for " << sip->brief());
+            // !bwc! We really should do something other than a 500 here.
+            // If none of the TUs liked the request because of the Request-
+            // Uri scheme, we should be returning a 416, for example.
+            InfoLog( << "No TU found for message: " << sip->brief());               
+            SipMessage* noMatch = Helper::makeResponse(*sip, 500);
+            Tuple target(sip->getSource());
+
+            controller.mTransportSelector.transmit(noMatch, target);
+            delete noMatch;
+            return false;
+         }
+         else
+         {
+            //InfoLog (<< "Found TU for " << sip->brief());
+         }
+      }
+   }
+   else
+   {
+      tu = sip->getTransactionUser();
+      if (!tu)
+      {
+         //InfoLog (<< "No TU associated with " << sip->brief());
+      }
+   }
+               
+   if (sip->isRequest())
+   {
+      // create a new state object and insert in the TransactionMap
+               
+      if (sip->isExternal()) // new sip msg from transport
+      {
+         if (sip->method() == INVITE)
+         {
+            // !rk! This might be needlessly created.  Design issue.
+            TransactionState* state = new TransactionState(controller, ServerInvite, Trying, tid, tu);
+            state->mMsgToRetransmit = state->make100(sip);
+            state->mResponseTarget = sip->getSource(); // UACs source address
+            // since we don't want to reply to the source port if rport present 
+            state->mResponseTarget.setPort(Helper::getPortForReply(*sip));
+            state->mIsReliable = isReliable(state->mResponseTarget.getType());
+            state->add(tid);
+               
+            if (Timer::T100 == 0)
+            {
+               state->sendToWire(state->mMsgToRetransmit); // will get deleted when this is deleted
+               state->mState = Proceeding;
+            }
+            else
+            {
+               //StackLog(<<" adding T100 timer (INV)");
+               controller.mTimers.add(Timer::TimerTrying, tid, Timer::T100);
+            }
+         }
+         else if (sip->method() == CANCEL)
+         {
+            TransactionState* matchingInvite = 
+               controller.mServerTransactionMap.find(sip->getTransactionId());
+            if (matchingInvite == 0)
+            {
+               InfoLog (<< "No matching INVITE for incoming (from wire) CANCEL to uas");
+               //was TransactionState::sendToTU(tu, controller, Helper::makeResponse(*sip, 481));
+               SipMessage* response = Helper::makeResponse(*sip, 481);
+               Tuple target(sip->getSource());
+               controller.mTransportSelector.transmit(response, target);
+               
+               delete response;
+               return false;
+            }
+            else
+            {
+               assert(matchingInvite);
+               TransactionState* state = TransactionState::makeCancelTransaction(matchingInvite, ServerNonInvite, tid);
+               state->startServerNonInviteTimerTrying(*sip,tid);
+            }
+         }
+         else if (sip->method() != ACK)
+         {
+            TransactionState* state = new TransactionState(controller, ServerNonInvite,Trying, tid, tu);
+            state->mResponseTarget = sip->getSource();
+            // since we don't want to reply to the source port if rport present 
+            state->mResponseTarget.setPort(Helper::getPortForReply(*sip));
+            state->add(tid);
+            state->mIsReliable = isReliable(state->mResponseTarget.getType());
+            state->startServerNonInviteTimerTrying(*sip,tid);
+         }
+            
+         // Incoming ACK just gets passed to the TU
+         //StackLog(<< "Adding incoming message to TU fifo " << tid);
+         TransactionState::sendToTU(tu, controller, sip);
+      }
+      else // new sip msg from the TU
+      {
+         if (sip->method() == INVITE)
+         {
+            TransactionState* state = new TransactionState(controller, ClientInvite, Calling, tid, tu);
+            state->add(state->mId);
+            state->processClientInvite(sip);
+         }
+         else if (sip->method() == ACK)
+         {
+            //TransactionState* state = new TransactionState(controller, Stateless, Calling, Data(StatelessIdCounter++));
+            TransactionState* state = new TransactionState(controller, Stateless, Calling, tid, tu);
+            state->add(state->mId);
+            state->mController.mTimers.add(Timer::TimerStateless, state->mId, Timer::TS );
+            state->processStateless(sip);
+         }
+         else if (sip->method() == CANCEL)
+         {
+            TransactionState* matchingInvite = controller.mClientTransactionMap.find(sip->getTransactionId());
+               
+            if (matchingInvite == 0)
+            {
+               InfoLog (<< "No matching INVITE for incoming (from TU) CANCEL to uac");
+               TransactionState::sendToTU(tu, controller, Helper::makeResponse(*sip,481));
+               //delete sip;
+               return false;
+            }
+            else if (matchingInvite->mState == Calling) // CANCEL before 1xx received
+            {
+               WarningLog(<< "You can't CANCEL a request until a provisional has been received");
+               StackLog (<< *matchingInvite);
+               StackLog (<< *sip);
+
+               matchingInvite->mIsAbandoned = true;
+               //delete sip;
+               return false;
+            }
+            else if (matchingInvite->mState == Completed)
+            {
+               // A final response was already seen for this INVITE transaction
+               matchingInvite->sendToTU(Helper::makeResponse(*sip, 200));
+               //delete sip;
+               return false;
+            }
+            else
+            {
+               handleInternalCancel(sip, *matchingInvite);
+            }
+         }
+         else 
+         {
+            TransactionState* state = new TransactionState(controller, ClientNonInvite, 
+            Trying, tid, tu);
+            state->add(tid);
+            state->processClientNonInvite(sip);
+         }
+      }
+   }
+   else if (sip->isResponse()) // stray response
+   {
+      if (controller.mDiscardStrayResponses)
+      {
+         InfoLog (<< "discarding stray response: " << sip->brief());
+         //delete message;
+         return false;
+      }
+      else
+      {
+         StackLog (<< "forwarding stateless response: " << sip->brief());
+         TransactionState* state = 
+            new TransactionState(controller, Stateless, Calling, 
+            Data(StatelessIdCounter++), tu);
+         state->add(state->mId);
+         state->mController.mTimers.add(Timer::TimerStateless, state->mId, Timer::TS );
+         state->processStateless(sip);
+      }
+   }
+   else // wasn't a request or a response
+   {
+      ErrLog (<< "Got a SipMessage that was neither a request nor response!" 
+      << sip->brief());
+      //delete sip;
+
+      return false;
+   }
+
+   return true;
+}
+
 void
 TransactionState::process(TransactionController& controller)
 {
@@ -386,184 +579,27 @@
    }
    else if (sip)  // new transaction
    {
-      StackLog (<< "No matching transaction for " << sip->brief());
-      TransactionUser* tu = 0;      
-      if (sip->isExternal())
-      {
-         if (controller.mTuSelector.haveTransactionUsers() && sip->isRequest())
-         {
-            tu = controller.mTuSelector.selectTransactionUser(*sip);
-            if (!tu)
-            {
-               //InfoLog (<< "Didn't find a TU for " << sip->brief());
-               // !bwc! We really should do something other than a 500 here.
-               // If none of the TUs liked the request because of the Request-
-               // Uri scheme, we should be returning a 416, for example.
-               InfoLog( << "No TU found for message: " << sip->brief());               
-               SipMessage* noMatch = Helper::makeResponse(*sip, 500);
-               Tuple target(sip->getSource());
-               delete sip;
-               controller.mTransportSelector.transmit(noMatch, target);
-               delete noMatch;
-               return;
-            }
-            else
-            {
-               //InfoLog (<< "Found TU for " << sip->brief());
-            }
-         }
-      }
-      else
-      {
-         tu = sip->getTransactionUser();
-         if (!tu)
-         {
-            //InfoLog (<< "No TU associated with " << sip->brief());
-         }
-      }
-               
-      if (sip->isRequest())
-      {
-         // create a new state object and insert in the TransactionMap
-               
-         if (sip->isExternal()) // new sip msg from transport
-         {
-            if (sip->method() == INVITE)
-            {
-               // !rk! This might be needlessly created.  Design issue.
-               TransactionState* state = new TransactionState(controller, ServerInvite, Trying, tid, tu);
-               state->mMsgToRetransmit = state->make100(sip);
-               state->mResponseTarget = sip->getSource(); // UACs source address
-               // since we don't want to reply to the source port if rport present 
-               state->mResponseTarget.setPort(Helper::getPortForReply(*sip));
-               state->mIsReliable = isReliable(state->mResponseTarget.getType());
-               state->add(tid);
-               
-               if (Timer::T100 == 0)
-               {
-                  state->sendToWire(state->mMsgToRetransmit); // will get deleted when this is deleted
-                  state->mState = Proceeding;
-               }
-               else
-               {
-                  //StackLog(<<" adding T100 timer (INV)");
-                  controller.mTimers.add(Timer::TimerTrying, tid, Timer::T100);
-               }
-            }
-            else if (sip->method() == CANCEL)
-            {
-               TransactionState* matchingInvite = 
-                  controller.mServerTransactionMap.find(sip->getTransactionId());
-               if (matchingInvite == 0)
-               {
-                  InfoLog (<< "No matching INVITE for incoming (from wire) CANCEL to uas");
-                  //was TransactionState::sendToTU(tu, controller, Helper::makeResponse(*sip, 481));
-                  SipMessage* response = Helper::makeResponse(*sip, 481);
-                  Tuple target(sip->getSource());
-                  controller.mTransportSelector.transmit(response, target);
-                  delete sip;
-                  delete response;
-                  return;
-               }
-               else
-               {
-                  assert(matchingInvite);
-                  state = TransactionState::makeCancelTransaction(matchingInvite, ServerNonInvite, tid);
-                  state->startServerNonInviteTimerTrying(*sip,tid);
-               }
-            }
-            else if (sip->method() != ACK)
-            {
-               TransactionState* state = new TransactionState(controller, ServerNonInvite,Trying, tid, tu);
-               state->mResponseTarget = sip->getSource();
-               // since we don't want to reply to the source port if rport present 
-               state->mResponseTarget.setPort(Helper::getPortForReply(*sip));
-               state->add(tid);
-               state->mIsReliable = isReliable(state->mResponseTarget.getType());
-               state->startServerNonInviteTimerTrying(*sip,tid);
-            }
-            
-            // Incoming ACK just gets passed to the TU
-            //StackLog(<< "Adding incoming message to TU fifo " << tid);
-            TransactionState::sendToTU(tu, controller, sip);
-         }
-         else // new sip msg from the TU
-         {
-            if (sip->method() == INVITE)
-            {
-               TransactionState* state = new TransactionState(controller, ClientInvite, Calling, tid, tu);
-               state->add(state->mId);
-               state->processClientInvite(sip);
-            }
-            else if (sip->method() == ACK)
-            {
-               //TransactionState* state = new TransactionState(controller, Stateless, Calling, Data(StatelessIdCounter++));
-               TransactionState* state = new TransactionState(controller, Stateless, Calling, tid, tu);
-               state->add(state->mId);
-               state->mController.mTimers.add(Timer::TimerStateless, state->mId, Timer::TS );
-               state->processStateless(sip);
-            }
-            else if (sip->method() == CANCEL)
-            {
-               TransactionState* matchingInvite = controller.mClientTransactionMap.find(sip->getTransactionId());
-               
-               if (matchingInvite == 0)
-               {
-                  InfoLog (<< "No matching INVITE for incoming (from TU) CANCEL to uac");
-                  TransactionState::sendToTU(tu, controller, Helper::makeResponse(*sip,481));
-                  delete sip;
-               }
-               else if (matchingInvite->mState == Calling) // CANCEL before 1xx received
-               {
-                  WarningLog(<< "You can't CANCEL a request until a provisional has been received");
-                  StackLog (<< *matchingInvite);
-                  StackLog (<< *sip);
+      try{
+         bool processed = processSipMessageAsNew(sip, controller, tid);
 
-                  matchingInvite->mIsAbandoned = true;
-                  delete sip;
-               }
-               else if (matchingInvite->mState == Completed)
-               {
-                  // A final response was already seen for this INVITE transaction
-                  matchingInvite->sendToTU(Helper::makeResponse(*sip, 200));
-                  delete sip;
-               }
-               else
-               {
-                  handleInternalCancel(sip, *matchingInvite);
-               }
-            }
-            else 
-            {
-               TransactionState* state = new TransactionState(controller, ClientNonInvite, 
-                                                              Trying, tid, tu);
-               state->add(tid);
-               state->processClientNonInvite(sip);
-            }
-         }
+         if (!processed)
+            delete sip;
       }
-      else if (sip->isResponse()) // stray response
+      catch(resip::ParseException& e)
       {
-         if (controller.mDiscardStrayResponses)
-         {
-            InfoLog (<< "discarding stray response: " << sip->brief());
-            delete message;
-         }
-         else
+         StackLog ( << "Got badly formatted sip message, error: " 
+                    << e.what());      
+         if(sip->isRequest() && sip->method()!=ACK)
          {
-            StackLog (<< "forwarding stateless response: " << sip->brief());
-            TransactionState* state = 
-               new TransactionState(controller, Stateless, Calling, 
-                                    Data(StatelessIdCounter++), tu);
-            state->add(state->mId);
-            state->mController.mTimers.add(Timer::TimerStateless, state->mId, Timer::TS );
-            state->processStateless(sip);
+            handleBadRequest(*sip, controller);
          }
+         
+         delete sip;
       }
-      else // wasn't a request or a response
+      catch(std::exception& err)
       {
-         ErrLog (<< "Got a SipMessage that was neither a request nor response!" 
-                  << sip->brief());
+         StackLog ( << "Got error: " << err.what());
+
          delete sip;
       }
    } 
@@ -575,7 +611,7 @@
 }
 
 void
-TransactionState::startServerNonInviteTimerTrying(SipMessage& sip, Data& tid)
+TransactionState::startServerNonInviteTimerTrying(SipMessage& sip, const Data& tid)
 {
    unsigned int duration = 3500;
    if(Timer::T1 != 500) // optimzed for T1 == 500
diff -Naur resip/stack/TransactionState.hxx resip/stack/TransactionState.hxx
--- resip/stack/TransactionState.hxx
+++ resip/stack/TransactionState.hxx
@@ -95,7 +95,7 @@
       void terminateServerTransaction(const Data& tid); 
       const Data& tid(SipMessage* sip) const;
 
-      void startServerNonInviteTimerTrying(SipMessage& sip, Data& tid);
+      void startServerNonInviteTimerTrying(SipMessage& sip, const Data& tid);
 
       static TransactionState* makeCancelTransaction(TransactionState* tran, Machine machine, const Data& tid);
       static void handleInternalCancel(SipMessage* cancel,
@@ -110,6 +110,11 @@
 
       void saveOriginalContactAndVia(const SipMessage& msg);
       void restoreOriginalContactAndVia();
+
+      static bool processSipMessageAsNew(resip::SipMessage* sip, 
+                                         resip::TransactionController& controller,
+                                         const resip::Data& tid);
+
       
       TransactionController& mController;