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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
