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

Re: [reSIProcate] Probable memory leaks in DialogUsageManager::incomingProcess


Well, I haven't seen any activity in this thread, but here's a patch. I'm going to apply it tomorrow unless I get some feedback telling me to do otherwise.

Attachment: dumfixes2.patch
Description: Binary data


Best regards,
Byron Campen

Just after running incoming messages through the feature chains, we enter a try block whose catch logs an error, and takes no further action.

*snip*
   catch(BaseException& e)
   {
      //unparseable, bad 403 w/ 2543 trans it from FWD, etc
  ErrLog(<<"Illegal message rejected: " << e.getMessage());
   }
*snip*

Looking at the code, incomingProcess is ultimately responsible for making and sending responses to requests. It may delegate this responsibility, but its job is to ensure that _everything_ gets a reply of some sort before returning. Failing to reply to a request will cause the stack to leak memory, as I have said a million times. I can see several places where an exception will be thrown upon receiving garbage from the wire.

================================================================================

in MasterProfile::getUnsupportedOptionsTags() at line 110 called by
DialogUsageManager::validateRequiredOptions() at line 1325 called by
DialogUsageManager::incomingProcess() at line 1218
*snip*
Tokens
MasterProfile::getUnsupportedOptionsTags(const Tokens& requiresOptionTags)
{
   Tokens tokens;
   for (Tokens::const_iterator i=requiresOptionTags.begin(); i != requiresOptionTags.end(); ++i)
   {
      // if this option is not supported
      if (!mSupportedOptionTags.find(*i))
      {
         tokens.push_back(*i);
      }
   }
  
   return tokens;
}
*snip*

The code highlighted will throw if we encounter a malformed Token (a Requires header-field (Token::parse() won't throw on us itself, but it calls parseParameters(), which means all bets are off)

================================================================================

in DialogUsageManager::validateContent() at line 1347 called by
DialogUsageManager::incomingProcess() at line 1223
*snip*
   // RFC3261 - 8.2.3
   // Don't need to validate content headers if they are specified as optional in the content-disposition
   if (!(request.exists(h_ContentDisposition) &&
     request.header(h_ContentDisposition).exists(p_handling) &&
     isEqualNoCase(request.header(h_ContentDisposition).param(p_handling), Symbols::Optional)))
   {
*snip*

The code highlighted will throw if Content-Disposition is malformed.

================================================================================

in MasterProfile::isMimeTypeSupported() at line 143 called by
DialogUsageManager::validateContent() at line 1351 called by
DialogUsageManager::incomingProcess() at line 1223

*snip*
bool
MasterProfile::isMimeTypeSupported(const MethodTypes& method, const Mime& mimeType)
{
   std::map<MethodTypes, Mimes>::iterator found = mSupportedMimeTypes.find(method);
   if (found != mSupportedMimeTypes.end())
   {
      return found->second.find(mimeType);
   }
   return false;
}
*snip*

The code highlighted will throw if the mimeType passed (the Content-Type header-field-value) is malformed.

================================================================================

in MasterProfile::isContentEncodingSupported() at line 184 called by
DialogUsageManager::validateContent() at line 1363 called by
DialogUsageManager::incomingProcess() at line 1223
*snip*
bool
MasterProfile::isContentEncodingSupported(const Token& encoding) const
{
   return mSupportedEncodings.find(encoding);
}
*snip*

The highlighted text will throw if the Token (the Content-Encoding header-field-value) is malformed.

================================================================================

in MasterProfile::isLanguageSupported() at line 184 called by
DialogUsageManager::validateContent() at line 1375 called by
DialogUsageManager::incomingProcess() at line 1223
*snip*
bool
MasterProfile::isLanguageSupported(const Tokens& langs) const
{
   for (Tokens::const_iterator i=langs.begin(); i != langs.end(); ++i)
   {
      if (mSupportedLanguages.find(*i) == false)
      {
         return false;
      }
   }
   return true;
}
*snip*

================================================================================

in MasterProfile::isMimeTypeSupported() at line 143 called by
DialogUsageManager::validateAccept() at line 1401 called by
DialogUsageManager::incomingProcess() at line 1228
*snip*
bool
MasterProfile::isMimeTypeSupported(const MethodTypes& method, const Mime& mimeType)
{
   std::map<MethodTypes, Mimes>::iterator found = mSupportedMimeTypes.find(method);
   if (found != mSupportedMimeTypes.end())
   {
      return found->second.find(mimeType);
   }
   return false;
}
*snip*

The code highlighted will throw if the mimeType passed (an Accept header-field-value) is malformed.

================================================================================

in DialogUsageManager::incomingProcess() at line 1234
*snip*
               }
            }
            if (sipMsg->header(h_From).exists(p_tag))
            {
               if (mergeRequest(*sipMsg) )
               {
                  InfoLog (<< "Merged request: " << *sipMsg);
                  return;
               }
            }
*snip*

If the From header-field-value is malformed, this will throw (unless something has already checked it).

================================================================================

in DialogUsageManager::mergeRequest() at line 1438 called by
DialogUsageManager::incomingProcess() at line 1236
*snip*
   assert(request.isRequest());
   assert(request.isExternal());

   if (!request.header(h_To).exists(p_tag))
   {
      if (mMergedRequests.count(MergedRequestKey(request, getMasterProfile()->checkReqUriInMergeDetectionEnabled())))
      {
         SipMessage failure;
         makeResponse(failure, request, 482, "Merged Request");
         failure.header(h_AcceptLanguages) = getMasterProfile()->getSupportedLanguages();
         sendResponse(failure);
         return true;
      }
   }
*snip*

If the To header-field-value is malformed, an exception is thrown.

================================================================================

in MergedRequestKey::MergedRequestKey() at line 17
called by DialogUsageManager::mergeRequest() at line 1440
DialogUsageManager::incomingProcess() at line 1236
*snip*
MergedRequestKey::MergedRequestKey(const SipMessage& req, bool checkRequestUri) :
   mRequestUri(Data::from(req.header(h_RequestLine).uri())),
   mCSeq(Data::from(req.header(h_CSeq))),
   mTag(req.header(h_From).exists(p_tag) ? req.header(h_From).param(p_tag) : Data::Empty),
   mCallId(req.header(h_CallID).value()),
   mCheckRequestUri(checkRequestUri)
{
}
*snip*

If the Call-Id header-field-value is malformed, this will throw. (additionally, if the From header-field-value is malformed, we throw, but there is code that runs before this that will throw first. CSeq is safe, since it is checked for well-formedness by the stack, but it may be appropriate to account for this anyway in case things change)

================================================================================


Hitting any of these code-paths with a request will cause the stack to leak a TransactionState. Our problems do not end there, however. I haven't had time to look through all the possible feature-chains that can be invoked, but if any of them throw, the exception ends up making it to an even higher level (whatever is calling DialogUsageManager::process()), and we leak. Lastly, I have not gotten around to inspecting either of DialogUsageManager::processRequest() or DialogUsageManager::processResponse(). If either of these end up throwing, we are going to leak.

We need to fix this stuff. I would take a crack at fixing it, but then again I am just getting my feet wet with DUM. I could break something unintentionally.

Best regards,
Byron Campen





_______________________________________________
resiprocate-devel mailing list

Attachment: smime.p7s
Description: S/MIME cryptographic signature