QPACK: use || in the varint decode range guards

[brbzull0]

Ten guards in src/proxy/http3/QPACK.cc are written

if ((ret = xpack_decode_integer(tmp, ...)) < 0 && tmp > 0xFFFF) {
  return -1;
}

With &&, neither condition rejects on its own: a decode failure falls through
because the value check does not hold, and an in-range failure-free decode of an
oversized varint falls through because the error check does not hold. The value
is then narrowed into the surrounding uint16_t. The delta_base_index guard in
_decode_header() additionally had its comparison inverted
(delta_base_index < 0xFFFF where > was meant).

Lines: 287, 925, 934, 1517, 1524, 1548, 1555, 1578, 1600, 1622.

One bound change, and why it is required

The value guard in _read_insert_with_name_ref() (line 1525 on master) is the
only one of the ten bounded at 0xFF; the other nine use 0xFFFF. This raises
it to 0xFFFF as well, and that is load bearing rather than tidying:

value_len is a size_t &, so 0xFF is not protecting against any narrowing --
it is a bare sanity limit on a decoded header value length. Under && it never
fired, so it did not matter. Under || it fires, and a 0xFF bound would abort
the decode for any header value longer than 255 bytes -- ordinary cookies and
tokens included. At 0xFFFF the check is unreachable, because
xpack_decode_string() is already capped by _header_field_max_size (32768 by
default), which matches the behaviour of the other nine sites.

Flipping that operator without raising the bound would be a regression, so the
two changes belong together.

The unchecked failure is the more interesting half

On failure xpack_decode_integer() returns -1. Callers then do
read_len += ret, and read_len is size_t, so it becomes SIZE_MAX.
IOBufferReader::consume() receives that as int64_t -1:

ink_release_assert(n == 0 || is_read_avail_more_than(n - 1));
start_offset += n;

is_read_avail_more_than(-2) is true, so the release-assert passes and
start_offset moves backwards by one. The helper returns 0 rather than a
negative, so _on_encoder_stream_read_ready() does not call _abort_decode(),
and its while (reader.is_read_avail_more_than(0)) loop reads the same byte
again on the next iteration. _read_duplicate() (line 1578) has exactly this
shape.

Widening the destination types would address the truncation but not this half --
the < 0 check is what is missing.

Relationship to #13621

#13621 also touches _read_insert_with_name_ref() (line 1517), widening its
index parameter to uint64_t so the narrowing disappears rather than being
rejected. That is a better fix for that one site.

This change flips the operator there too, so the ten sites are corrected
consistently and none is left with a known-broken guard while #13621 is in
review. If #13621 lands afterwards its approach supersedes the operator flip at
that line -- a one-line resolution. Line 1524, the value guard in the same
function, is not covered by #13621 at all.

Test

Adds decode() rejects oversized Required Insert Count at entry to
test_QPACK.cc, tagged [qpack-decode-entry-bounds]. It drives
QPACK::decode() with a 4-byte 8-bit-prefix varint encoding 0x10000 and
asserts the decoder reports failure rather than accepting it.

Confirmed to be a regression test: with the guards restored to && it fails on
both assertions with sync_ret := 0 and
handler->last_event() := 2700 (QPACK_EVENT_DECODE_COMPLETE) -- the oversized
count is accepted and truncated to 0. With the change it passes, 3 assertions.

Also run, 5/5 pass: h3_proxy_verifier, h3_python_client,
h3_stream_lifetime, h3_flow_control, h3_sni_check.

The remaining eight guards have no direct test. Reaching _decode_header()
requires getting past the QPACK::decode() check this test exercises, and the
encoder-stream helpers need a QPACK encoder stream driven with crafted
instructions, which the current test harness does not set up.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Fixes QPACK varint range-guard logic to correctly reject decode errors and oversized values (preventing silent truncation into uint16_t), and adds a regression test for an oversized Required Insert Count.

Changes:

• Replace && with || in multiple QPACK decode range guards so either a decode failure or an out-of-range value is rejected.
• Fix an inverted comparison in _decode_header() for delta_base_index.
• Add a unit test that drives QPACK::decode() with an oversized varint and asserts decode failure.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/proxy/http3/QPACK.cc	Corrects guard conditions to reject decode failures and out-of-range varints (and fixes one inverted bound check).
src/proxy/http3/test/test_QPACK.cc	Adds a regression test ensuring oversized Required Insert Count is rejected rather than truncated.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bneradt]

No actionable findings in this diff. Reviewed all ten guards against the XPACK decoder return conventions and the surrounding destination types, including the corrected delta-base comparison and the value-length bound. The new regression test exercises the synchronous oversized Required Insert Count rejection. This was a source review; I did not run a local build or the QPACK tests. The current PR checks are green.