Skip to content

fix(http2): bound retained response headers#1000

Merged
Coldwings merged 1 commit into
mainfrom
fix/http2-response-header-limits
Jul 21, 2026
Merged

fix(http2): bound retained response headers#1000
Coldwings merged 1 commit into
mainfrom
fix/http2-response-header-limits

Conversation

@Coldwings

Copy link
Copy Markdown
Owner

Description

Bound retained HTTP/2 response headers per stream by both field count and aggregate name/value bytes. An over-limit peer now fails only the offending stream with ENHANCE_YOUR_CALM, while callers receive EMSGSIZE and sibling streams remain usable.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation (changes to documentation, comments, or examples)
  • Tests (adding or modifying tests)

Related Issues

Closes #998
Related to #986

Changes Made

Core Changes

  • Track retained response header count and aggregate name/value bytes per stream.
  • Reject limits with overflow-safe arithmetic before copying the offending field.
  • Reset accounting between informational and final response blocks while sharing the final-header/trailer budget.
  • Queue RST_STREAM(ENHANCE_YOUR_CALM) and return nghttp2's stream-local temporal callback failure.
  • Map header-limit failures to EMSGSIZE for the waiting client.
  • Add configurable 100-field / 64-KiB defaults without breaking legacy positional aggregate initialization.

API Changes (if applicable)

h2_client_config and h2_session_config gain two trailing, source-compatible fields:

size_t max_response_headers = 100;
size_t max_response_header_bytes = 64 * 1024;

Testing

Unit Tests

  • Added new tests for the changes
  • All tests pass locally

Test Results

Static review only; local builds and tests were intentionally not run. GitHub Actions is the executable validation source for this PR.

Focused coverage includes independent count and byte boundaries, oversized fields, informational/final/trailer accounting, config propagation and legacy aggregate initialization, plus a raw nghttp2 two-stream reset regression.

Checklist

Code Quality

  • My code follows the project's code style
  • I have added/updated comments for complex logic
  • I have removed any debug code, TODOs, or commented-out code

Documentation

  • I have updated documentation
  • I have updated API documentation

Testing

  • I have added tests that prove the fix is effective
  • New and existing unit tests pass locally with my changes
  • I have tested with ASAN and TSAN

Compatibility

  • My changes are backward compatible
  • I have considered the impact on existing users
  • I have updated CHANGELOG.md

Additional Notes

Two independent static reviews passed against the exact rebased head before opening this PR. No local compilation or test execution was performed.

Reviewer Guidance

Areas requiring special attention:

  • nghttp2 callback return/reset semantics and sibling-stream survival.
  • Informational response, final header, and trailer accounting boundaries.
  • Source compatibility of the appended public aggregate members.

Copilot AI review requested due to automatic review settings July 21, 2026 15:15

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.

@Coldwings

Copy link
Copy Markdown
Owner Author

Independent current-head review summary (4cb8e13d4b3299ddb586e0677a76b0674bc47cea)

Two read-only reviewers examined this exact head from complementary security/lifecycle and public-contract perspectives. Both reported PASS with no blocker, major, or minor findings.

Verified points:

  • Field-count and aggregate-byte checks are overflow-safe and reject before copying the offending field.
  • Duplicate regular fields consume both budgets; informational response headers reset before the final response, while final headers and trailers share one retained budget.
  • The nghttp2 callback queues a targeted RST_STREAM(ENHANCE_YOUR_CALM) and returns NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE, preserving sibling-stream/session viability.
  • Header-limit state remains distinguishable after stream-close error updates and maps to EMSGSIZE for callers.
  • The client limits propagate to direct session configuration.
  • The new public config members are appended after every legacy aggregate member; positional initialization remains source-compatible and is regression-tested.
  • Focused tests independently pin count-only rejection, byte boundaries, oversized fields, state transitions, config propagation, and raw nghttp2 two-stream reset behavior.
  • The final rebase was patch-identical to the fully reviewed commit (git range-diff equality and matching stable patch ID); whitespace checks were clean.

Review method: static inspection only; no local build or tests were run. GitHub Actions remains the executable validation gate.

