| < Previous by Date | Date Index | Next by Date > |
| < Previous in Thread | Thread Index | Next in Thread > |
If I understand it correctly it does not grab a huge chunk at a time, it reads only 2048 bytes from the stream at a time.
I will illustrate with a small example. here the chunk size is 5 instead of 2048 and the message has the following header:
Example-Header: aaabbbcccddd
first round:
bufferSize=5
buffer1="Examp"
scanChunk returns scrNextChunk and that 5 bytes are not parsed
buffer1 is given to SipMessage
buffer2 is allocated at size 1.5*(unparsed bytes) = 8
the unparsed bytes are copied to buffer2
SipMessage now contains buffer1(5 bytes)
second round:
bufferSize=8
buffer2="Example-"
scanChunk returns scrNextChunk and that 8 bytes are not parsed
buffer2 is given to SipMessage
buffer3 is allocated at size 1.5*(unparsed bytes) = 12
the unparsed bytes are copied to buffer3
SipMessage now contains buffer1(5 bytes) + buffer2(8) = 13 in total
third round:
bufferSize=12
buffer3="Example-Head"
scanChunk returns scrNextChunk and that 12 bytes are not parsed
buffer3 is given to SipMessage
buffer4 is allocated at size 1.5*(unparsed bytes) = 18
the unparsed bytes are copied to buffer4
SipMessage now contains buffer1(5 bytes) + buffer2(8) + buffer3(12) = 25 in total
This will continue with larger and larger chunks.
with real world numbers this would of course be buffer1(2000) + buffer2(3000) + buffer3(4500)
When the largest chunk is 1MB it means you also have 600k + 400k + 300k + 200k and so on stored in the SipMessage.
Is this correct or have I misunderstood?
I added debug prints and chunkLength and numUnprocessedChars were both > 1M the last time before it ran out of memory. Both were also increasing for each round.
> It looks like we are taking things in smallish chunks in order to
> save room; if we grab a chunk that is 50K long, and it contains 50 SIP
> messages,
The way I understand it is that it only gets that big because one header is that big. With smaller headers it would never reach that state.
> Since a chunk can contain more than one SIP message, we can't really
> arbitrarily restrict the chunk-size, although we _can_ put code in
> that will notice when we have run the pre-parser over an unreasonably
> large number of bytes.
Isn't that what my code does, saying that 50k is unreasonable and then discard the tcp connection? Or did you mean to handle it in another way? If I'm correct, then 50k means that there is 50 + 30 + 20 + ... bytes allocated already = 150k or so.
Since I don't understand scanChunk completely I only assumed that this behavior is because it wants to put a complete header in one chunk, and that is why it request more and more data.
Regards
/Markus
-----Original Message-----
From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx]
Sent: Thu 7/10/2008 4:54 PM
To: Markus Eriksson
Cc: resiprocate-devel@xxxxxxxxxxxxxxx
Subject: Re: [reSIProcate] resiprocate crash due to no free memory when receiving sip message with large header
Responses inline, since this is kinda complicated:
> 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.
>
>
It looks like we are taking things in smallish chunks in order to
save room; if we grab a chunk that is 50K long, and it contains 50 SIP
messages, that 50K buffer will be added to the first SipMessage we
parse, and then will be copied and trimmed. The next SipMessage will
get a 49K buffer added. And the next a 48K buffer. It would be
difficult to just give the SipMessage a memcpy of the part of the
buffer it needs, since that would destroy the overlay that the
MsgHeaderScanner just put together (ie, all those pointers would have
to be adjusted to point into the new buffer). So, the buffers are kept
relatively small. I am not sure how you're seeing "increasingly large
buffers" being added; they should be kept to a maximum of 2K.
> In the failing case the header is >1MB, so there will be
> approximately 500 buffers in the SipMessage containing part of the
> large header.
>
>
This is baffling; 1M is not a large amount of memory. Are you sure it
is the buffers that are pushing us over the edge?
> 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.
>
>
The buffers are used; as the pre-parser runs, it passes pointers and
lengths to SipMessage that point into these buffers. The buffers are
kept around to keep the memory intact, so the pointers aren't left
dangling.
> 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;
> > }
>
>
Since a chunk can contain more than one SIP message, we can't really
arbitrarily restrict the chunk-size, although we _can_ put code in
that will notice when we have run the pre-parser over an unreasonably
large number of bytes.
Best regards,
Byron Campen