Skip to content

fix(client, qpack): align QPACK decoder with protocol spec compliance - #14

Open
0x676e67 wants to merge 91 commits into
mainfrom
fix-qpack-decoder-compliance
Open

fix(client, qpack): align QPACK decoder with protocol spec compliance#14
0x676e67 wants to merge 91 commits into
mainfrom
fix-qpack-decoder-compliance

Conversation

@0x676e67

Copy link
Copy Markdown
Owner

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Insert-count increments remain unvalidated and can overflow or violate the QPACK decoder-stream protocol.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Aligns QPACK decoding, buffering, blocked-stream handling, and error behavior with HTTP/3 protocol requirements.

Changes:

  • Corrects QPACK prefix, capacity, reference, and insert-count handling.
  • Adds fragmented encoder-stream parsing and blocked-stream coordination.
  • Improves cancellation, connection-error wakeups, settings validation, and tests.
File summaries
File Description
http3/src/tests/http3_quinn.rs Reorders imports.
http3/src/qpack/tests.rs Tests blocked-stream coordination.
http3/src/qpack/stream.rs Supports larger insert-count increments.
http3/src/qpack/parse_error.rs Expands protocol parsing errors.
http3/src/qpack/mod.rs Adds blocked-stream registry and events.
http3/src/qpack/encoder.rs Handles widened insert-count increments.
http3/src/qpack/dynamic.rs Revises limits, references, and decoder insertion.
http3/src/qpack/decoder.rs Adds buffered parsing and capacity enforcement.
http3/src/qpack/block.rs Corrects field-section prefix reconstruction.
http3/src/proto/frame.rs Validates SETTINGS varints.
http3/src/error/connection_error_creators.rs Wakes QPACK waiters after publishing errors.
http3/src/connection.rs Integrates QPACK blocking, cancellation, and wakeups.
http3/src/client/stream.rs Routes receive cancellation through QPACK cleanup.
http3/src/client/builder.rs Documents QPACK setting ranges.
http3/src/buf.rs Tracks buffered byte counts efficiently.
http3-quic/src/lib.rs Reorders conditional exports.
Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

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

Comment thread http3/src/qpack/encoder.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The server still does not poll incoming QPACK encoder streams, preventing required processing and critical-stream error detection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 19/19 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread http3/src/server/connection.rs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Field prefixes with zero Required Insert Count can still accept a protocol-invalid nonzero Base.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

http3/src/qpack/block.rs:187

  • RFC 9204 §4.5.1.2 requires Base to be zero whenever Required Insert Count is zero. This positive-sign branch currently accepts a peer-controlled nonzero Delta Base (for example, RIC 0 and Delta Base 1), so a malformed field-section prefix is treated as valid instead of causing QPACK decompression failure. Handle the zero-RIC case explicitly before applying Delta Base.
        let base = if !self.sign_negative {
            required
                .checked_add(self.delta_base)
                .ok_or_else(invalid_base)?

http3/src/qpack/decoder.rs:56

  • n is the Required Insert Count, not the number of references that are missing. For example, with insert count 99 and required count 100 this reports “missing 100 refs,” although only one insertion is unavailable. Describe the required count directly so diagnostics are accurate.
            DecoderError::MissingRefs(n) => write!(f, "missing {} refs to decode block", n),
  • Files reviewed: 21/21 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@0x676e67
0x676e67 requested a balanced review from Copilot August 27, 2026 04:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

fix(qpack): publish errors before waking waiters
fix(qpack): track decoder feedback per field section
Publish blocked-stream limit failures before waking the rejected request, map local decoder configuration failures to H3_INTERNAL_ERROR, and handle critical stream setup and zero-progress writes without stalling.

Add connection-level coverage for decoder feedback without blocked requests and document the transport progress contract.
Install the nightly toolchain for the style job and invoke cargo fmt with an explicit +nightly selector so CI matches the repository formatting rule.
fix(qpack): harden decoder driver error handling
fix(qpack): harden decoder protocol boundaries
* docs(qpack): correct protocol terminology

* docs(qpack): restore address space index diagrams
@0x676e67
0x676e67 requested a balanced review from Copilot August 28, 2026 10:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants