[PR] THRIFT-6178: Read a WebSocket payload to the end in the C++ library [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G opened a new pull request, #3781:
URL: https://github.com/apache/thrift/pull/3781

   > **Stacked on #3780 (THRIFT-6177).** Only the second commit belongs to this ticket. Merge #3780 first and this rebases to a single commit.
   
   `TWebSocketServer::readFrame` reads a frame's payload with a single call to the transport underneath and treats anything short of the whole payload as end of stream:
   
   ```cpp
   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 can 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 them:
   
   ```
   client: payload 4000 bytes, sent as 2000 + 2000 with 300 ms between
   server: readAll returned 0 of 4000
   ```
   
   and with this change, over the same socket:
   
   ```
   server: readAll returned 4000 of 4000
   server: payload intact: yes
   ```
   
   In practice this is any frame past the path MTU, so the transport could not carry a Thrift message beyond roughly 1.4 kB over an ordinary Ethernet path.
   
   ### The fix
   
   Read the payload with `readAll()`, which is how `TFramedTransport::readFrame` 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.
   
   ### Why it is stacked
   
   `readAll()` waits for the number of bytes the frame declared. That is only safe once that number is bounded, which is what #3780 does — without it, a header declaring `0xFFFFFFFF` would make the server wait for 4 GB rather than return. With it, the wait is bounded by `TConfiguration::maxFrameSize` and by the socket's own receive timeout, the same two limits that bound `TFramedTransport`.
   
   ### Tests
   
   Two more cases in `lib/cpp/test/TWebSocketServerTest.cpp`:
   
   - a frame delivered as 8 + 1000 + the rest arrives whole — fails before the change with `0 != 4000`;
   - a frame whose payload stops after 500 bytes is still reported as end of stream rather than handed over half-read — passes either way, and is there so that waiting for the rest of a frame cannot quietly become handing a half-read one to the protocol.
   
   Full `bin/UnitTests`: 108 of 109. The one failure, `TServerSocketTest/test_bind_to_address`, is a pre-existing environment failure on this host and is present on `master`.
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] THRIFT-6178: Read a WebSocket payload to the end in the C++ library [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G commented on PR #3781:
URL: https://github.com/apache/thrift/pull/3781#issuecomment-5516097086

   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 3d98d7214, byte-identical to the commit here, so there is nothing left to merge.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] THRIFT-6178: Read a WebSocket payload to the end in the C++ library [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G closed pull request #3781: THRIFT-6178: Read a WebSocket payload to the end in the C++ library
URL: https://github.com/apache/thrift/pull/3781


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org