Repository navigation
fix(protocols): let from_data rebuild a parsed packet - #536
Conversation
Closes #506. `IPv4.from_data()` raised `ProtocolUnbound: unsupported type ... Raw` on any datagram read off the wire. Two independent faults, both downstream of the `_make_data` that #494 fixed: - schema/schema.py: the `PayloadField` branch of `Schema.pack` imported `Protocol` where every other site in the tree imports `ProtocolBase as Protocol`, so it tested `isinstance(data, Protocol)`. `Protocol` has 0 descendants and `ProtocolBase` has 43 (measured), so the branch that packs a protocol payload was unreachable and every protocol instance fell through to `ProtocolUnbound` -- including the `Raw` that parsing yields. Strictly a widening: `Protocol` is itself a `ProtocolBase`. - internet/ipv4.py: both branches of `_make_ipv4_options` appended a bare `Enum_OptionNumber.EOOL` as end-of-list padding where the option field takes only schemas and bytes. Now an `EOOL` option schema, as the adjacent `NOP` padding always was. - tests: round-trip three *parsed* datagrams, pack a `ProtocolBase` payload through `Schema.pack` directly, and assert both padding branches emit an option. `EXPECTED_FAILURES` drops from 59 entries to 55 as `ipv4-option/RR`, `LSR`, `SSR` and `SID` now round-trip. `SID` closes only because padding is no longer fatal; its 6-octet pack asymmetry survives and is pinned by a new test and filed as #534.
|
✅ GOOD TO MERGE — the shipped fix genuinely checks |
Detailed review (independent verification, falsify-not-bless)Reviewed at head 1. Test design — both required properties present, confirmed by reading the diff.
2. Falsification attempt — a genuine, worth-noting finding. Two "wrong fixes" tried against the shipped tests:
3. Issue #534 — exists and matches precisely. Title, body, the exact file/line ( 4. 5. Repo-wide sweep — re-derived independently; the fix's effect corroborates, the absolute total does not. No sweep script is checked into the repo (searched
Zero regressions confirmed independently either way. The two "moved" categories — the actual measurable effect of the fix — are within single digits of the PR's claim, strongly corroborating the fix's impact. The two "unchanged" categories differ by exactly +1580 each in the PR's favor — too symmetric to be noise, suggesting roughly 3160 additional layer instances exist in whatever capture set the PR author actually used, which could not be reproduced via the documented 6. Real-packet evidence for root cause 2 (IPv4 7. Descendant counts — confirmed exact. VerdictThe shipped code fix is correct and its targeted fails-before/passes-after proofs are exact. Two things are flagged for visibility rather than as blockers: a test-suite coverage gap surfaced by falsification (not a code defect), and an unresolved sweep-total discrepancy (the fix's effect corroborates; the denominator doesn't, and no alternate generation path could explain it). Recommend merge. |
, #537) * `_make_opt_sec` never set RFC 1108's field termination indicator, so every SEC option the library wrote with an authority was one its own reader warned about -- including in the project's own `options-ipv4.pcap`. Bit 0 of the final octet is now set. * `_make_opt_sec` sized the bitmap from the highest authority *index* rather than the bit count, so a lone `GENSER` (value 0) built a zero-octet bitmap and raised a bare `IndexError`, and every multiple of eight under-sized by an octet. It sizes from the count and rejects a non-authority index with `ProtocolError`. * `Field_Termination_Indicator` (index 7) was accepted as an authority, writing an option that read as terminated while carrying none. Index 7, 15 and 23 are termination bits by `_read_opt_sec`'s own numbering and are now rejected; the reader's `range(7)` is left alone, being the correct half. * `SIDOption.sid` was a `UInt32Field` where RFC 791 gives a 2-octet Stream ID, so a well-formed option over-read by two octets (`packet length < 0: -2`) and re-emitted six octets wide, dragging NOP/EOOL padding in behind it. Narrowed to `UInt16Field`; a parsed datagram now rebuilds byte-identically. * Adds a UDP-in-IPv4 payload test, closing the gap that let a fix special-casing `(Protocol, Raw, NoPayload)` pass all four of #536's tests. Full pytest suite green: 1181 passed, 17 skipped, 2661 subtests.
1 participant
Closes #506.
IPv4.from_data()could not rebuild a datagram that had been parsed — only one built by hand. Two independent faults, both downstream of the_make_datathat #494 fixed, so #499 moved the wall rather than building it.Root cause 1 —
Schema.packnamed the wrong classpcapkit/protocols/schema/schema.py:669, thePayloadFieldbranch:The metaclass revision renamed the base to
ProtocolBaseand keptProtocolas a thin subclass that adds auto-registration for externally defined engines. Every other site in the tree imports it asProtocolBase as Protocol; this one was missed, because its import is a runtime import inside a method rather than a module-levelTYPE_CHECKINGone. I grepped the whole package to check that claim —schema/schema.pywas the sole outlier, and even the other two function-local imports (corekit/protochain.py,internet/esp.py:1337) get it right.Measured descendant counts, by importing every module under
pcapkit.protocolsand walking__subclasses__:ProtocolProtocolBaseSo the branch that packs a protocol payload had been unreachable for about three years, and every protocol instance fell through to
ProtocolUnbound— including theRawthat parsing yields for an unrecognised payload, which is exactly what_make_payloadhands back. This is strictly a widening:Protocolis itself aProtocolBase(asserted in the test), so nothing that satisfied the old check stops satisfying the new one.Because the fault is in the shared base, it was never IPv4-specific — it affected every protocol whose parse yields a protocol-instance payload. That answers the "scope worth checking" question in #506.
Root cause 2 — IPv4 option padding was a wire code, not an option
pcapkit/protocols/internet/ipv4.py:1263and:1294. Both branches of_make_ipv4_optionspadded to the 32-bit boundary withNOPoption schemas but terminated with a bareEnum_OptionNumber.EOOL— the wire code rather than an option. The enclosing options field takes only schemas andbytes, so packing failed withFieldValueError: Field options has invalid valuefor any option whose length is not already a multiple of four.Now an
EOOLoption schema, as the adjacentNOPpadding always was.Evidence
Fails without the fix, one half at a time
Each fix reverted in isolation against the same tests (exit codes read directly;
pytest-subtestsis absent and pytest 9.1.1 reports a failing subtest's parent asPASSED).Reverting the schema fix fails all four new IPv4 tests (three as subtests of the round-trip test) with
ProtocolUnbound: unsupported type ... Raw. Reverting the ipv4 fix fails the padding test, two pre-existing option-constructor tests, and exactly the fourEXPECTED_FAILUREScases below — which is what makes the table edit load-bearing rather than cosmetic:The two halves are independent: neither revert reproduces the other's failures.
Test counts
tests/protocols/internet/test_ipv4_unit.pytests/protocols/test_option_roundtrip_unit.pyEXPECTED_FAILURES— imported, not grepped, since it is built with**unpacking — goes from 59 entries to 55, dropping exactlyipv4-option/{RR, LSR, SSR, SID}.RR,LSRandSSRcould not have been routed around: their length is3 + counts * 4, never a multiple of four.Real-packet sweep
Every protocol layer instance in all 24 sample captures pushed through
type(layer).from_data(layer.info)and compared againstbytes(layer). Both trees swept over the same capture set (the one today'smaingenerates), so the rows line up one-for-one — 8086 layer instances each.RAISED→DIFFERENTRAISED→IDENTICALRAISED→RAISEDIDENTICAL→IDENTICAL3803 previously-raising layer instances now reconstruct (
RAISED6197 → 2394), 300 are newly byte-identical (1889 → 2189), and 0 regressions — no row moved backwards in the transition matrix.The 3503 that reconstruct but do not match are pre-existing reconstruction gaps elsewhere in the library, now reachable where they used to be masked by the exception. They are not introduced here.
An honest note on which half the sweep credits
The sweep above credits the schema half only. I swept a third tree carrying the schema fix alone, and it came back row-for-row identical to the tree with both fixes — 0 differing rows of 8086. No capture in today's fixture set contains a misaligned IPv4 option, so the padding fix changes nothing there.
The IPv4 half does have real-packet evidence, of a different kind: it is why those captures contain no such option. With the fix,
examples/generators/make_samples.pyemits four frames it previously could not write at all, andoptions-ipv4.pcapgrows from 8 frames / 456 octets to 12 frames / 784:Those four new frames are exactly the four
EXPECTED_FAILURESentries dropped. This also renumbers the capture, which invalidated a docstring in this branch's own tests that cited "frame 8 ofoptions-ipv4.pcap" — the inlined literal is now frame 12. It is named by its option rather than its index now, with a note saying why.No regressions beyond the targeted files
schema.pyis shared by every protocol, so the whole blast radius was run on both trees:tests/protocols/ tests/corekit/ tests/foundation/.mainatc8fd97bcdExactly +5 tests and +3 subtests — the five added here, and the three subtests of the round-trip test. The same 11 skips, and nothing that passed before stops passing. Per the host-care constraint I did not run
coverageor the full suite.ipv4-option/SID: dropped from the table, but not droppedSIDstarts passing here only because padding is no longer fatal. The underlying defect survives untouched:SIDOption.sidis aUInt32Fieldatpcapkit/protocols/schema/internet/ipv4.py:368, where RFC 791 §3.1 gives the Stream ID two octets inside a four-octet option — which is also what_make_opt_siditself writes intolength=4atipv4.py:1637. Only the schema field disagrees.Its generator cycle closes because both halves go through the same
_make_opt_sidand emit the same wrong six octets, matching each other. It is only against the wire that the width shows:88040037from_datare-emits8804000000370100It cannot stay an
EXPECTED_FAILURESentry, because the status such an entry would have to record is'OK'— the one valuetest_round_trip_is_identity_or_a_recorded_gapreads as "needs no entry". So rather than deleting it with no trace, it is recorded twice:test_a_parsed_sid_option_re_emits_two_octets_too_wide, which starts from wire octets rather than the generator's case, and turns red when the field is narrowed;Tests added
test_ipv4_from_data_rebuilds_a_parsed_datagram— three parsed datagrams (no options; aRTRALToption; options with no payload at all), from wire literals rather than constructed, so the info carries what parsing really produces.test_ipv4_make_accepts_a_protocol_instance_as_payload— the documentedbytes | Protocol | Schemaconstruction API, asserted on shape as well as bytes so that a_make_payloadchanged to returnbytescannot make the round trip pass while leaving the API broken.test_schema_pack_packs_a_protocol_base_payload_on_its_own—Schema.packdirectly, with nomakein front of it, so the test says which layer was at fault. Also assertsissubclass(Protocol, ProtocolBase)(the no-regression claim) and that a non-payload still raisesProtocolUnbound(widened, not removed).test_ipv4_make_options_pads_with_an_eool_option_not_its_wire_code— both padding branches, thelistone reached throughmakeand the container one reached throughfrom_data.test_a_parsed_sid_option_re_emits_two_octets_too_wide— the IPv4 SID option: SIDOption.sid is UInt32Field where RFC 791 gives a 16-bit Stream ID, so a parsed option re-emits two octets too wide #534 pin.Each new test carries
assertNotIsInstance(raw_payload, Protocol). That negative matters: the defective branch was not uncovered before this fix, it was covered byDummyProtocolattests/protocols/schema/test_schema_unit.py:167— the only class in the tree subclassing the engineProtocol— which packed happily while every real protocol raised. Without the negative assertion the same hole reopens if someone makes a protocol subclassProtocolinstead of fixing the check.One disambiguation for whoever greps this
grep 'class .*(Protocol)'returns 28 hits underpcapkit/protocols/data/, which look like counter-examples to the "0 descendants" claim above and are not. There are two unrelated classes namedProtocol:pcapkit.protocols.protocol.Protocolpcapkit.protocols.data.protocol.ProtocolVerified:
issubclass(data.internet.ipv4.IPv4, protocols.protocol.Protocol)isFalse, and so isissubclass(..., ProtocolBase). The descendant counts above were measured by walking__subclasses__after importing every module underpcapkit.protocols, so they already account for these.