brbzull0 commented on code in PR #13655:
URL: https://github.com/apache/trafficserver/pull/13655#discussion_r3966883574
##########
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:
The `sleep(1)` matches the existing pattern in this file --
`TEST_CASE("Decoding")` waits the same way at line 390, and
`TestQPACKEventHandler` is a plain `Continuation` holding an `int`, with no
condition variable to signal. Introducing one would mean reworking the shared
handler that the existing test also uses, which is beyond the scope of this
change.
It also does not run on the passing path: with the guard corrected,
`decode()` returns -1 synchronously, so the `if (sync_ret >= 0)` branch is
skipped. The sleep is only reached when the guards are reverted to `&&`, i.e.
when confirming the test fails without the fix.
##########
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 `0xFFFF` literals are not new here -- all ten are unchanged from master.
This PR only flips `&&` to `||`, fixes the inverted comparison at line 934, and
raises the one outlier bound at line 1525 from `0xFF` to `0xFFFF`.
Renaming all ten to `UINT16_MAX` would add unrelated churn to an otherwise
minimal fix. #13621 also widens the `index` parameter at line 1517 to
`uint64_t`, which removes that bound entirely, so naming them now would touch
code that is already changing.
--
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]
