711205 - Set WebSocket message size limit to 2 GB

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Right now there is an artificial limit 16MB for incoming messages.
That breaks easily for example any kind of chat application which has also file sending
capabilities. The web application just need to guess what the limit is and split
large files to parts. Not good.

bug 711003 is adding similar limit to outgoing messages, which is bad some API usage
perspective.
I did file some API bugs
https://bugzilla.mozilla.org/show_bug.cgi?id=711003#c5

Is there something to fix in the protocol too?
Should the protocol indicate to server how large messages can be sent?
(I haven't reviewed any recent versions of the protocol.)

Comment 1

[Jason Duell]

We should ask hixie about this (it seems more like an issue for the w3c spec than the wire protocol IMO).

Comment 2

[Patrick McManus [:mcmanus]]

the existing recv limit is somewhat arbitrary - I don't have a problem with a larger number

Comment 3

[Jason Duell]

Attachment: WIP: allow arbitrary incoming WS messages (up to 2 GB)

Patrick,

This is work in progress, but perhaps you can tell me if I'm barking up the right tree.  The basic idea is that we resize the incoming WS buffer with a fallible realloc and fail the WS if we can't get enough memory for it.  I also try to give back the buffer when we're done with a large one, though IIRC lots of allocators' free() implementations don't actually release memory back to the OS.

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 582646 [details] [diff] [review]
WIP: allow arbitrary incoming WS messages (up to 2 GB)

Review of attachment 582646 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +668,5 @@
>  
>  static nsWSAdmissionManager *sWebSocketAdmissions = nsnull;
>  
> +#define WEBSOCKET_INITIAL_BUFFERSIZE  16384
> +

websocket tries to use const static in the h file for numeric constants rather than preprocessor.

@@ +856,5 @@
>      ::memmove(mBuffer, mFramePtr - accumulatedFragments, mBuffered);
>      mFramePtr = mBuffer + accumulatedFragments;
>    } else {
>      // existing buffer is not sufficient, extend it
>      mBufferSize += count + 8192;

So this isn't really optimal. (I know, its my code). it is really saying "allocate enough for the current network read plus 8KB. That was good enough when we were capped to smallish messages but if the message size is unlimited this will result in lots of reallocs.

I'd love to see the known payload length of the frame (not the message) used to size the larger buffer. That won't work if we are reallocing to hold the message header so care must be taken, but it would save the majority of the reallocs when dealing with a 20MB message.

I'd be happy to take that in a different non-blocking patch if you file the bug for it.

@@ +860,5 @@
>      mBufferSize += count + 8192;
>      LOG(("WebSocketChannel: update read buffer extended to %u\n", mBufferSize));
>      PRUint8 *old = mBuffer;
> +    mBuffer = (PRUint8 *)moz_realloc(mBuffer, mBufferSize);
> +    if (mBuffer == nsnull)

the error path leaks mBuffer. (the original allocation is not freed if the new one fails)

I think just adding mBuffer = old to the realloc == nsnull path works.

also !mBuffer instead of == nsnull according to the coding style doc the last time I read it.

@@ +957,5 @@
>  
>      // we don't deal in > 31 bit websocket lengths.. and probably
>      // something considerably shorter (16MB by default)
> +//    if (payloadLength + mFragmentAccumulator > mMaxMessageSize) {
> +    if (payloadLength + mFragmentAccumulator > PR_INT32_MAX) {

Max message size is currently configurable - I think that's ok and I would prefer you just change the default to be PR_INT32_MAX instead of 16MB.

@@ +1198,5 @@
>      LOG(("WebSocketChannel:: Internal buffering not needed anymore"));
>      mBuffered = 0;
> +
> +    // release memory if we've been processing large messages
> +    // TODO: actually ensure memory is released to OS (use mmap?)

drop the TODO.. malloc uses mmap internally for large allocations and for small ones it can easily recycle them within firefox.. best not to mess with that voodoo - but we can avoid the alloc/free cycle for smaller sizes (see below)

@@ +1200,5 @@
> +
> +    // release memory if we've been processing large messages
> +    // TODO: actually ensure memory is released to OS (use mmap?)
> +    if (mBufferSize > WEBSOCKET_INITIAL_BUFFERSIZE) {
> +      mBufferSize = WEBSOCKET_INITIAL_BUFFERSIZE;

I generally like the idea of throwing out huge buffers, but we don't need to get into a alloc/free cycle for every usage pattern more than the default - especially because the default keeps usage intentionally low. But if you have an app that uses somewhat larger (but not extreme) sizes its ok too keep the larger buffer in place imo.

so maybe instead of checking > INITIAL we can check > 128KB ? (and make that a const).. either case can reset to initial though. If we really want to squeeze things we can have mobile always check > initial (or better a pref that is set to different vals for desktop and mobile) - but I'm not sure that its necessary in the case of websockets - there is only 1 buffer (not 1 per message) and we have active information that messages of that size are actively being used. So I'd be ok with just 128KB for all platforms.

@@ +1201,5 @@
> +    // release memory if we've been processing large messages
> +    // TODO: actually ensure memory is released to OS (use mmap?)
> +    if (mBufferSize > WEBSOCKET_INITIAL_BUFFERSIZE) {
> +      mBufferSize = WEBSOCKET_INITIAL_BUFFERSIZE;
> +      free(mBuffer);

moz_free()

Comment 5

[Jason Duell]

Attachment: IRC chat about incoming msg buffer sizing

As discussed in the attached IRC chat, to avoid a DOS attack (by dumbly allocated whatever size a server says it's going to send us: they could then just sit there and have forced us to allocate a 1 GB buffer that waits around), I'm going to 

1) allow arbitrary msg size

2) for up to 16MB (1MB on mobile) msg size just allocate full size of fragment, so no reallocs

3) to avoid DOS attacks, for >16/1MB, allocate a 16/1MB buffer, and increment it by 16/1MB as it fills.

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #5)
>
> 3) to avoid DOS attacks, for >16/1MB, allocate a 16/1MB buffer, and
> increment it by 16/1MB as it fills.

that's actually not a great strategy for large messages and not just because of the copy costs.

Consider a 716MB message. Somewhere around 700MB you'll need to realloc - for that to happen you need to actually have 1416MB of storage (and VM in 2 chunks) available to have them both around while you do the copy.

The odds of this succeeding are much lower than if you allocated 716MB up front which ends up reducing our effective max message size considerably. It also probably leads to a lot of late allocation failures after hundreds of MB have already been transferred rather than failing early.

both of those things are more important to me than a DOS path when there are so many existing ones anyhow. Maybe we don't do the big frame-size really until a decent "offer-of-proof" of 32MB or something but only leading by 16MB at a time seems crazy if we want to support the largest messages possible, which seems to be the point of this bug.

Comment 7

[Patrick McManus [:mcmanus]]

From an email I sent:  I think changes to the realloc logic (to something other than
+= amt + 8KB of constant realloc) is something that can be dealt with on
the next train without a problem. The key for unprefixing would be to
remove the limit so people don't feel the need to build the 16MB thing
into their JS.

Comment 8

[Jason Duell]

Attachment: v1

OK, here's the minimal fix to this bug.  It still reallocs every 10K of input, which is polynomial for large message sizes.  I'm working on a more efficient patch.

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 582959 [details] [diff] [review]
v1

Review of attachment 582959 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +854,5 @@
>      ::memmove(mBuffer, mFramePtr - accumulatedFragments, mBuffered);
>      mFramePtr = mBuffer + accumulatedFragments;
>    } else {
>      // existing buffer is not sufficient, extend it
>      mBufferSize += count + 8192;

If you want to change this now to something like

mBufferSize += count + 8192 + (mBufferSize / 4)

so that it scales a bit better I'm ok with that. or not - up to you.

Comment 10

[Jason Duell]

Comment on attachment 582959 [details] [diff] [review]
v1

This passes the Autobahn large size test (tweaked to send 160 MB), BTW.

Comment 11

[Jason Duell]

Attachment: Limit outbound msg size to PR_INT32_MAX

We need to this too, as all the internal plumbing for sending still uses PRInt32's at the moment.

Comment 12

[Jason Duell]

Attachment: Followup to v1: fewer reallocs

This avoids most reallocs while still keeping the buffer small until we've got some sense that this isn't a DOS attack.  

Big-O is n*log(n), but better in practice once we hit the trust threshold.

Comment 13

[Jason Duell]

Note: I haven't tested this on fragmented msgs yet, will do once I get back in few hours.  Won't check in unless both frag/non-frag msgs pass.  And of course we can punt, too...

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 583037 [details] [diff] [review]
Followup to v1:  fewer reallocs

this isn't going to work with fragments - mIncomingSize is just the size of the fragment and the buffer has to hold the whole message.. if fragments are in play we don't really know what the whole message size is (just like http chunked encoding).

also I don't think mIncomingSize is being managed correctly.. its possible the buffer needs to be resized just to read the frameheader with the frame size in it. (this is unlikely, but with fragments and whatnot that buffer does not always start at the front) and of course the place where it is reset to 0 does not happen after every frame (though it usually does).

lets punt this to ff12

Comment 15

[Jason Duell]

Went with

     mBufferSize += count + 8192 + (mBufferSize / 3)

https://hg.mozilla.org/mozilla-central/rev/998eecfec4de

Comment 16

[Jason Duell]

"whatever malloc can handle" sounded too cool not to put into hg commit, but is actually off by up to 18,446,744,071,562,067,967 bytes on 64-bit systems.

Also this bug actually lowered the outbound max from 4 GB down to 2 GB.   So I guess it's really a wash--the *average* max websocket message size is actually now 8MB lower.  Maybe 16MB incoming/4 GB outgoing message limits could have been useful for users interacting with websocket servers located on home ASDL networks (with high send, slow receive from websockets' perspective).  

Ah well, too late to back it out now.  But we can document mistakes that were made.

Comment 17

[Jason Duell]

Sigh. Even mistakes documenting the mistakes.

Comment 18

[Jason Duell]

Which reminds me...

So we might want to mention in our docs that we currently support up to 2 GB Web Socket messages, both incoming/outgoing.  That's the theoretical limit--malloc is sure to fail on mobile for such sizes, in which case (as spec mandates) the websocket is failed.

Comment 19

[Eric Shepherd [:sheppy]]

Added a note here:

https://developer.mozilla.org/en/WebSockets/WebSockets_reference/WebSocket#Gecko_notes

and here:

https://developer.mozilla.org/en/WebSockets#Gecko_11.0

And mentioned on Firefox 11 for developers.