From a7dc8cb4f7638b568e15fe4abcda5e384672c3c9 Mon Sep 17 00:00:00 2001
From: Yang Chi <yangchi@fb.com>
Date: Wed, 15 Jul 2020 11:17:45 -0700
Subject: [PATCH] Log an error when we write a packet larger than limit

Summary: as title

Reviewed By: bschlinker

Differential Revision: D22542572

fbshipit-source-id: 4f53a1c916b22e33f183208efb671c1d0883309c
---
 quic/api/QuicBatchWriter.cpp          |  7 +++---
 quic/api/QuicTransportFunctions.cpp   |  8 +++++++
 quic/api/test/QuicBatchWriterTest.cpp | 33 ---------------------------
 3 files changed, 12 insertions(+), 36 deletions(-)

diff --git a/quic/api/QuicBatchWriter.cpp b/quic/api/QuicBatchWriter.cpp
index eb943e78d..9f48c5588 100644
--- a/quic/api/QuicBatchWriter.cpp
+++ b/quic/api/QuicBatchWriter.cpp
@@ -290,9 +290,10 @@ 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);
+  if (diffToEnd >= conn_.udpSendPacketLen + kPacketSizeViolationTolerance) {
+    LOG(ERROR) << "Remaining buffer contents larger than udpSendPacketLen by "
+               << (diffToEnd - conn_.udpSendPacketLen);
+  }
   uint64_t diffToStart = lastPacketEnd_ - buf->data();
   buf->trimEnd(diffToEnd);
   auto bytesWritten = (numPackets_ > 1)
diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp
index 53a2c1b82..498604a93 100644
--- a/quic/api/QuicTransportFunctions.cpp
+++ b/quic/api/QuicTransportFunctions.cpp
@@ -267,6 +267,10 @@ DataPathResult continuousMemoryBuildScheduleEncrypt(
   // Include previous packets back.
   packetBuf->prepend(prevSize);
   connection.bufAccessor->release(std::move(packetBuf));
+  if (encodedSize > connection.udpSendPacketLen) {
+    LOG(ERROR) << "Quic sending pkt larger than limit, encodedSize="
+               << encodedSize;
+  }
   // TODO: I think we should add an API that doesn't need a buffer.
   bool ret = ioBufBatch.write(nullptr /* no need to pass buf */, encodedSize);
   // update stats and connection
@@ -338,6 +342,10 @@ DataPathResult iobufChainBasedBuildScheduleEncrypt(
       packetBuf->length() - headerLen,
       headerCipher);
   auto encodedSize = packetBuf->computeChainDataLength();
+  if (encodedSize > connection.udpSendPacketLen) {
+    LOG(ERROR) << "Quic sending pkt larger than limit, encodedSize="
+               << encodedSize;
+  }
   bool ret = ioBufBatch.write(std::move(packetBuf), encodedSize);
   if (ret) {
     // update stats and connection
diff --git a/quic/api/test/QuicBatchWriterTest.cpp b/quic/api/test/QuicBatchWriterTest.cpp
index deba69d10..5ea2cfc13 100644
--- a/quic/api/test/QuicBatchWriterTest.cpp
+++ b/quic/api/test/QuicBatchWriterTest.cpp
@@ -624,39 +624,6 @@ TEST_P(QuicBatchWriterTest, InplaceWriterBufResidueCheck) {
   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,