add a check for Quic inplace writer for the remaining buffer size

Summary:
as title. Instead of checking against the packet size limit, this
leaves a 10 bytes room since we have a bug that writes out packets that's
slightly larger than udpSendPacketLen. Most of such packet will be 1-2 bytes
larger than original packets.

Reviewed By: mjoras

Differential Revision: D21642386

fbshipit-source-id: 6ca68d48828cb7f8ee692e0d5f452f5389a56bfd

quic/api/QuicBatchWriter.cpp

@@ -14,8 +14,13 @@
 #define USE_THREAD_LOCAL_BATCH_WRITER 0
 #endif
 
-#if USE_THREAD_LOCAL_BATCH_WRITER
 namespace {
+// There is a known problem in the CloningScheduler that it may write a packet
+// that's a few bytes larger than the original packet. If the original packet is
+// a full packet, then the new packet will be larger than a full packet.
+constexpr size_t kPacketSizeViolationTolerance = 10;
+
+#if USE_THREAD_LOCAL_BATCH_WRITER
 class ThreadLocalBatchWriterCache : public folly::AsyncTimeout {
  private:
   ThreadLocalBatchWriterCache() = default;
@@ -128,8 +133,8 @@ class ThreadLocalBatchWriterCache : public folly::AsyncTimeout {
   std::unique_ptr<quic::BatchWriter> batchWriter_;
   std::unique_ptr<folly::AsyncUDPSocket> socket_;
 };
-} // namespace
 #endif
+} // namespace
 
 namespace quic {
 // BatchWriter
@@ -285,6 +290,9 @@ ssize_t GSOInplacePacketBatchWriter::write(
       (nextPacketSize_ && diffToEnd == nextPacketSize_))
       << "diffToEnd=" << diffToEnd << ", pktLimit=" << conn_.udpSendPacketLen
       << ", nextPacketSize_=" << nextPacketSize_;
+  CHECK_LT(diffToEnd, conn_.udpSendPacketLen + kPacketSizeViolationTolerance)
+      << "Remaining buffer contents larger than udpSendPacketLen by "
+      << (diffToEnd - conn_.udpSendPacketLen);
   uint64_t diffToStart = lastPacketEnd_ - buf->data();
   buf->trimEnd(diffToEnd);
   auto bytesWritten = (numPackets_ > 1)

quic/api/QuicTransportBase.cpp

@@ -2441,6 +2441,9 @@ void QuicTransportBase::setTransportSettings(
   // If we've already encoded the transport parameters, silently return as
   // setting the transport settings again would be buggy.
   // TODO should we throw or return Expected here?
+  if (conn_->nodeType == QuicNodeType::Client) {
+    conn_->transportSettings.dataPathType = DataPathType::ChainedMemory;
+  }
   if (conn_->transportParametersEncoded) {
     return;
   }

quic/api/test/QuicBatchWriterTest.cpp

@@ -606,9 +606,10 @@ TEST_P(QuicBatchWriterTest, InplaceWriterBufResidueCheck) {
   ASSERT_FALSE(
       batchWriter->append(nullptr, 700, folly::SocketAddress(), nullptr));
 
-  size_t packetSizeTooBig = 1200;
+  // There is a check against packet 10 bytes or more larger than the size limit
-  rawBuf->append(packetSizeTooBig);
+  size_t packetSizeBig = 1009;
-  EXPECT_TRUE(batchWriter->needsFlush(packetSizeTooBig));
+  rawBuf->append(packetSizeBig);
+  EXPECT_TRUE(batchWriter->needsFlush(packetSizeBig));
 
   EXPECT_CALL(sock, write(_, _))
       .Times(1)
@@ -619,11 +620,43 @@ TEST_P(QuicBatchWriterTest, InplaceWriterBufResidueCheck) {
       }));
   // No crash:
   EXPECT_EQ(700, batchWriter->write(sock, folly::SocketAddress()));
-
+  EXPECT_EQ(1009, rawBuf->length());
-  EXPECT_EQ(1200, rawBuf->length());
   EXPECT_EQ(0, rawBuf->headroom());
 }
 
+TEST_P(QuicBatchWriterTest, InplaceWriterBufResidueTooBig) {
+  bool useThreadLocal = GetParam();
+  folly::EventBase evb;
+  folly::test::MockAsyncUDPSocket sock(&evb);
+  EXPECT_CALL(sock, getGSO()).WillRepeatedly(Return(1));
+
+  uint32_t batchSize = 20;
+  auto bufAccessor =
+      std::make_unique<SimpleBufAccessor>(conn_.udpSendPacketLen * batchSize);
+  conn_.bufAccessor = bufAccessor.get();
+  conn_.udpSendPacketLen = 1000;
+  auto batchWriter = quic::BatchWriterFactory::makeBatchWriter(
+      sock,
+      quic::QuicBatchingMode::BATCHING_MODE_GSO,
+      batchSize,
+      useThreadLocal,
+      quic::kDefaultThreadLocalDelay,
+      DataPathType::ContinuousMemory,
+      conn_);
+  auto buf = bufAccessor->obtain();
+  folly::IOBuf* rawBuf = buf.get();
+  bufAccessor->release(std::move(buf));
+  rawBuf->append(700);
+  ASSERT_FALSE(
+      batchWriter->append(nullptr, 700, folly::SocketAddress(), nullptr));
+
+  size_t packetSizeTooBig = 1090;
+  rawBuf->append(packetSizeTooBig);
+  EXPECT_TRUE(batchWriter->needsFlush(packetSizeTooBig));
+
+  EXPECT_DEATH(batchWriter->write(sock, folly::SocketAddress()), "");
+}
+
 INSTANTIATE_TEST_CASE_P(
     QuicBatchWriterTest,
     QuicBatchWriterTest,