[reSIProcate] ParseBuffer::assertNotEof() ?

[Byron Campen]

A few small comments inline.

> comments inline.
>
> On 9/29/06, Byron Campen <bcampen at estacado.net> 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 at sip.callshop.lvdx.com>;tag=\r\nTo:
>> > <sip:11 at sip.callshop.lvdx.com>;tag=\r\nContact:
>> > <sip:11 at 83.146.x.x:5070>\r\nSupported: replaces\r\nProxy-
>> > Authorization:
>> > Digest username=\"11\", realm=\"sip."..., mPosition = 0x809dd76 "\r
>> > \nTo:
>> > <sip:11 at sip.callshop.lvdx.com>;tag=\r\nContact:
>> > <sip:11 at 83.146.x.x: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 at sip.callshop.lvdx.com>;tag=\r\nContact:
>> > <sip:11 at 83.146.x.x: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 at list.sipfoundry.org
>> > https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
>>
>>
>>
>> _______________________________________________
>> resiprocate-devel mailing list
>> resiprocate-devel at list.sipfoundry.org
>> https://list.sipfoundry.org/mailman/listinfo/resiprocate-devel
>>
>>
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2369 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20061002/0998b392/attachment.bin>