Re: [reSIProcate] SipFrag parsing leads to heap corruption

[Alan Hawrylyshen <alan@xxxxxxxxxx>]

On 8-Nov-05, at 00:42 , Sasha Youkhananov wrote:

> Hello resiprocate developers.
>
>
>
> I build a soft phone application under Windows XP [snip]
>
> I examined the situation several times and found that SipFrag::parse 
> () call leads to memory corruption.
>
> Here is the call stack:
>
> resip::DialogUsageManager::process ()
>
> resip::DialogUsageManager::internalProcess ()
>
> resip::DialogUsageManager::processRequest ()
>
> [snip]
> resip::MsgHeaderScanner::scanChunk ()
>
> resip::processMsgHeaderStatusLine ()
>
> resip::SipMessage::setStartLine
>
>
>
> So, during construction of BaseSubscription the original request  
> with sip-frag body is copied into mLastRequest member. When copy  
> constructor of SipMessage works it copies mContentsHfv of original  
> request to mContentsHfv of message that is being constructed. The  
> copy constructor of HeaderFieldValue copies exactly mFieldLength  
> bytes to constructed object.
>
> When SipFrag parser starts its processing it writes 4 consecutive  
> bytes (according to its algorithm) in area that is located at the  
> address: buffer + size. That address is behind the allocated memory  
> (was previously allocated in HeaderFieldValue copy constructor) and  
> in debug mode is used to store the internal heap data (4  
> consecutive 0xFD). Thus, the next memory allocation request in  
> SipMessage::setStartLine fails.
>
>
>
> In order to repair this situation I changed copy constructor of  
> HeaderFieldValue type to following:
>
>
>
> HeaderFieldValue::HeaderFieldValue (const HeaderFieldValue& hfv)
>
>    : mField(0),
>
>      mFieldLength(hfv.mFieldLength),
>
>      mMine(true)
>
> {
>
>    //InfoLog (<< "Making a copy of a HFV" << hex << this);
>
>    mField = new char[mFieldLength + 5];
>
>    memcpy(const_cast<char*>(mField), hfv.mField, mFieldLength + 5);
>
> }
>
>
>
> Of course, this change will affect other stack parts which use this  
> type, but I haven’t found more appropriate place to my fix.
>
> I’m sure you can suggest the better way to repair it.
>
> Wait for any answer about this problem.

Sasha;

Thanks for the excellent report on this. I think it's high time we  
fixed the MHS. There have been bits and pieces scattered around the  
code to deal with the 4 byte over-run. I think it's not appropriate  
and the MHS should scale back it's aggression, I welcome anyone to  
argue in the other direction, but you'll have to convince me that  
it's worthwhile to do a fix anywhere but in the MHS itself.

Thanks,

Alan