On 25/03/13 16:09, Byron Campen wrote:
On Mar 25, 2013 12:56 AM, "Daniel Pocock" <daniel@xxxxxxxxxxxxx>
wrote:
>
>
>
> On 25/03/13 04:05, Byron Campen wrote:
> > Ok, the code that handles buffer allocation for the
body runs from
> > ConnectionBase.cxx:303-386. Here's the gist of it:
> >
> > - Parse the Content-Length header, or drop.
>
> http://tools.ietf.org/html/rfc3261#section-20.14
>
> Content-Length is actually optional for UDP and for
WebSockets, because
> they both have their own way of specifying the size of a
message unit:
>
> http://tools.ietf.org/html/draft-ietf-sipcore-sip-websocket-08#section-5.1
>
Yep, as far as I know the udp code doesn't require it.
Should check though.
> This has two consequences:
>
> a) We should not assume it is always there
>
> b) We should add a Content-Length header when necessary
(e.g. proxying
> from WebSocket to TLS or TCP)
>
We always add it on send, I think.
> If this appears undesirable, now is the time to comment on
the draft.
>
>
> > - Perform sanity-check on value of Content-Length, or
drop.
>
> I see 10485760 hardcoded there now (10MB), I would propose:
> - making it configurable
> - WebSocket code will use the same value when screening the
size in a
> payload size header
>
Yeah, definitely should be configurable.
Done - default is the same, default can be changed at compile time
and it can be set at run time too
Here is an example of setting it a compile time:
./configure RESIP_SIP_MSG_MAX_BYTES=16384 --with-ssl --with-c-ares
> > - If Content-Length indicates that we have already
read the entire body in
> > a prior read (ie; while reading headers), do a little
buffer management so
> > that the next message is preserved (we've already read
at least part of
> > it), call SipMessage::setBody(), and move the message
along.
> > - Otherwise, alloc a buffer of either ChunkSize or
half again the size of
> > the body read so far, whichever is _bigger_, up to a
maximum of the
> > Content-Length.
> >
> > As more bytes of the body come in, whenever we run out
of space, we (sort
> > of) repeat the logic in the last step above (see
ConnectionBase.cxx:503).
> >
> > In no case is ChunkSize acting as an upper bound on
the size of a buffer
> > allocation for a body. Initially it is used as a lower
bound, though.
>
> For traditional streams, I agree, it is not so bad. It is
the way that
> it has been slightly misused by the WebSocket code that
leads to problems.
>
> > In regard to the logging above, I've looked again at
UdpTransport since I
> > was not seeing any of the other logging I would expect
to, but it seems
> > that the debug logging in there has been commented
out. I would bet you $10
> > that what you just saw was a truncated UDP datagram
come in due to
> > fragmentation; the Content-Length says there are 4657
bytes of body, but
> > the datagram ends 866 bytes after the headers.
>
> Nathan's example was for WebSocket messages (not UDP), and
it was using
> the latest version of b-webrtc, which no longer tries to
rearrange
> mBuffer and force it back into preparseNewBytes (see the
changes I made
> about a week ago)
> http://svn.resiprocate.org/viewsvn/resiprocate/branches/b-webrtc/resip/stack/ConnectionBase.cxx?r1=10045&r2=10046&diff_format=l
>
> In the WebSocket code, ChunkSize has been used as a limit
on buffer
> size, and this is one of the main things I need to resolve
>
On a mobile again, so difficult to look for myself right now.
I've also used the same limit for WS messages now. It will use the
limit to limit the buffer size as bytes come in, and also to
validate the length field in the frame header. Anything too big
drops the connection.
We could potentially add this size limit as an option in
repro.config too
|