reSIProcate

Mailing Lists

Source Code

Google Search:



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

Re: [reSIProcate] ParseBuffer::assertNotEof() ?


A few small comments inline.


comments inline.

On 9/29/06, Byron Campen <bcampen@xxxxxxxxxxxx> wrote:

        This sounds like a bug in dum. If an exception due to a parse
failure is making it out of dum, then it is very likely the stack has
leaked a TransactionState. If the exception is not caught in the
appropriate place, not only is the SipMessage leaked, but there is no
way to respond to it, meaning the TransactionState will never get
cleaned up either (the stack waits indefinitely for the TU to respond). 


The current design leaves it up to the TU to catch parse exceptions
and to send a failure response to the request as a result. I agree
this is far from ideal.
I really don't have an objection to this architecture; it is the price we pay for saving performance. What boggles my mind is that none of the code I have seen actually does this. Were people just not aware?
Unfortunately, there are many such bugs, because a lot of the code 
was written before we transitioned to a lazy-parser, and the code was
never fixed. The upshot is that calling something like msg.header
(h_HeaderName).someAccessor() will probably be what triggers the
parse of the header, and if that header is malformed, kaboom!


The above comment is not really correct. resip has had a lazy parser
since the beginning. Any application code that triggers parsing needs
to check for parse exceptions. In many cases, it should be possible to
put a single try/catch that handles ParseExceptions somewhere fairly
high up in a ThreadIf subclass main loop.  I think we can safely put a
try/catch in DialogUsageManager::internalProcess that can handle lots
of these cases and automatically send a failure response when a parse
exception occurs as a result of an inbound sip request. This should
catch most of these issues.
I don't know; that message is passed around using an auto_ptr, by value. We won't have a pointer to the malformed request once we call incomingProcess. We could make an extra copy of the message (expensive) or the pointer (dangerous), and use that to form a response if something goes wrong.
Fixing all the instances of this will take some time. I tried my 
best to clear up any errors of this type that existed in the stack
(and I think I've gotten them all). I think that, at this point, it
would make sense to add bool LazyParser::isWellFormed(), so we can
check whether the header is well-formed in an exception-safe manner.
This will make fixing the existing code a little easier. And for
those who are writing new code, keep this in mind!


Adding isWellFormed() to ParserCategory seems like a good idea.
Now, I added a bandage for this problem that people can use in the 
meantime. As a configure option, you can set PEDANTIC_STACK to true,
which cause the stack to do a full parse of SipMessages on receipt.
If a problem is discovered, it will be rejected immediately. (ie, no
potential for a parse to fail later down the line, since everything
is already parsed) This is an expensive solution, but at least you
have the option.


In general this is not a good idea. Trigger a parse of all header
fields including all of the headers that you don't care about can make
your application less tolerant. For example, most proxies only need to
look at a handful of headers and none of the bodies.
I agree completely, but when we are vulnerable to memory exhaustion attacks that leverage the issues above, this option is a nice one to have. 


Best regards,
Byron Campen

>
>
> Occasionally, my application is receiving a packet which causes
> assertNotEof() to throw an exception.
>
> How is this exception meant to be dealt with?  Am I intended to
> catch it
> with a try { } catch { } block around dum->process()? Or should this > exception be caught and handled (presumably by discarding the packet) 
> within the stack?
>
> Here is the content of pb when the exception is thrown, the code in
> DataParameter.cxx suggests the empty ;tag= is the fault:
>
> (gdb) print pb
> $1 = (class resip::ParseBuffer &) @0xbfffe114: {
>   static Whitespace = 0xb7ac4264 " \t\r\n",
>   static ParamTerm = 0xb7ac4261 ";?",
>   mBuff = 0x809dd4a "\"Booth2\"
> <sip:11@xxxxxxxxxxxxxxxxxxxxx>;tag=\r\nTo:
> <sip:11@xxxxxxxxxxxxxxxxxxxxx>;tag=\r\nContact:
> <sip:11@xxxxxxxxxx:5070>\r\nSupported: replaces\r\nProxy-
> Authorization:
> Digest username=\"11\", realm=\"sip."..., mPosition = 0x809dd76 "\r
> \nTo:
> <sip:11@xxxxxxxxxxxxxxxxxxxxx>;tag=\r\nContact:
> <sip:11@xxxxxxxxxx:5070>\r\nSupported: replaces\r\nProxy-
> Authorization:
> Digest username=\"11\", realm=\"sip.callshop.lvdx.com\",
> algorithm=MD5,
> uri=\"sip:"...,
>   mEnd = 0x809dd76 "\r\nTo:
> <sip:11@xxxxxxxxxxxxxxxxxxxxx>;tag=\r\nContact:
> <sip:11@xxxxxxxxxx:5070>\r\nSupported: replaces\r\nProxy-
> Authorization:
> Digest username=\"11\", realm=\"sip.callshop.lvdx.com\",
> algorithm=MD5,
> uri=\"sip:"...,
>   mErrorContext = @0xb7dc5c18}
>
>
> Regards,
>
> Daniel
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel@xxxxxxxxxxxxxxxxxxx
> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel



_______________________________________________
resiprocate-devel mailing list
resiprocate-devel@xxxxxxxxxxxxxxxxxxx
https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel

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