Skip to content

fix(httpv2): HTTP/2 frames lose octets on rebuild (#1242-#1248, #1251) - #1341

Merged
JarryShaw merged 1 commit into
mainfrom
fix/http2-roundtrip
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/http2-roundtrip

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — ran the same tools on the four changed modules: mypy and isort clean, pylint adds nothing new
  • make test passes, and a test case covers the change — the modules listed below only, not the full suite
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1242
Closes #1243
Closes #1244
Closes #1245
Closes #1246
Closes #1247
Closes #1248
Refs #1251 (only the HTTP/2 PING half; the OSPF half is separate)

Breaking.

  • Data classes gain the required fields reserved, promised_reserved, last_reserved and increment_reserved.
  • flags is now a Flags object, not None, for PRIORITY, RST_STREAM, GOAWAY, WINDOW_UPDATE and unassigned frame types.
  • make now ORs the flags argument with the frame's own flags.
  • info.settings is now Settings, not a plain OrderedMultiDict.
  • A custom reader registered through register_frame must now return an HTTP/2 data object with reserved and flags; one that does not breaks from_data (see the stub edit in test_http_unit.py).
  • New make keywords: reserved, payload and preface, plus padded/*_reserved on the frame constructors.

Follow-up (not in this PR). examples/generators/options.py:1159 still says flags is "silently ignored", which is no longer true now that make combines them.

Evidence. Before the fix, all 16 repros rebuilt wrong and frame=b'abc' raised AttributeError. After it, all 16 match byte for byte, including when frames are followed by a SETTINGS ACK. All 241 HTTP layers in examples/captures still round-trip.

New tests. The new module has 10 tests. Against origin/main source, 36 failed and 5 passed; against the previous head 95ffb1b59, only the last-value test failed (1 != 3). The 5 that passed are the PRIORITY bad-bytes guard and the 8-octet PING controls. With the fix, all pass.

Test runs.

  • test_http_unit: 65 passed.
  • test_http2_roundtrip_unit: 10 passed.
  • The remaining HTTP/2 and HTTP modules, test_option_roundtrip_unit, test_construction_keyword_check_unit and tests/project: all passed (389 passed, 1 skipped).
  • run_unittest_leg.py protocols --exclude protocols/internet: 860 tests, 0 failures.

test_http_unit.py edits. Its stubs gained the new reserved and PADDED keys. Two assertions pinned the old behaviour and were updated: explicit version=2 refusing a prefaced buffer, and info equality without preface.

@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 95ffb1b59: NEEDS CHANGES (Sonnet reviewed; the author ran on Opus). One blocker, which I re-derived myself.

#1247 flips which value info.settings[id] returns for a repeated identifier. For one SETTINGS frame carrying HEADER_TABLE_SIZE=1, ENABLE_PUSH=0, HEADER_TABLE_SIZE=3:

settings[HEADER_TABLE_SIZE] dict(settings) getlist
11ae68ebf 3 …: 3 [0]/[3]
this PR 1 …: 1 [1, 3]

RFC 9113 §6.5 has the last value win. So the rebuild is byte-exact, but indexing now hands consumers the superseded value, and the PR's breaking list does not mention it. Make [] (and so dict()) return the last value, keeping order for getlist and the rebuild, or else list the change and document getlist. The first is right.

Everything else held. Every repro is byte-exact. #1243 handles Length 0, three back-to-back frames and Length > buffer; the prefaced and non-prefaced version=2 paths are not mis-sliced; PADDED with Pad Length 0 and Pad Length with PADDED clear both rebuild. RFC 9113 confirms the four reserved bits (PRIORITY has E, untouched), and §6.7 makes a non-8-octet PING a FRAME_SIZE_ERROR. Across 23 fixtures, info differs from main only on the 10 HTTP/2 layers, by the documented keys, and the dumpers run clean. The new module fails 35/5 on the base. protocols --exclude protocols/internet gives 859 tests, 0 failures, which closes the author's gap.

Add to the breaking list: a custom reader registered via register_http_frame that returns no reserved/flags now breaks from_data. Nit: examples/generators/options.py:1159 says flags is "silently ignored", which is no longer true.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
- Schema: bound the frame payload by its declared Length, and keep what
  follows as a `payload` dispatched as the next layer (#1243). Name the
  reserved R bit of the header, PUSH_PROMISE, GOAWAY and WINDOW_UPDATE
  stream fields (#1246). Keep undefined flag bits in `__flags__` (#1245).
- Data: add `reserved`, `promised_reserved`, `last_reserved` and
  `increment_reserved`; frame types with no defined flags now carry a bare
  `Flags` holding the raw octet instead of `None`.
- Makers: PADDED follows the flag, not a non-zero Pad Length, with a new
  `padded` keyword (#1244); `make` ORs its `flags` with the frame's; new
  `reserved` and `payload` keywords; PING opaque data must be 8 octets
  (#1251, HTTP/2 half).
- Reader: SETTINGS keeps repeated identifiers in order in a new
  `Settings` multidict whose index answers the last value (#1247); `make`
  with raw frame octets parses them back with the frame schema (#1248).
- HTTP dispatcher: record the connection preface as `preface` and rebuild
  it, also through an explicit `version=2` (#1242).

Closes #1242
Closes #1243
Closes #1244
Closes #1245
Closes #1246
Closes #1247
Closes #1248
Refs #1251
@JarryShaw
JarryShaw force-pushed the fix/http2-roundtrip branch from 95ffb1b to f6e21cf Compare October 7, 2026 19:37
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on f6e21cf67: GOOD TO GO. I verified the delta from 95ffb1b59 myself: 3 files, +49/−6, with corekit/ untouched.

The round-1 blocker is fixed. A new Settings(OrderedMultiDict) in data/application/httpv2.py overrides __getitem__ and the non-multi items(). I tested it on HEADER_TABLE_SIZE=1, ENABLE_PUSH=0, HEADER_TABLE_SIZE=3:

access value
[], get 3
values() [3, 0]
dict(), to_dict() HEADER_TABLE_SIZE: 3
getlist [1, 3] (wire order kept)
copy, deepcopy, pickle 3, still a Settings

The rebuild stays byte-exact. The new test fails against the previous head with 1 != 3. The protocols --exclude protocols/internet leg gives 860 tests, 0 failures.

The breaking list now names Settings and the custom-reader reserved/flags requirement. One correction to my round-1 comment: the registrar is register_frame, not register_http_frame. The PR body has it right. The stale options.py:1159 comment is listed as a follow-up.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.89% (unit tier, Python 3.14, f6e21cf67, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1034 2342 845 90.24%
pcapkit/corekit 1878 89 580 22 94.43%
pcapkit/dumpkit 142 0 42 0 100.00%
pcapkit/foundation 2442 102 852 39 94.38%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15976 209 4136 180 98.04%
pcapkit/toolkit 495 73 150 1 83.57%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 9d87abc into main Oct 7, 2026
36 checks passed
@JarryShaw
JarryShaw deleted the fix/http2-roundtrip branch October 7, 2026 22:55
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment