Client: cpp
TWebSocketServer overrides readAll_virt(), which reads WebSocket frames. The
non-virtual readAll() it inherited, however, comes from
TVirtualTransport<THttpTransport> and reads through THttpTransport::read().
A readAll() called on a TWebSocketServer, rather than through a TTransport,
therefore read the HTTP stream instead of the frame payload. TWebSocketServer
now defines readAll() to call readAll_virt(), as the note on
TVirtualTransport::readAll() suggests, so both calls behave alike.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Client: cpp
readAll on TWebSocketServer now returns the number of bytes it was asked
for, drawing on further frames when one frame does not carry them all, and
raises at end of stream once a request has begun. That is the contract the
base transport and the other bindings' readAll already meet.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three related framing defects in TWebSocketServer, all demonstrated against
master.
The masking key of a zero-length masked frame was never consumed. readFrame
read the four bytes inside "if (length > 0)", but RFC 6455 5.2 puts the
masking key in every frame whose MASK bit is set whatever the payload
length, and readFrame itself refuses a client frame that does not set MASK.
The four bytes stayed in the stream and the next header was parsed out of
the masking key:
client: 89 80 37 37 37 37 an empty Ping
82 85 37 37 37 37 ... a five-byte data frame
server: TTransportException: Reserved bits must be zeroes
An empty Ping, an empty Pong and a Close with no status code are all
ordinary traffic, so nothing unusual on the wire is needed to reach it.
A Pong declared a length of zero and then wrote the payload after it.
writeFrameHeader took its length from writeBuffer_.available_read(), which
describes the body only for flush(), the one caller whose body is the write
buffer. A Ping of eight bytes produced
8A 08 ... expected 8A 00 50 49 4E 47 44 41 54 41 written
so a conforming client read a zero-length Pong and then parsed 0x50 as the
next frame header, whose reserved bits are set. failConnection did the same
with its two-byte close code, 88 00 03 F1, which made the reason the
connection ended unreadable.
So pass writeFrameHeader the length it is describing instead of letting it
guess, read the masking key as part of the header it belongs to, and reset
the read buffer for a frame with no payload, so that a Pong for an empty
Ping echoes nothing rather than whatever the frame before it left unread.
On the wire, after: 8A 08 50 49 4E 47 44 41 54 41 and 88 02 03 F1.
Five tests. Four fail before the change -- two on the exception, two on the
bytes written. The fifth is a regression guard on flush(), the caller whose
body really is the write buffer, and passes either way.
Client: cpp
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TWebSocketServer::readFrame answered a Ping by calling itself:
case Opcode::Ping:
pong();
return readFrame();
A Ping carries nothing for the caller, so the reader has to go on to the
next frame to satisfy it. Doing that by re-entering readFrame costs one
stack frame per Ping, and nothing bounds how many a peer may send.
Measured against master, with Ping frames of one payload byte -- seven
bytes each on the wire -- followed by one ordinary data frame:
20,000 pings (140 kB) -> 3,200,344 bytes of stack
40,000 pings (280 kB) -> 6,400,344 bytes of stack
60,000 pings (420 kB) -> segmentation fault
160 bytes of stack for every seven bytes of input, so the 8 MB a Linux
thread starts with is gone by about 52,000 pings. It is not build
dependent: gcc 11 does not turn the call into a jump at -O2 any more than
at -O0, and both builds fault at the same point. With the loop the same
run reads from a constant depth -- 416 bytes at 60,000 pings and at a
million, where the wire cost is 7 MB.
A loop is the whole change. Pings are still answered one after another for
as long as a peer cares to send them, no configuration is involved, and
the rest of the body is untouched apart from its indentation.
The test asserts how far below its own frame the reader got rather than
waiting for a crash: 2,000 pings, four orders of magnitude short of
exhausting anything, take the unmodified reader 320,480 bytes deep against
a 64 kB ceiling, and the fix holds it constant.
Worth knowing for anyone reading this code: a Ping with no payload does
not reach this path at all, because readFrame does not consume the four
masking-key bytes of a zero-length masked frame and the next header is
parsed out of the masking key instead. That is a separate defect, filed
separately, which is why the numbers above use one-byte pings.
Client: cpp
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TWebSocketServer::readFrame read a frame's payload with a single call to
the transport underneath and treated anything short of the whole payload
as end of stream:
read = transport_->read(buffer, length);
readBuffer_.wroteBytes(read);
if (read < length) {
return false;
}
TSocket::read performs one recv() and returns whatever that produced, and
the transport factory hands TWebSocketServer the accepted TSocket itself,
so the payload arrives in as many pieces as the network chose. Returning
false makes readAll_virt return 0, which the caller reads as a closed
connection.
So no frame whose payload does not arrive in one read could be received at
all. Demonstrated with a TServerSocket, a TWebSocketServer over the
accepted socket, and a client writing a well-formed 4000-byte frame as
2000 + 2000 bytes with a pause between: readAll returned 0 of 4000. In
practice that is any frame past the path MTU, so this transport could not
carry a Thrift message beyond roughly 1.4 kB over ordinary Ethernet.
Read the payload with readAll(), which is how TFramedTransport reads its
own frames, and keep the existing signal for a peer that leaves in the
middle of one: readAll raises END_OF_FILE, which is caught and turned back
into the same "return false" the caller sees today. Nothing else about the
contract changes.
This waits for the number of bytes the frame declared, which is only safe
because THRIFT-6177 holds that number to TConfiguration::maxFrameSize; the
wait is bounded by that and by the socket's own receive timeout, the same
two limits that bound TFramedTransport.
Two tests. A frame delivered as 8 + 1000 + the rest now arrives whole,
where before it came back as 0 of 4000; a frame whose payload stops after
500 bytes is still reported as end of stream rather than handed over
half-read, which passes either way.
Client: cpp
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TWebSocketServer::readFrame sizes its read buffer from the payload length
the frame header declares, before a single payload byte has arrived:
// size_t is smaller than a ulong on a 32-bit system
if (payloadLength > UINT32_MAX) {
failConnection(CloseCode::MessageTooBig);
return false;
}
auto length = static_cast<uint32_t>(payloadLength);
if (length > 0) {
...
readBuffer_.resetBuffer(length);
uint8_t* buffer = readBuffer_.getWritePtr(length);
read = transport_->read(buffer, length);
UINT32_MAX is the only bound, so the fourteen bytes of a header carrying a
64-bit length decide the size of the allocation. Measured on a Linux x86-64
build: a header declaring 0xFFFFFFFF takes the process from a VmPeak of
10,512 kB to 4,205,092 kB and asks the transport underneath for
4,294,967,295 bytes, with no payload sent at all. resetBuffer() constructs
a fresh TMemoryBuffer of that size, so the allocation happens whether or
not the bytes ever turn up, and the buffer is a member that lives as long
as the connection does.
The transport has a TConfiguration and consults neither of its limits here.
Hold the declared length to TConfiguration::maxFrameSize, the ceiling
TFramedTransport::readFrame already applies to its own frames, and refuse
anything above it with close code 1009 Message Too Big, which is what this
transport already sends when a frame is too large for it.
Worth a release note: maxFrameSize defaults to 16384000, so a peer sending
a single WebSocket frame larger than that is refused where it was accepted
before. The bound follows whatever an operator sets.
Six tests, the first this transport has had. They assert the largest read
the server asked of the transport underneath it rather than merely that the
read failed: a payload that never arrives ends the frame either way, so
"did it fail?" passes on the unmodified library too. Two fail before the
change, 67108864 > 1024 and 32768 > 1024, and all six pass after; the other
four are regression guards -- an ordinary frame, a frame of exactly the
maximum, two frames in a row, and a length with the high bit set -- that
pass either way.
Client: cpp
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
