| < Previous by Date | Date Index | Next by Date > |
| Thread Index | Next in Thread > |
Hi,
I have run a test tool that tries to find faults in sip user agents by sending faulty or weird messages.
The problem I have found is that when resiprocate gets a message via TCP with an extremely large header it will use up all memory and then crash from a "new" that fails.
If I understand ConnectionBase::preparseNewBytes and MsgHeaderScanner::scanChunk correctly it will allocate a new buffer of size 2048 or greater. If a header does not fit in this it will leave some bytes unparsed and parse them next time. These unparsed bytes will then be copied into the next buffer that is bigger than the previous one. Then the buffer is added to the SipMessage that takes over the ownership of the buffer.
The second buffer will then again not be big enough and the procedure is repeated. This will lead to increasingly larger buffers ,containing more and more of the large header, being allocated and given to the SipMessage.
In the failing case the header is >1MB, so there will be approximately 500 buffers in the SipMessage containing part of the large header.
What I want to know is why the buffers are given to the SipMessage in this way. I could not even find a place where the buffers stored in SipMessage was actually used.
What I did to "fix" this is I check if chunkLength (buffer size) is bigger than 50kb. If so, the header is too big and the connection is dropped.
Someone with more in-depth knowledge of how the pre-parser works and what it is meant to do, should probably fix the memory leak properly though. If a header is just big enough so that the last allocation succeeds, the SipMessage would end up owning buffers that has allocated all your memory and something else could crash from a failed allocation.
Here is a diff with my addition to drop the connection.
ConnectionBase.cxx.diff:
143a144,154
> if (chunkLength > 50000)
> {
> WarningLog(<< "Discarding preparse! Too big Header");
> delete [] mBuffer;
> mBuffer = 0;
> delete mMessage;
> mMessage = 0;
> //.jacob. Shouldn't the state also be set here?
> delete this;
> return;
> }
/Markus