From 924fff065f7376473ba597f53345edd66a240cf6 Mon Sep 17 00:00:00 2001 From: Daniel Sommermann Date: Wed, 4 Dec 2024 13:33:53 -0800 Subject: [PATCH] S471570 repro and fix Summary: Without the fixes to the state machine, this new test crashes in the same way as the SEV. See also https://fburl.com/logview/9t8mibce Credit to hanidamlaj for creating the repro. Reviewed By: afrind Differential Revision: D66457655 fbshipit-source-id: 2e75d636be273172a3b8c9bacb389f6f3b275fed --- proxygen/lib/http/session/HTTPTransaction.cpp | 6 +-- proxygen/lib/http/session/HTTPTransaction.h | 2 +- .../session/test/HQDownstreamSessionTest.cpp | 43 +++++++++++++++++++ .../test/HTTPDownstreamSessionTest.cpp | 36 ++++++++++++++-- 4 files changed, 77 insertions(+), 10 deletions(-) diff --git a/proxygen/lib/http/session/HTTPTransaction.cpp b/proxygen/lib/http/session/HTTPTransaction.cpp index c1a58266e6..930baa68ee 100644 --- a/proxygen/lib/http/session/HTTPTransaction.cpp +++ b/proxygen/lib/http/session/HTTPTransaction.cpp @@ -1447,11 +1447,7 @@ size_t HTTPTransaction::maybeSendDeferredNoError() { void HTTPTransaction::sendPadding(uint16_t bytes) { VLOG(4) << "egress padding=" << bytes << " on " << *this; - if (!validateEgressStateTransition( - // Sending padding is valid only when you can send body - HTTPTransactionEgressSM::Event::sendBody)) { - return; - } + // Padding is allowed at any time, even before headers and after EOM transport_.sendPadding(this, bytes); } diff --git a/proxygen/lib/http/session/HTTPTransaction.h b/proxygen/lib/http/session/HTTPTransaction.h index 6fa1e43d08..816f673b3f 100644 --- a/proxygen/lib/http/session/HTTPTransaction.h +++ b/proxygen/lib/http/session/HTTPTransaction.h @@ -1239,7 +1239,7 @@ class HTTPTransaction * is currently implemented only for HTTP/2 and HTTP/3 and will do nothing on * HTTP/1 connections. * - * sendPadding() may be called only when sendBody() is also valid to call. + * sendPadding() may be called at any time, even before headers and after EOM. * * @param bytes The number of bytes of padding to send on this transaction. * The actual serialized size of the padding will be greater than this number diff --git a/proxygen/lib/http/session/test/HQDownstreamSessionTest.cpp b/proxygen/lib/http/session/test/HQDownstreamSessionTest.cpp index 575b50ba6a..130b668e7b 100644 --- a/proxygen/lib/http/session/test/HQDownstreamSessionTest.cpp +++ b/proxygen/lib/http/session/test/HQDownstreamSessionTest.cpp @@ -604,6 +604,49 @@ TEST_P(HQDownstreamSessionTest, SimplePostWithPadding) { hqSession_->closeWhenIdle(); } +TEST_P(HQDownstreamSessionTest, PaddingSM) { + auto id = nextStreamId(); + auto res = requests_.emplace(std::piecewise_construct, + std::forward_as_tuple(id), + std::forward_as_tuple(makeCodec(id))); + auto& request = res.first->second; + request.id = request.codec->createStream(); + request.readEOF = false; + request.codec->generatePadding(request.buf, request.id, 1'000); + request.codec->generateHeader( + request.buf, request.id, getPostRequest(100), false); + request.codec->generatePadding(request.buf, request.id, 2'000); + request.codec->generateBody( + request.buf, request.id, makeBuf(100), HTTPCodec::NoPadding, true); + request.codec->generatePadding(request.buf, request.id, 4'000); + request.readEOF = true; + + auto handler = addSimpleStrictHandler(); + handler->expectHeaders(); + handler->expectBody([](uint64_t, std::shared_ptr body) { + EXPECT_EQ(body->computeChainDataLength(), 100); + }); + handler->expectEOM([&handler] { + handler->txn_->sendPadding(100); + handler->txn_->sendHeaders(getResponse(200)); + handler->txn_->sendPadding(100); + handler->txn_->sendChunkHeader(10); + handler->txn_->sendPadding(100); + handler->txn_->sendBody(makeBuf(10)); + handler->txn_->sendPadding(100); + handler->txn_->sendChunkTerminator(); + handler->txn_->sendPadding(100); + handler->txn_->sendEOM(); + handler->txn_->sendPadding(100); + }); + handler->expectDetachTransaction(); + flushRequestsAndLoop(); + EXPECT_GT(socketDriver_->streams_[id].readOffset, 7'100); + EXPECT_GT(socketDriver_->streams_[id].writeBuf.chainLength(), 610); + EXPECT_TRUE(socketDriver_->streams_[id].writeEOF); + hqSession_->closeWhenIdle(); +} + TEST_P(HQDownstreamSessionTest, SimpleGetEofDelay) { auto idh = checkRequest(); flushRequestsAndLoop(false, std::chrono::milliseconds(10)); diff --git a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp index fc280db71d..657eeaa62b 100644 --- a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp +++ b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp @@ -1114,11 +1114,33 @@ TEST_F(HTTPDownstreamSessionTest, BadContentLength) { TEST_F(HTTP2DownstreamSessionTest, FrameBasedPadding) { // Send a request with padding. Padding should not affect anything. auto handler = addSimpleStrictHandler(); - auto id = sendHeader(); - clientCodec_->generatePadding(requests_, id, 123); - clientCodec_->generateEOM(requests_, id); + + const auto streamID = clientCodec_->createStream(); + clientCodec_->generatePadding(requests_, streamID, 100); + clientCodec_->generateHeader( + requests_, streamID, getPostRequest(1'000), false); + clientCodec_->generatePadding(requests_, streamID, 200); + clientCodec_->generateBody( + requests_, streamID, makeBuf(1'000), HTTPCodec::NoPadding, false); + clientCodec_->generatePadding(requests_, streamID, 400); + clientCodec_->generateEOM(requests_, streamID); + clientCodec_->generatePadding(requests_, streamID, 800); + handler->expectHeaders(); - handler->expectEOM([&handler] { handler->sendReplyWithBody(200, 100); }); + handler->expectBody(); + handler->expectEOM([&handler] { + handler->txn_->sendPadding(100); + handler->txn_->sendHeaders(getResponse(200)); + handler->txn_->sendPadding(100); + handler->txn_->sendChunkHeader(10); + handler->txn_->sendPadding(100); + handler->txn_->sendBody(makeBuf(10)); + handler->txn_->sendPadding(100); + handler->txn_->sendChunkTerminator(); + handler->txn_->sendPadding(100); + handler->txn_->sendEOM(); + handler->txn_->sendPadding(100); + }); handler->expectGoaway(); flushRequestsAndLoopN(1); handler->expectDetachTransaction(); @@ -1130,6 +1152,12 @@ TEST_F(HTTP2DownstreamSessionTest, FrameBasedPadding) { InSequence enforceOrder; EXPECT_CALL(callbacks, onHeadersComplete(_, _)); + // No chunk header received, since it is not allowed in HTTP/2 + // EXPECT_CALL(callbacks, onChunkHeader(_, 10)); + EXPECT_CALL(callbacks, onBody(_, _, 0)); + // No chunk terminator received, since it is not allowed in HTTP/2 + // EXPECT_CALL(callbacks, onChunkComplete(_)); + EXPECT_CALL(callbacks, onMessageComplete(_, _)); parseOutput(*clientCodec_); }