| < Previous by Date | Date Index | Next by Date > |
| < Previous in Thread | Thread Index | Next in Thread > |
|
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] 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. Best regards, Byron Campen
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]
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. Best regards, Byron Campen
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) @@ -148,6 +148,14 @@ assert(!contents.mMine); 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); + delete temp; + } + Contents* c; if
(ContentsFactoryBase::getFactoryMap().find(contentType) !=
ContentsFactoryBase::getFactoryMap().end()) { Best regards, Byron Campen
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 From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx]
Yes, but is this code safe when you have a sipfrag in a multipart-mixed payload? Best regards, Byron Campen
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 From: resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx
[mailto:resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx]
On Behalf Of Byron Campen
Good eye. Your solution sounds about as optimal as can be managed. Best regards, Byron Campen I found this in
SipFrag.cxx: msgHeaderScanner.prepareForFrag(mMessage,
hasStartLine(buffer, size)); saveTermCharArray[0] = termCharArray[0];
&scanTermCharPtr); |