@Coldwings
Coldwings force-pushed the fix/http2-response-header-limits branch from 4cb8e13 to c071d8d Compare July 21, 2026 15:29
@Coldwings

Copy link
Copy Markdown
Owner Author

CI diagnosis and current-head update

The original head built successfully on all four C++ matrix jobs, but the new synthetic client/server nghttp2 regression failed deterministically at its first transfer (REQUIRE(flush_client())) on arm64 Release, arm64 Debug, x64 Release, and x64 Debug. The production implementation was not implicated; the test server rejected the artificial initial client byte sequence before the reset assertion ran.

Head c071d8d139e7caf5534809308665d6969edc7cc6 replaces that harness with a deterministic client-side wire assertion:

  • drain all initial client output while nghttp2 reports pending writes;
  • invoke the exact callback reset helper;
  • verify the sole emitted frame is the protocol-fixed 13-byte RST_STREAM targeting the limited stream with ENHANCE_YOUR_CALM in network byte order;
  • submit a higher-ID follow-up request and verify the session remains readable.

An independent security/lifecycle re-review passed this exact head and specifically verified the nghttp2 state preconditions, frame layout, byte-order portability, pointer lifetimes, and continued-session assertion. A second independent contract review is also being completed while the replacement CI run executes.

@Coldwings

Copy link
Copy Markdown
Owner Author

Updated independent current-head review summary (c071d8d139e7caf5534809308665d6969edc7cc6)

After the CI-driven test-harness correction, two independent read-only reviewers re-examined this exact head from security/lifecycle and public-contract perspectives. Both reported PASS with no blocker, major, or minor findings.

In addition to the production guarantees in the earlier review, they specifically verified:

  • all initial client magic/settings/request output is drained before reset inspection;
  • the remaining output is the protocol-fixed 9-byte frame header plus 4-byte RST_STREAM payload;
  • type, reserved-bit-masked stream ID, and big-endian ENHANCE_YOUR_CALM parsing are correct and architecture-independent;
  • the reset targets only the limited stream;
  • a higher-ID follow-up request plus live read state demonstrates continued session usability;
  • the production HTTP/2 implementation and all count/byte/state/config/source-compatibility regressions remain intact.

Review method: static inspection only. The replacement GitHub Actions run is the executable validation gate.

@Coldwings

Copy link
Copy Markdown
Owner Author

Second CI diagnosis and final test-harness correction

The client-only replacement still failed at its preliminary drain because nghttp2_session_mem_send() returned 0 while nghttp2_session_want_write() remained true. That state is permitted: want_write() can reflect deferred/internal work even when no serialized bytes are currently available.

Head 682fc713ea6de86f4ca29c9bb4bd84aa3d8a191f now follows the serialization API directly: it repeatedly calls mem_send() until negative error or 0 (no available bytes), without requiring want_write() to become false. The fixture has no DATA provider or callback capable of replenishing output, and the subsequent exact 13-byte result proves the targeted RST is the sole next available frame.

Two independent reviewers passed this exact rebased head. They verified the corrected nghttp2 termination condition, absence of an infinite positive-output source, exact RST wire parsing, follow-up stream viability, the full production security/API contract, and preservation of both #997 and #998 documentation during rebase. The new GitHub Actions run is the remaining validation gate.

@Coldwings
Coldwings force-pushed the fix/http2-response-header-limits branch from c071d8d to 682fc71 Compare July 21, 2026 15:43
@Coldwings

Copy link
Copy Markdown
Owner Author

CI diagnosis for run 29845293621: the x64-Debug job built successfully and the normal unit suite passed. Its ASAN-only rerun failed at the unchanged task_group publishes completion after child state release timing assertion (tests/unit/test_task_group.cpp:1193, join_observed_pending == false).

This PR does not modify task_group or that test. The HTTP/2 response-header tests passed in the normal suite, and both Release matrix jobs passed. This is the same unrelated timing-sensitive ASAN failure previously observed on the repository matrix, so I will rerun the failed job after the current run finishes and require that rerun to pass before merge.

@Coldwings
Coldwings merged commit ed886f2 into main Jul 21, 2026
16 of 17 checks passed
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.

[Bug] HTTP/2 response headers are buffered without a per-stream bound

2 participants