diff --git a/quic/api/QuicBatchWriter.cpp b/quic/api/QuicBatchWriter.cpp index abb2c4732..00d081384 100644 --- a/quic/api/QuicBatchWriter.cpp +++ b/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 batchWriter_; std::unique_ptr 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) diff --git a/quic/api/QuicTransportBase.cpp b/quic/api/QuicTransportBase.cpp index 1a7551913..70f5f2428 100644 --- a/quic/api/QuicTransportBase.cpp +++ b/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; } diff --git a/quic/api/test/QuicBatchWriterTest.cpp b/quic/api/test/QuicBatchWriterTest.cpp index 5d44a9928..deba69d10 100644 --- a/quic/api/test/QuicBatchWriterTest.cpp +++ b/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; - rawBuf->append(packetSizeTooBig); - EXPECT_TRUE(batchWriter->needsFlush(packetSizeTooBig)); + // There is a check against packet 10 bytes or more larger than the size limit + size_t packetSizeBig = 1009; + 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(1200, rawBuf->length()); + EXPECT_EQ(1009, 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(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,