Skip to content

THRIFT-6177: Bound the WebSocket frame payload length in the C++ library - #3780

Closed
Jens-G wants to merge 1 commit into
apache:masterfrom
Jens-G:THRIFT-6177
Closed

THRIFT-6177: Bound the WebSocket frame payload length in the C++ library#3780
Jens-G wants to merge 1 commit into
apache:masterfrom
Jens-G:THRIFT-6177

Conversation

@Jens-G

@Jens-G Jens-G commented Sep 2, 2026

Copy link
Copy Markdown
Member

TWebSocketServer::readFrame sizes its read buffer from the payload length the frame header declares, before a single payload byte has arrived. 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 on this path. This holds the declared length to TConfiguration::maxFrameSize — the ceiling TFramedTransport::readFrame already applies to its own frames — and refuses 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.

Compatibility — 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.

Tests

lib/cpp/test/TWebSocketServerTest.cpp, the first tests 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. All six pass after.
  • Four are regression guards that pass either way: an ordinary frame, a frame of exactly the maximum, two frames in a row, and a length with the high bit set.
  • Full bin/UnitTests: 106 of 107 before and after. The one failure, TServerSocketTest/test_bind_to_address, is a pre-existing environment failure on this host, unrelated and present on master.

The test is registered under if(OPENSSL_FOUND AND WITH_OPENSSL) in CMake, because that is the condition under which TWebSocketServer.cpp is built at all; the autotools build compiles it unconditionally, so Makefile.am needs no guard.

Separable, found while checking this and deliberately not in scope

readFrame reads the payload with a single transport_->read() and treats a short read as end of stream. Verified by execution: an ordinary 40-byte frame delivered as 20 + 20 bytes makes readAll return 0, so any frame whose payload does not arrive in one read is silently dropped. That is a separate correctness defect, it changes blocking behaviour to fix, and the bound added here is the precondition for fixing it safely — readAll on an unbounded declared length would wait for 4 GiB. It will follow as its own ticket.

🤖 Generated with Claude Code

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>
@Jens-G

Jens-G commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

Superseded by the rebase-merge of #3782, which carried this commit into master along with the rest of the stack. The change is on master as 9189ba0, byte-identical to the commit here, so there is nothing left to merge.

@Jens-G Jens-G closed this Sep 2, 2026
@Jens-G
Jens-G deleted the THRIFT-6177 branch September 2, 2026 20:45
@Kunal8954

Copy link
Copy Markdown

wht ai do you used to write code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build and general CI cmake, automake and build system changes c++ Pull requests that update C++ code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants