Copilot commented on code in PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#discussion_r3966222956
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -162,10 +166,53 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
CHECK(settings_frame->get(Http3SettingsId::MAX_FIELD_SECTION_SIZE) ==
0x0400);
CHECK(settings_frame->get(Http3SettingsId::NUM_PLACEHOLDERS) == 0x0f);
+ // ~Http3Frame deallocates the reader through its MIOBuffer, so release the
+ // frames before freeing that buffer.
+ settings_frame.reset();
+ frame.reset();
free_MIOBuffer(input);
}
}
+// A SETTINGS identifier is a full QUIC variable-length integer, and an
+// identifier the implementation does not understand must be ignored
+// (RFC 9114, Section 7.2.4). Comparing a narrowed copy of the decoded
+// identifier against the known-ID set lets an unknown identifier alias a
+// known one.
+//
+// Identifiers of the form 0x1f * N + 0x21 are reserved to exercise that
+// requirement, and endpoints are expected to send one (RFC 9114, Section
+// 7.2.4.1). 0x1f * 33824 + 0x21 == 0x100001, which shares its low 16 bits
+// with HEADER_TABLE_SIZE (0x01).
+TEST_CASE("Load SETTINGS Frame ignores reserved identifier", "[http3]")
+{
+ uint8_t buf[] = {
+ 0x04, // Type
+ 0x05, // Length
+ 0x80, 0x10, 0x00, 0x01, // Identifier: QUIC varint encoding of 0x100001
+ 0x2a, // Value
+ };
+ MIOBuffer *input = new_MIOBuffer(BUFFER_SIZE_INDEX_128);
+ input->write(buf, sizeof(buf));
+ IOBufferReader *input_reader = input->alloc_reader();
+
+ std::shared_ptr<Http3Frame> frame = Http3FrameFactory::create(*input_reader);
+ frame->update();
+ REQUIRE(frame->type() == Http3FrameType::SETTINGS);
+
+ std::shared_ptr<Http3SettingsFrame> settings_frame =
std::dynamic_pointer_cast<Http3SettingsFrame>(frame);
+ REQUIRE(settings_frame);
+ REQUIRE(settings_frame->is_valid());
+
+ CHECK_FALSE(settings_frame->contains(Http3SettingsId::HEADER_TABLE_SIZE));
+
+ // ~Http3Frame deallocates the reader through its MIOBuffer, so release the
+ // frames before freeing that buffer.
+ settings_frame.reset();
+ frame.reset();
Review Comment:
This manual `reset()` pattern (plus explanatory comment) is now repeated in
multiple test cases. To reduce duplication and make lifetime ordering harder to
regress, consider using scopes to control destruction order (e.g., wrap frame
usage in an inner block so `frame`/`settings_frame` destruct before
`free_MIOBuffer(input)`), or introduce a small helper/RAII wrapper for the
buffer.
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -162,10 +166,53 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
CHECK(settings_frame->get(Http3SettingsId::MAX_FIELD_SECTION_SIZE) ==
0x0400);
CHECK(settings_frame->get(Http3SettingsId::NUM_PLACEHOLDERS) == 0x0f);
+ // ~Http3Frame deallocates the reader through its MIOBuffer, so release the
+ // frames before freeing that buffer.
+ settings_frame.reset();
+ frame.reset();
free_MIOBuffer(input);
}
}
+// A SETTINGS identifier is a full QUIC variable-length integer, and an
+// identifier the implementation does not understand must be ignored
+// (RFC 9114, Section 7.2.4). Comparing a narrowed copy of the decoded
+// identifier against the known-ID set lets an unknown identifier alias a
+// known one.
+//
+// Identifiers of the form 0x1f * N + 0x21 are reserved to exercise that
+// requirement, and endpoints are expected to send one (RFC 9114, Section
+// 7.2.4.1). 0x1f * 33824 + 0x21 == 0x100001, which shares its low 16 bits
+// with HEADER_TABLE_SIZE (0x01).
+TEST_CASE("Load SETTINGS Frame ignores reserved identifier", "[http3]")
Review Comment:
The PR description says the new test is tagged `[http3-settings-id-width]`,
but the added test is currently tagged only `[http3]`. If the tag is used for
targeted runs/filtering, please add it (e.g.,
`\"[http3][http3-settings-id-width]\"`) to match the description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
