brbzull0 commented on code in PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#discussion_r3967741582
##########
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 description was updated before this review -- it now reads ``tagged
`[http3]``` and no longer mentions `http3-settings-id-width`, so the
description and the test agree.
Dropping the tag was deliberate, per review feedback on this PR: the other
15 `[http3]` cases under `src/proxy/http3/test/` use that tag alone, so a
case-specific tag here would be the sole exception. Nothing filters on it, so
re-adding it would break that consistency without buying anything.
##########
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:
Same suggestion as the earlier thread on this hunk, which is now resolved --
keeping the explicit `reset()` calls.
`free_MIOBuffer()` ends in `THREAD_FREE(mio, ioAllocator, this_thread())`,
and `~Http3Frame()` calls `_reader->dealloc()`, which is
`mbuf->dealloc_reader(this)` on that same buffer. An inner block or an RAII
wrapper enforces the ordering but records nothing about why it matters; the
comment is what carries that. All three load-path teardowns now use the same
note and the same ordering, so the repetition is a uniform shape rather than
drift. Reworking the teardown of pre-existing test bodies is out of scope for a
one-line parse fix.
--
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]
