Yeah, that did it. Check testSipFrag under valgrind. So, the fix you have proposed sounds like it would work. Lets have that integrated/tested sometime tomorrow if we can.
Best regards, Byron Campen Ahh, that isn't a full parse at MultipartMixedContents.cxx:228. So, I guess if you called MultipartMixedContents::parts(), without causing the SipFrag to parse, and THEN copied the SipMessage, we would get badness. Let me try this...
Best regards, Byron Campen Not sure it can’t occur; It looks like Contents::createContents doesn’t add padding bytes(which is fine). The issue is that MultipartMixedContents::MultipartMixedContents(const MultipartMixedContents& rhs) will call SipFrag->clone() and if the SipFrag isn’t parsed the passing bytes are lost. Where do you see the individual parts being non-lazily parsed?
From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx]
Sent: Tuesday, September 12, 2006 2:51 PM
To: Derek MacDonald
Cc: 'resiprocate-devel'
Subject: Re: [reSIProcate] bug in SipFrag.cxx I thought about that possibility; but right now the individual parts never get their own HeaderFieldValues. Only one is ever assigned, to the MultipartMixedContents. When we call parts(), checkParsed() is called, and the individual *Contents are then created and immediately parsed (HeaderFieldValues are never assigned). However, it occurs to me that if we had a single unparsed SipFrag in a SipMessage's body, and then copied that SipMessage, we could be in loads of trouble.
Looking at this further, we don’t have an issue with MultipartMixed. What the sipfrag parser does it save the extra 4 bytes it needs, parse the sipfrag, and restores the four bytes. As a result, we don’t actually stomp on the boundary token, we just temporarily overwrite. This is completely safe until the unlikely day where we parallelize the MultipartMixed parser. However, there might be an even more subtle problem. If the MultiPartMixed is cloned after parse but before the parts are parsed, HeaderFieldValue(const HeaderFieldValue& hfv, CopyPaddingEnum) is not invoked which would mean that SipFrag::parse will write beyond the memory of its hfv. This didn’t show up in valgrind, but I believe it occurs(may not have hit the test, it might be valgrind behaviour). SipMessage invokes HeaderFieldValue(const HeaderFieldValue& hfv, CopyPaddingEnum) to avoid an issue we found w/ SipFrag parsing after a copy. The fix would be to have SipFrag have the same effective behaviour, adding the CopyPaddingEnum cons. Parameter to a new constructor for Contents and LazyParser. Thoughts? -Derek
From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx]
Sent: Tuesday, September 12, 2006 8:55 AM
To: Derek MacDonald
Cc: resiprocate-devel
Subject: Re: [reSIProcate] bug in SipFrag.cxx Well, I wrote a test-case for this, and it appears to function without the fix. It looks like we stomp on the boundary token while parsing the sipfrag, but at that point we have already done everything we need with the boundary token. When we encode, we use the boundary token we stored at the very beginning, and so the bits that we stomped on never make it to the wire. (Although it looks like we are encoding the extra CRLFCRLF at the end of the sipfrag; is this a problem?) This seems fragile to me; any change in the way we are parsing or encoding could cause the whole thing to fall apart.
Does this look sane? Is it important to check whether the primary mimetype is message? Index: resip/stack/Contents.cxx =================================================================== --- resip/stack/Contents.cxx (revision 6561) +++ resip/stack/Contents.cxx (working copy) HeaderFieldValue *hfv = new HeaderFieldValue(contents.data(), contents.size()); + if(contentType.subType()=="sipfrag"||contentType.subType()=="external-body") + // !bwc! The parser for sipfrag requires padding at the end of the hfv. + HeaderFieldValue* temp = hfv; + hfv = new HeaderFieldValue(*temp,HeaderFieldValue::CopyPadding); if (ContentsFactoryBase::getFactoryMap().find(contentType) != ContentsFactoryBase::getFactoryMap().end())
Hmm, probably not; looking at MultipartMixedContents we probably need a fairly expensive special case copy when the content-type is message/sipfrag or message/external. It should be easy to reproduce&fix. -Derek Yes, but is this code safe when you have a sipfrag in a multipart-mixed payload?
Yes; the transport classes use MsgHeaderScanner::allocateBuffer(int size) to guarantee that there are 5 extra bytes at the end of each buffer. I do not know of any memory problems as a result of this with the reciprocate transports. -Derek Good eye. Your solution sounds about as optimal as can be managed. Byron Campen
Hi, I found this in SipFrag.cxx: msgHeaderScanner.prepareForFrag(mMessage, hasStartLine(buffer, size)); saveTermCharArray[0] = termCharArray[0]; ; &scanTermCharPtr);
_______________________________________________ resiprocate-devel mailing list
|