Copilot commented on code in PR #13655:
URL: https://github.com/apache/trafficserver/pull/13655#discussion_r3966684153
##########
src/proxy/http3/QPACK.cc:
##########
@@ -284,7 +284,7 @@ QPACK::decode(uint64_t stream_id, const uint8_t
*header_block, size_t header_blo
uint64_t tmp = 0;
int64_t ret = xpack_decode_integer(tmp, header_block, header_block +
header_block_len, 8);
- if (ret < 0 && tmp > 0xFFFF) {
+ if (ret < 0 || tmp > 0xFFFF) {
Review Comment:
The newly-updated guards use the literal `0xFFFF` to model a `uint16_t` max.
Using a named constant (e.g., `UINT16_MAX` or
`std::numeric_limits<uint16_t>::max()`) would make the intent explicit and
reduce the risk of inconsistent bounds if the destination type changes.
##########
src/proxy/http3/test/test_QPACK.cc:
##########
@@ -439,6 +439,46 @@ TEST_CASE("Encoding", "[qpack-encode]")
}
}
+// QPACK::decode() parses the Required Insert Count varint from the header
+// block prefix and stores the decoded value in a uint16_t local; the call site
+// must reject a varint whose value exceeds the uint16_t range, otherwise the
+// value silently truncates and the decoder proceeds with corrupted state.
+// Drive QPACK::decode() with a 4-byte 8-bit-prefix varint encoding 0x10000
+// and assert that the decoder reports failure rather than accepting it.
+TEST_CASE("decode() rejects oversized Required Insert Count at entry",
"[qpack-decode-entry-bounds]")
+{
+ QUICApplicationDriver driver;
+ auto qpack =
std::make_unique<QPACK>(driver.get_connection(), UINT32_MAX, 4096, 100,
MAX_FIELD_SIZE);
+ auto handler = std::make_unique<TestQPACKEventHandler>();
+
+ HTTPHdr hdr;
+ hdr.create(HTTPType::REQUEST);
+
+ uint8_t block[16] = {0};
+ uint8_t *p = block;
+ int enc_len = xpack_encode_integer(p, p + sizeof(block), 0x10000, 8);
+ REQUIRE(enc_len > 0);
+ p += enc_len;
+ *p++ = 0x00;
+ size_t block_len = static_cast<size_t>(p - block);
+
+ int sync_ret = qpack->decode(1, block, block_len, hdr, handler.get(),
eventProcessor.all_ethreads[0]);
+
+ // decode() schedules its result asynchronously when it returns >= 0; only
+ // wait in that case. A synchronous failure (sync_ret < 0) means no event
+ // will be delivered and there is nothing to wait for.
+ if (sync_ret >= 0) {
+ sleep(1);
+ }
Review Comment:
`sleep(1)` makes the test timing-dependent and potentially flaky under
load/slow CI (event delivery could take >1s, or could already be delivered
immediately). Prefer a deterministic wait mechanism (e.g., have
`TestQPACKEventHandler` signal a condition variable/promise when `handle_event`
runs, then wait with a bounded timeout), or explicitly drive whatever event
loop/test harness is responsible for running the scheduled continuation.
--
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]
