You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "brbzull0 (via GitHub)" <gi...@apache.org> on 2026/09/03 11:08:50 UTC
[PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
brbzull0 opened a new pull request, #13627:
URL: https://github.com/apache/trafficserver/pull/13627
`Http3SettingsFrame::_parse()` decodes each SETTINGS identifier with
`QUICIntUtil::read_QUICVariableInt()`, which returns `uint64_t`, but stored the
result in a `uint16_t` local (`src/proxy/http3/Http3Frame.cc:459`). RFC 9114
defines SETTINGS identifiers as variable-length integers and requires
unrecognized identifiers to be ignored; narrowing the decoded value before the
comparison against the known-ID set means an identifier outside that set can
alias a known one on its low 16 bits.
This widens the local to `uint64_t`. Nothing else changes -- the surrounding
parse loop and the known-ID comparison are untouched.
### Test
Adds `Load SETTINGS Frame ignores wide unknown identifier` to
`test_Http3Frame.cc`, tagged `[http3-settings-id-width]`. It feeds a SETTINGS
frame whose identifier is the 4-byte varint encoding of `0x10001` -- sharing its
low 16 bits with `HEADER_TABLE_SIZE` (`0x01`) -- and asserts the frame parses as
valid while *not* containing `HEADER_TABLE_SIZE`.
Confirmed this is a regression test and not a tautology: with the `uint16_t`
restored it fails on `CHECK_FALSE(contains(HEADER_TABLE_SIZE))` (expansion
`!true`); with the fix it passes, 4 assertions.
Also run: full `test_http3` (138 assertions / 16 cases, up from 134/15 on
master) and the `h3_proxy_verifier` autest.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#discussion_r3932246346
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -166,6 +166,39 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
}
}
+// A SETTINGS identifier is a full QUIC variable-length integer (up to 62 bits).
+// Parsing must compare the decoded identifier against the known-ID set at full
+// width; narrowing the decoded value into a smaller integer type can cause an
+// identifier outside the known set to be misinterpreted as a known one.
+// Here, encoding 0x10001 as a 4-byte QUIC varint (0x80 0x01 0x00 0x01) shares
+// its low 16 bits with HEADER_TABLE_SIZE (0x01). The parser must treat the
+// identifier as unknown and skip the setting rather than store it under
+// HEADER_TABLE_SIZE.
+TEST_CASE("Load SETTINGS Frame ignores wide unknown identifier", "[http3][http3-settings-id-width]")
+{
+ uint8_t buf[] = {
+ 0x04, // Type
+ 0x05, // Length
+ 0x80, 0x01, 0x00, 0x01, // Identifier: QUIC varint encoding of 0x10001
+ 0x2a, // Value (1-byte QUIC varint = 42)
+ };
+ 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));
+
+ free_MIOBuffer(input);
+}
Review Comment:
`free_MIOBuffer(input)` is called while `frame` / `settings_frame` still own the `IOBufferReader` allocated from that `MIOBuffer`. Since `Http3Frame::~Http3Frame()` calls `_reader->dealloc()`, freeing the `MIOBuffer` first can lead to a use-after-free / nullptr dereference in the frame destructor (depending on optimization and allocator reuse). Reset the shared_ptrs before freeing the buffer so the reader is deallocated while the `MIOBuffer` is still alive.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#issuecomment-5599670379
[approve ci autest 2]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 merged PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#discussion_r3932861584
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -166,6 +166,43 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
}
}
+// A SETTINGS identifier is a full QUIC variable-length integer (up to 62 bits).
+// Parsing must compare the decoded identifier against the known-ID set at full
+// width; narrowing the decoded value into a smaller integer type can cause an
+// identifier outside the known set to be misinterpreted as a known one.
+// Here, encoding 0x10001 as a 4-byte QUIC varint (0x80 0x01 0x00 0x01) shares
+// its low 16 bits with HEADER_TABLE_SIZE (0x01). The parser must treat the
+// identifier as unknown and skip the setting rather than store it under
+// HEADER_TABLE_SIZE.
+TEST_CASE("Load SETTINGS Frame ignores wide unknown identifier", "[http3][http3-settings-id-width]")
+{
+ uint8_t buf[] = {
+ 0x04, // Type
+ 0x05, // Length
+ 0x80, 0x01, 0x00, 0x01, // Identifier: QUIC varint encoding of 0x10001
+ 0x2a, // Value (1-byte QUIC varint = 42)
+ };
+ 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 it holds, so drop the frames before the
+ // MIOBuffer that reader belongs to.
+ settings_frame.reset();
+ frame.reset();
+ free_MIOBuffer(input);
Review Comment:
Several other tests in this file free the source MIOBuffer while an Http3Frame (which deallocates its IOBufferReader in ~Http3Frame) is still alive. This can lead to use-after-free when the frame is destroyed after the buffer has been freed. This new test correctly resets the frames before free_MIOBuffer(); please apply the same teardown order to the earlier "Load DATA Frame" and "Load SETTINGS Frame" sections (and any similar cases) so the test suite is not relying on destructor order or MIOBuffer internals.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#discussion_r3960222558
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -162,10 +166,51 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
CHECK(settings_frame->get(Http3SettingsId::MAX_FIELD_SECTION_SIZE) == 0x0400);
CHECK(settings_frame->get(Http3SettingsId::NUM_PLACEHOLDERS) == 0x0f);
+ 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:
Manual `reset()` calls and repeated explanatory comments are now duplicated across multiple tests. Consider restructuring these tests to rely on scope-based lifetimes instead (e.g., put `frame`/`settings_frame` in an inner scope and call `free_MIOBuffer(input);` after that scope). This reduces repetition and makes the destruction order harder to get wrong in future edits.
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -162,10 +166,51 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
CHECK(settings_frame->get(Http3SettingsId::MAX_FIELD_SECTION_SIZE) == 0x0400);
CHECK(settings_frame->get(Http3SettingsId::NUM_PLACEHOLDERS) == 0x0f);
+ 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
Review Comment:
The PR description says the test is tagged `[http3-settings-id-width]` and uses the 4-byte varint encoding of `0x10001`, but the added test is tagged `[http3]` and encodes `0x100001` (as also stated in the inline comment). Please align the PR description with the actual test (or adjust the test/tag/value) to avoid confusion when running or referencing this regression.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#discussion_r3976723928
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -166,6 +166,43 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
}
}
+// A SETTINGS identifier is a full QUIC variable-length integer (up to 62 bits).
+// Parsing must compare the decoded identifier against the known-ID set at full
+// width; narrowing the decoded value into a smaller integer type can cause an
Review Comment:
Done in b1342b11fe. The identifier is now `0x1f * 33824 + 0x21 == 0x100001`, encoded as `0x80 0x10 0x00 0x01`; the frame length and the assertions are unchanged. Re-confirmed the case still fails with the `uint16_t` restored.
https://github.com/apache/trafficserver/pull/13627/commits/b1342b11fed651b8fe981b31dc427d45983114f1
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -166,6 +166,43 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
}
}
+// A SETTINGS identifier is a full QUIC variable-length integer (up to 62 bits).
+// Parsing must compare the decoded identifier against the known-ID set at full
+// width; narrowing the decoded value into a smaller integer type can cause an
+// identifier outside the known set to be misinterpreted as a known one.
+// Here, encoding 0x10001 as a 4-byte QUIC varint (0x80 0x01 0x00 0x01) shares
+// its low 16 bits with HEADER_TABLE_SIZE (0x01). The parser must treat the
+// identifier as unknown and skip the setting rather than store it under
+// HEADER_TABLE_SIZE.
+TEST_CASE("Load SETTINGS Frame ignores wide unknown identifier", "[http3][http3-settings-id-width]")
+{
+ uint8_t buf[] = {
+ 0x04, // Type
+ 0x05, // Length
+ 0x80, 0x01, 0x00, 0x01, // Identifier: QUIC varint encoding of 0x10001
+ 0x2a, // Value (1-byte QUIC varint = 42)
+ };
+ 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();
Review Comment:
Done in b1342b11fe and d1b88d56f0. "Load DATA Frame" and "Load SETTINGS Frame" now release the frames before `free_MIOBuffer()`, carrying the same explanatory comment as the new case.
https://github.com/apache/trafficserver/pull/13627/commits/b1342b11fed651b8fe981b31dc427d45983114f1
https://github.com/apache/trafficserver/pull/13627/commits/d1b88d56f0d48707de7c652a17bda682773e08fc
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -166,6 +166,43 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
}
Review Comment:
Done in b1342b11fe — the case is tagged `[http3]` now.
https://github.com/apache/trafficserver/pull/13627/commits/b1342b11fed651b8fe981b31dc427d45983114f1
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#issuecomment-5631324440
[approve ci freebsd]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#issuecomment-5615017786
[approbe ci autest freebsd]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#issuecomment-5615074456
[approve ci autest 1]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#discussion_r3939423318
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -166,6 +166,43 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
}
}
+// A SETTINGS identifier is a full QUIC variable-length integer (up to 62 bits).
+// Parsing must compare the decoded identifier against the known-ID set at full
+// width; narrowing the decoded value into a smaller integer type can cause an
Review Comment:
Use a real GREASE identifier instead of `0x10001`. Identifiers of the form `0x1f * N + 0x21` (§7.2.4.1) are the ones clients actually send, and `0x1f * 33824 + 0x21 = 0x100001` aliases `HEADER_TABLE_SIZE` on its low 16 bits. Same assertion, same 4-byte varint, same frame length — bytes become `0x80 0x10 0x00 0x01`. Roughly 1 in 16k random GREASE values hits one of the four `VALID_SETTINGS_IDS`, so this pins the trigger that occurs in the field.
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -166,6 +166,43 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
}
}
+// A SETTINGS identifier is a full QUIC variable-length integer (up to 62 bits).
+// Parsing must compare the decoded identifier against the known-ID set at full
+// width; narrowing the decoded value into a smaller integer type can cause an
+// identifier outside the known set to be misinterpreted as a known one.
+// Here, encoding 0x10001 as a 4-byte QUIC varint (0x80 0x01 0x00 0x01) shares
+// its low 16 bits with HEADER_TABLE_SIZE (0x01). The parser must treat the
+// identifier as unknown and skip the setting rather than store it under
+// HEADER_TABLE_SIZE.
+TEST_CASE("Load SETTINGS Frame ignores wide unknown identifier", "[http3][http3-settings-id-width]")
+{
+ uint8_t buf[] = {
+ 0x04, // Type
+ 0x05, // Length
+ 0x80, 0x01, 0x00, 0x01, // Identifier: QUIC varint encoding of 0x10001
+ 0x2a, // Value (1-byte QUIC varint = 42)
+ };
+ 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();
Review Comment:
Please apply this same ordering to "Load DATA Frame" and "Load SETTINGS Frame". Copilot's finding is correct — `free_MIOBuffer()` does `THREAD_FREE(mio, ioAllocator, …)`, so the reader outlives its buffer; it's latent only because the freelist doesn't unmap, which is why CI is green. One correct test with an explanatory comment next to two that do it wrong is the shape that gets copied.
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -166,6 +166,43 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
}
Review Comment:
Drop `[http3-settings-id-width]`; the other 15 cases in this directory use `[http3]` alone.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#discussion_r3966152140
##########
src/proxy/http3/test/test_Http3Frame.cc:
##########
@@ -162,10 +166,51 @@ TEST_CASE("Load SETTINGS Frame", "[http3]")
CHECK(settings_frame->get(Http3SettingsId::MAX_FIELD_SECTION_SIZE) == 0x0400);
CHECK(settings_frame->get(Http3SettingsId::NUM_PLACEHOLDERS) == 0x0f);
+ 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:
Keeping the explicit `reset()` calls. Scope-based teardown does enforce the order, but the comment is the point here -- `free_MIOBuffer()` ends in `THREAD_FREE(mio, ioAllocator, this_thread())` while `~Http3Frame()` calls `_reader->dealloc()`, which is `mbuf->dealloc_reader(this)` on that same buffer. An unlabelled inner block enforces the order without recording why it matters, and a later edit that moves a line past the closing brace gets no warning from CI either way, since the freelist doesn't unmap.
All three load-path teardowns now carry the same note, so the repetition is deliberate -- a uniform shape rather than one annotated case beside two bare ones. Restructuring 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Preserve full width of SETTINGS frame identifiers [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #13627:
URL: https://github.com/apache/trafficserver/pull/13627#issuecomment-5599503367
[approve ci autest 1 2]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org