diff --git a/quic/api/QuicPacketScheduler.cpp b/quic/api/QuicPacketScheduler.cpp index f085413fa..8e11c6dd8 100644 --- a/quic/api/QuicPacketScheduler.cpp +++ b/quic/api/QuicPacketScheduler.cpp @@ -358,7 +358,7 @@ bool StreamFrameScheduler::writeStreamLossBuffers( buffer != stream.lossBuffer.cend(); ++buffer) { auto bufferLen = buffer->data.chainLength(); - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( builder, stream.id, buffer->offset, @@ -367,6 +367,11 @@ bool StreamFrameScheduler::writeStreamLossBuffers( buffer->eof, none /* skipLenHint */, stream.groupId); + if (res.hasError()) { + throw QuicInternalException( + res.error().message, *res.error().code.asLocalErrorCode()); + } + auto dataLen = *res; if (dataLen) { wroteStreamFrame = true; writeStreamFrameData(builder, buffer->data, *dataLen); @@ -528,7 +533,7 @@ bool StreamFrameScheduler::writeStreamFrame( bool canWriteFin = stream.finalWriteOffset.has_value() && bufferLen <= flowControlLen && stream.writeBufMeta.offset == 0; auto writeOffset = stream.currentWriteOffset; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( builder, stream.id, writeOffset, @@ -537,6 +542,11 @@ bool StreamFrameScheduler::writeStreamFrame( canWriteFin, none /* skipLenHint */, stream.groupId); + if (res.hasError()) { + throw QuicInternalException( + res.error().message, *res.error().code.asLocalErrorCode()); + } + auto dataLen = *res; if (!dataLen) { return false; } diff --git a/quic/codec/BUCK b/quic/codec/BUCK index c3ea6f81b..0104d55dd 100644 --- a/quic/codec/BUCK +++ b/quic/codec/BUCK @@ -94,7 +94,6 @@ mvfst_cpp_library( "-fstrict-aliasing", ], deps = [ - "//folly:string", "//quic:exception", ], exported_deps = [ @@ -163,7 +162,6 @@ mvfst_cpp_library( ], deps = [ ":decode", - "//quic:exception", ], exported_deps = [ ":packet_number", diff --git a/quic/codec/QuicPacketRebuilder.cpp b/quic/codec/QuicPacketRebuilder.cpp index f94012903..5ac788a26 100644 --- a/quic/codec/QuicPacketRebuilder.cpp +++ b/quic/codec/QuicPacketRebuilder.cpp @@ -91,7 +91,7 @@ Optional PacketRebuilder::rebuildFromPacket( if (stream && retransmittable(*stream)) { auto streamData = cloneRetransmissionBuffer(streamFrame, stream); auto bufferLen = streamData ? streamData->chainLength() : 0; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( builder_, streamFrame.streamId, streamFrame.offset, @@ -103,6 +103,11 @@ Optional PacketRebuilder::rebuildFromPacket( // frame last we need to end the stream frame in that case. lastFrame && bufferLen && !hasAckFrame, streamFrame.streamGroupId); + if (res.hasError()) { + throw QuicInternalException( + res.error().message, *res.error().code.asLocalErrorCode()); + } + auto dataLen = *res; bool ret = dataLen.has_value() && *dataLen == streamFrame.len; if (ret) { // Writing 0 byte for stream data is legit if the stream frame has diff --git a/quic/codec/QuicWriteCodec.cpp b/quic/codec/QuicWriteCodec.cpp index 9acaf10c9..c52fa7dee 100644 --- a/quic/codec/QuicWriteCodec.cpp +++ b/quic/codec/QuicWriteCodec.cpp @@ -28,7 +28,7 @@ bool packetSpaceCheck(uint64_t limit, size_t require) { namespace quic { -Optional writeStreamFrameHeader( +folly::Expected, QuicError> writeStreamFrameHeader( PacketBuilderInterface& builder, StreamId id, uint64_t offset, @@ -42,9 +42,9 @@ Optional writeStreamFrameHeader( return none; } if (writeBufferLen == 0 && !fin) { - throw QuicInternalException( - "No data or fin supplied when writing stream.", - LocalErrorCode::INTERNAL_ERROR); + return folly::makeUnexpected(QuicError( + LocalErrorCode::INTERNAL_ERROR, + "No data or fin supplied when writing stream.")); } StreamTypeField::Builder streamTypeBuilder; if (streamGroupId) { diff --git a/quic/codec/QuicWriteCodec.h b/quic/codec/QuicWriteCodec.h index e79634bef..b957708ba 100644 --- a/quic/codec/QuicWriteCodec.h +++ b/quic/codec/QuicWriteCodec.h @@ -49,7 +49,7 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder); * to decide it. When skipLenHint is true, the field is skipped. When it's * false, it will be encoded into the header. */ -Optional writeStreamFrameHeader( +folly::Expected, QuicError> writeStreamFrameHeader( PacketBuilderInterface& builder, StreamId id, uint64_t offset, diff --git a/quic/codec/test/QuicWriteCodecTest.cpp b/quic/codec/test/QuicWriteCodecTest.cpp index fbd1b9f05..2414a7f0d 100644 --- a/quic/codec/test/QuicWriteCodecTest.cpp +++ b/quic/codec/test/QuicWriteCodecTest.cpp @@ -496,8 +496,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameToEmptyPacket) { StreamId streamId = 1; uint64_t offset = 0; bool fin = false; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 10, 10, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, 10); writeStreamFrameData(pktBuilder, inputBuf->clone(), 10); @@ -542,8 +544,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameToPartialPacket) { // 4 bytes offset // 1 byte for length // => 8 bytes of header - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 20, 20, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); EXPECT_EQ(*dataLen, 20); writeStreamFrameData(pktBuilder, inputBuf->clone(), 20); @@ -591,8 +595,10 @@ TEST_F(QuicWriteCodecTest, WriteTwoStreamFrames) { uint64_t offset1 = 65535; bool fin1 = false; auto inputBuf = buildRandomInputData(30); - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId1, offset1, 30, 30, fin1, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, 30); writeStreamFrameData(pktBuilder, inputBuf->clone(), 30); @@ -612,7 +618,7 @@ TEST_F(QuicWriteCodecTest, WriteTwoStreamFrames) { // 2 bytes for stream // 4 bytes for offset // => 7 bytes - dataLen = writeStreamFrameHeader( + res = writeStreamFrameHeader( pktBuilder, streamId2, offset2, @@ -620,6 +626,8 @@ TEST_F(QuicWriteCodecTest, WriteTwoStreamFrames) { remainingSpace, fin2, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, remainingSpace - 7); writeStreamFrameData(pktBuilder, inputBuf2->clone(), remainingSpace - 7); @@ -688,8 +696,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFramePartialData) { // 2 bytes for stream id // 4 bytes for offset // => 7 bytes for header - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 50, 50, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, 33); writeStreamFrameData(pktBuilder, inputBuf->clone(), 33); @@ -727,8 +737,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameTooSmallForStreamHeader) { StreamId streamId = 1; uint64_t offset = 65535; bool fin = false; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 1, 1, fin, none /* skipLenHint */); + EXPECT_TRUE(res.hasValue()); + auto dataLen = *res; EXPECT_FALSE(dataLen); EXPECT_EQ(1, pktBuilder.remainingSpaceInPkt()); } @@ -745,8 +757,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamNoSpaceForData) { // 1 byte for stream id // 1 byte for offset // => 3 bytes - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 10, 10, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; EXPECT_FALSE(dataLen.has_value()); EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 3); } @@ -767,8 +781,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamSpaceForOneByte) { // 1 byte for stream id // 1 byte for offset // => 3 bytes - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 100, 100, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, 1); writeStreamFrameData(pktBuilder, inputBuf->clone(), 1); @@ -813,8 +829,10 @@ TEST_F(QuicWriteCodecTest, WriteFinToEmptyPacket) { StreamId streamId = 1; uint64_t offset = 0; bool fin = true; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 10, 10, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, 10); writeStreamFrameData(pktBuilder, inputBuf->clone(), 10); @@ -864,7 +882,7 @@ TEST_F(QuicWriteCodecTest, TestWriteIncompleteDataAndFin) { StreamId streamId = 1; uint64_t offset = 0; bool fin = true; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, @@ -872,6 +890,8 @@ TEST_F(QuicWriteCodecTest, TestWriteIncompleteDataAndFin) { inDataSize, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); EXPECT_LT(*dataLen, inDataSize); } @@ -887,8 +907,10 @@ TEST_F(QuicWriteCodecTest, TestWriteNoDataAndFin) { uint64_t offset = 0; bool fin = true; Buf empty; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); EXPECT_EQ(*dataLen, 0); } @@ -900,10 +922,14 @@ TEST_F(QuicWriteCodecTest, TestWriteNoDataAndNoFin) { uint64_t offset = 0; bool fin = false; Buf empty; - EXPECT_THROW( - writeStreamFrameHeader( - pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */), - QuicInternalException); + auto res = writeStreamFrameHeader( + pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */); + EXPECT_TRUE(res.hasError()); + EXPECT_EQ( + res.error(), + QuicError( + LocalErrorCode::INTERNAL_ERROR, + "No data or fin supplied when writing stream.")); } TEST_F(QuicWriteCodecTest, PacketOnlyHasSpaceForStreamHeader) { @@ -919,8 +945,10 @@ TEST_F(QuicWriteCodecTest, PacketOnlyHasSpaceForStreamHeader) { StreamId streamId = 1; uint64_t offset = 0; bool fin = true; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 20, 20, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; EXPECT_FALSE(dataLen.has_value()); EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 2); } @@ -936,8 +964,10 @@ TEST_F(QuicWriteCodecTest, PacketOnlyHasSpaceForStreamHeaderWithFin) { StreamId streamId = 1; uint64_t offset = 0; bool fin = true; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen.has_value()); EXPECT_EQ(*dataLen, 0); EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 0); @@ -954,8 +984,10 @@ TEST_F(QuicWriteCodecTest, PacketNotEnoughSpaceForStreamHeaderWithFin) { StreamId streamId = 1; uint64_t offset = 0; bool fin = true; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 0, 0, fin, none /* skipLenHint */); + EXPECT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_FALSE(dataLen.has_value()); EXPECT_EQ(pktBuilder.remainingSpaceInPkt(), 2); } @@ -980,8 +1012,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeSkipLen) { StreamId streamId = 0; uint64_t offset = 10; bool fin = false; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 1200 * 2, 1200 * 2, fin, none); + EXPECT_TRUE(res.hasValue()); + auto dataLen = *res; EXPECT_LT(*dataLen, 1200); } @@ -1005,8 +1039,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeNotSkipLen) { StreamId streamId = 0; uint64_t offset = 10; bool fin = false; - auto dataLen = + auto res = writeStreamFrameHeader(pktBuilder, streamId, offset, 200, 200, fin, none); + EXPECT_TRUE(res.hasValue()); + auto dataLen = *res; EXPECT_EQ(*dataLen, 200); } @@ -1030,8 +1066,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeLengthHintTrue) { StreamId streamId = 0; uint64_t offset = 10; bool fin = false; - auto dataLen = + auto res = writeStreamFrameHeader(pktBuilder, streamId, offset, 200, 200, fin, true); + EXPECT_TRUE(res.hasValue()); + auto dataLen = *res; EXPECT_EQ(*dataLen, 200); } @@ -1055,8 +1093,10 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameHeadeLengthHintFalse) { StreamId streamId = 0; uint64_t offset = 10; bool fin = false; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, 1200 * 2, 1200 * 2, fin, false); + EXPECT_TRUE(res.hasValue()); + auto dataLen = *res; EXPECT_LT(*dataLen, 1200); } @@ -2587,7 +2627,7 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameWithGroup) { uint64_t offset = 0; bool fin = true; - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( pktBuilder, streamId, offset, @@ -2596,6 +2636,8 @@ TEST_F(QuicWriteCodecTest, WriteStreamFrameWithGroup) { fin, none /* skipLenHint */, groupId); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, 50); writeStreamFrameData(pktBuilder, inputBuf->clone(), 50); diff --git a/quic/common/test/TestUtils.cpp b/quic/common/test/TestUtils.cpp index 78ebc6622..db7b79293 100644 --- a/quic/common/test/TestUtils.cpp +++ b/quic/common/test/TestUtils.cpp @@ -305,7 +305,7 @@ RegularQuicPacketBuilder::Packet createStreamPacket( } builder->encodePacketHeader(); builder->accountForCipherOverhead(cipherOverhead); - auto dataLen = *writeStreamFrameHeader( + auto res = *writeStreamFrameHeader( *builder, streamId, offset, @@ -313,6 +313,8 @@ RegularQuicPacketBuilder::Packet createStreamPacket( data.computeChainDataLength(), eof, none /* skipLenHint */); + CHECK(res.hasValue()) << "failed to write stream frame header"; + auto dataLen = *res; auto dataBuf = data.clone(); writeStreamFrameData( *builder, diff --git a/quic/dsr/backend/DSRPacketizer.cpp b/quic/dsr/backend/DSRPacketizer.cpp index 0e82a6df2..72bdb760a 100644 --- a/quic/dsr/backend/DSRPacketizer.cpp +++ b/quic/dsr/backend/DSRPacketizer.cpp @@ -42,7 +42,7 @@ bool PacketGroupWriter::writeSingleQuicPacket( builder.accountForCipherOverhead(aead.getCipherOverhead()); // frontend has already limited the length to flow control, thus // flowControlLen == length - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( builder, streamId, offset, @@ -52,6 +52,11 @@ bool PacketGroupWriter::writeSingleQuicPacket( true /* skip length field in stream header */, std::nullopt, /* stream group id */ false /* don't append frame to builder */); + if (res.hasError()) { + throw QuicInternalException( + res.error().message, *res.error().code.asLocalErrorCode()); + } + auto dataLen = *res; ChainedByteRangeHead chainedByteRangeHead(buf); writeStreamFrameData(builder, chainedByteRangeHead, *dataLen); auto packet = std::move(builder).buildPacket(); diff --git a/quic/server/test/QuicServerTest.cpp b/quic/server/test/QuicServerTest.cpp index f5140f4a1..82e0f70d6 100644 --- a/quic/server/test/QuicServerTest.cpp +++ b/quic/server/test/QuicServerTest.cpp @@ -1461,7 +1461,7 @@ auto createInitialStream( 0 /* largestAcked */); builder.encodePacketHeader(); auto streamData = data.clone(); - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( builder, streamId, 0, @@ -1469,6 +1469,8 @@ auto createInitialStream( streamData->computeChainDataLength(), true, none /* skipLenHint */); + EXPECT_TRUE(res.hasValue()); + auto dataLen = *res; EXPECT_TRUE(dataLen); writeStreamFrameData(builder, std::move(streamData), *dataLen); return packetToBuf(std::move(builder).buildPacket()); diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 33286f989..fca8b3c50 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -87,7 +87,7 @@ TEST_F(QuicServerTransportTest, TestReadMultipleStreams) { auto buf1 = IOBuf::copyBuffer("Aloha"); auto buf2 = IOBuf::copyBuffer("Hello"); - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( builder, 0x08, 0, @@ -95,11 +95,13 @@ TEST_F(QuicServerTransportTest, TestReadMultipleStreams) { buf1->computeChainDataLength(), true, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, buf1->computeChainDataLength()); writeStreamFrameData(builder, buf1->clone(), buf1->computeChainDataLength()); - dataLen = writeStreamFrameHeader( + res = writeStreamFrameHeader( builder, 0x0C, 0, @@ -107,6 +109,8 @@ TEST_F(QuicServerTransportTest, TestReadMultipleStreams) { buf1->computeChainDataLength(), true, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + dataLen = *res; ASSERT_TRUE(dataLen); ASSERT_EQ(*dataLen, buf1->computeChainDataLength()); writeStreamFrameData(builder, buf2->clone(), buf2->computeChainDataLength()); @@ -1058,7 +1062,7 @@ TEST_F(QuicServerTransportTest, RecvStopSendingFrameAfterHalfCloseRemote) { StopSendingFrame stopSendingFrame( streamId, GenericApplicationErrorCode::UNKNOWN); ASSERT_TRUE(builder.canBuildPacket()); - auto dataLen = writeStreamFrameHeader( + auto res = writeStreamFrameHeader( builder, 0x00, stream->currentReadOffset, @@ -1066,6 +1070,8 @@ TEST_F(QuicServerTransportTest, RecvStopSendingFrameAfterHalfCloseRemote) { 10, true, none /* skipLenHint */); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; ASSERT_TRUE(dataLen.has_value()); ASSERT_EQ(*dataLen, 0); writeFrame(QuicSimpleFrame(stopSendingFrame), builder); @@ -3789,7 +3795,7 @@ TEST_F( // add some data auto data = IOBuf::copyBuffer("hello!"); - auto dataLen = *writeStreamFrameHeader( + auto res = *writeStreamFrameHeader( builder, /*id=*/4, /*offset=*/0, @@ -3797,6 +3803,8 @@ TEST_F( data->computeChainDataLength(), /*fin=*/true, /*skipLenHint=*/none); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; writeStreamFrameData( builder, data->clone(), @@ -3830,7 +3838,7 @@ TEST_F( // add some data auto data = IOBuf::copyBuffer("hello!"); - auto dataLen = *writeStreamFrameHeader( + auto res = *writeStreamFrameHeader( builder, /*id=*/4, /*offset=*/0, @@ -3838,6 +3846,8 @@ TEST_F( data->computeChainDataLength(), /*eof=*/true, /*skipLenHint=*/none); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; writeStreamFrameData( builder, data->clone(), @@ -3871,7 +3881,7 @@ TEST_F( // add some data auto data = IOBuf::copyBuffer("hello!"); - auto dataLen = *writeStreamFrameHeader( + auto res = *writeStreamFrameHeader( builder, /*id=*/4, /*offset=*/0, @@ -3879,6 +3889,8 @@ TEST_F( data->computeChainDataLength(), /*eof=*/true, /*skipLenHint=*/none); + ASSERT_TRUE(res.hasValue()); + auto dataLen = *res; writeStreamFrameData( builder, data->clone(),