Incorrect remainingBytes when continuation fragment is present in the same TCP packet

[borrrden]

I've been working on an issue that was reported to us where our implementation is force closing a websocket connection. Debugging has shown that we are considering this a buffer overflow, and with the information given to us that seems to be "correct". I'm not sure if this is a valid thing to happen or not since it only seems to happen with a specific proxy web service in place but the symptom is that I receive approximately a 17 KiB chunk of data from the remote, and that gets piped into the client protocol "consume" method.

Eventually it gets to a couple of fragments. One is a binary web socket message, and the next is a continuation message. Since they are all part of this large chunk, the following condition is satisfied for both of them:

https://github.com/uNetworking/uWebSockets/blob/7568327834020f2ceed3ce2a823f23d0cc18cb8c/src/WebSocketProtocol.h#L358

This means that when handleFragment is called, the remainingBytes are always 0.

https://github.com/uNetworking/uWebSockets/blob/7568327834020f2ceed3ce2a823f23d0cc18cb8c/src/WebSocketProtocol.h#L367-L369

You can see in our implementation, we rely on this information to create an appropriately sized buffer:

https://github.com/couchbase/couchbase-lite-core/blob/94af1bbc38e763c428397deec1cb41f41a2c45e8/Networking/WebSockets/WebSocketImpl.cc#L242-L246

However this gets called, for example, like the following:

payLength = 4092, remainingBytes = 0, opCode = 2, fin = 0 payLength = 1195, remainingBytes = 0, opCode = 2, fin = 1

Since the first entry here is not fin, we don't pipe the message onward since it is incomplete. That means that when the second entry comes, we don't have any space left in the buffer to write the continuation. Obviously, we could resize the buffer here but I'd prefer to have the information regarding the remaining bytes. It seems that the library is set up to handle continuations, but only if they come in separate TCP packets. Is there something that can be done here? Is this legal to do in terms of web sockets (i.e. should we press the proxy vendor about this?). It does seem like the remaining bytes won't be known until the following loop iteration, but maybe if the total length of the TCP packet were known (i.e. length, in addition to payLength) the buffer could be made that big "just in case".

[borrrden]

PS: Apologies if TCP packet is not the correct terminology here. I actually don't know what to call it, but it is a chunk of data that I've received from the network stack.

[uNetworkingAB]

It sounds highly unlikely given that this kind of edge case is something that is tested a lot by Autobahn. But in any case, should be very easy to write a triggering unit test given that the WebSocketProtocol.h is independent. But since it is very unlikely, I don't plan to debug something that's not shown to be a bug.

If you can show a triggering unit test I will fix it.

[borrrden]

It might not be a bug, but rather just a misunderstanding on my part, but what is remainingBytes supposed to represent inside of handleFragment? Since filing this issue a few minutes ago I came across what looks like the implementation from this repo (WebSocketContext.h) and it seems to be assuming more data is coming when remainingBytes is non-zero or fin is zero. I had thought that remainingBytes meant how many more bytes to expect for a given message but if that is not always the case and I should be prepared to accept more bytes in this case as well then I'll close the issue.

[borrrden]

Noticed the note, I'll take this to the Q&A.

[uNetworkingAB]

I haven't really done any major changes in WebSocketProtocol.h since 2016 so I don't have it fresh in my head but the probability of misuse by third party is about 50 trillion times more likely than such a common edge case being broken here since 2016 😉

[borrrden]

For sure. We took that protocol file into our own codebase about a year after that, and it is our own misuse it turns out. I think I totally misunderstood the remainingBytes variable. We never faced an issue until last month which lured me into overconfidence. I guess continuation frames are realllllly uncommon in the wild (at least with the servers and proxies that we have come across so far) because our implementation is not able to handle them (though the fix was simple). But having some time to think about it, it totally makes sense now why it is the way it is, it's just a bit of a gotcha since length / remaining is such a common C pattern and in this case remaining being 0 doesn't mean "nothing else is coming" but rather "something might be coming, please check fin"