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

Author: dcsommer

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);
 }
 

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

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<folly::IOBuf> 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));

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_);
 }