fix(pcap): seed the frame byte order under the key the reader consults (#605) - #614
Conversation
|
✅ GOOD TO MERGE — head |
Cross-review appendix — PR #614Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head
|
#605) Reading a big-endian classic PCAP byte-swapped every record header field, and then crashed. `Frame.unpack` seeded the file's declared byte order as `packet['bytesorder']` (misc/pcap/frame.py:199) where `byteorder_callback` (schema/misc/pcap/frame.py:29) reads `packet['byteorder']`, so the `.get()` never found the key and always fell back to `sys.byteorder` -- the reading host's order rather than the file's. On a little-endian host reading a little-endian capture that fallback gives the right answer by coincidence, and every capture in this repository was little-endian, so the wrong code path has always produced correct results. - One character in `Frame.unpack`. The sibling `Frame.pack` eleven lines earlier already spelled the key correctly, which is what marks this as a slip rather than a second key deliberately named. - `byteorder_callback` now records that it is the definition of the key and that its `sys.byteorder` fallback -- which a standalone schema needs -- makes a misspelled key indistinguishable from an absent one. That is what hid this for as long as it hid. - New `examples/generators/endian.py`, wired into `make_samples.py`, because there was no big-endian `.pcap` here at all and a one-character fix with no fixture leaves the corrected path exactly as untested as the broken one. It writes `big_endian.pcap` (magic a1 b2 c3 d4), `big_endian_nanosecond.pcap` (a1 b2 3c 4d, the first fixture to take that branch of the magic-number table) and `little_endian.pcap` (d4 c3 b2 a1), the microsecond pair carrying byte-identical records in the two containers so the tests can assert that the byte order makes no difference to what is read out. Frames come from scapy with real checksums; the containers are packed with `struct`, since `wrpcap` writes the host's order and offers no way to ask for the other. Frame 3 is captured short -- 1200 octets cut to a 96-octet `snaplen` -- so `incl_len` and `orig_len` differ, which frames 1 and 2 cannot show. - New `tests/protocols/misc/pcap/test_frame_endian_runtime.py` drives all three through `extract()` and walks each file's record chain with `struct` to derive its own expectations rather than trusting the numbers it also writes down. A unit-tier case in `test_header_frame_unit.py` builds a two-record big-endian capture in memory instead, so the regression is caught by the fixture-free selection CI runs on every push, not only by the tier that needs `make samples` first. Measured on the unfixed tree, `big_endian.pcap` frame 1 -- really `ts_sec` 1500000000, `ts_usec` 123456, `incl_len` 74 -- read `ts_sec=3106905`, `ts_usec=1088553216`, `incl_len=1241513984`, dated 1970-02-05 rather than 2017-07-14, and `incl_len` being the payload length, that one record consumed the whole file: three frames became one. Through `extract()` the read that followed was handed a negative payload length and raised `ValueError: read length must be non-negative or -1` from schema.py:826, which is the crash the report describes -- the second symptom, not the first. New tests proven to fail without the fix: 4 failed, 1 passed, the three fixture-backed failures by that `ValueError` and the in-memory one by `AssertionError: 3106905 != 1500000000`. All 5 pass with it, and the little-endian twin is the one that passes either way, which is what shows the records themselves are not the variable. After the fix the big-endian and little-endian files parse to identical values, field for field. tests/protocols/misc/ plus the changelog, tier-guard and docstring-contract suites: 131 passed, 219 subtests. Full unit tier (the selection unit-tests.yml runs): 1284 passed, 5 skipped, 2836 subtests. Fixtures verified to regenerate byte-identically. Fixes #605
0307338 to
401fce2
Compare
|
✅ GOOD TO MERGE — head |
Cross-review appendix — PR #614 (re-point)Reviewer: Sonnet; PR authored on Opus 5. This re-points the The six source files are byte-identical between the two headsZero bytes of output. All six paths confirmed present at the new head first, and the same command form over Why the raw head-to-head diff looks alarming, and why it is not
Comparing each head against its own base instead gives an exact match: Both produce the identical 8-file list — Regression re-run at the new head
CI and mergeability
Not independently checked
Disagreement logNone on substance. One note for the record rather than a finding: a reader who runs the bare head-to-head |
Correction to the re-point above — stale head sha, and posted after mergeTwo things about my previous two comments that I should state rather than leave for someone to notice: 1. They were posted after this PR had already merged. 2. The head sha I cited was one commit behind the head that actually merged. I verified The verdict's substance is unaffected, and here is the check rather than the assertion. The control matters: an empty diff can also mean a pathspec silently matched nothing, so it is shown here to prove the command form does detect a real difference. The one-character So ✅ GOOD TO MERGE holds for what actually landed — but it should have named |
…one (#618) BREAKING CHANGE to a public attribute. The PCAP and PCAP-NG readers filled these two from opposite wire fields, so `frame.len` meant the captured length out of a `.pcap` and the on-wire length out of a `.pcapng`. * `Frame.read` wrote `len=incl_len, cap_len=orig_len` and now writes `len=orig_len, cap_len=incl_len`, matching `toolkit.pcapng.block2frame`. Code reading either attribute from a `.pcap` now gets the other field's value, and for a truncated frame that is a different number rather than a relabelling. * Which reader to move was a decision, not a typo fix: the PCAP spelling is the older of the two (`c43892af`, 2022-01-11, docstrings agreeing a day later) and the PCAP-NG one arrived 15 months later (`25f216f4`). Both were self-consistent. Wireshark's `packet-frame.c` breaks the tie -- `frame.len` is "Frame length on the wire", `frame.cap_len` is "Frame length stored into the capture file", and `frame_len < cap_len` raises `frame.len_lt_caplen`, `PI_MALFORMED` -- so the later spelling is the one that fits the names. * The data model documented the inverted meanings; its docstrings now match. * `_decode_next_layer` is handed `frame.cap_len`, not `frame.len`: it needs the octets present, which is the value it already got, so dissection is unchanged. No test pins that line and the comment there records why. * New `test_frame_length_runtime.py` covers both readers on the only frames that can tell the two fields apart -- the truncated ones #614 added. `tests/protocols/misc/ tests/toolkit/ tests/dumpkit/` passes 116 tests and 234 subtests; `tests/foundation/ tests/integration/` passes 315 and 489. The new module is 6 tests and 28 subtests, and fails on `main` with `96 != 1200`. Fixes #618
…one (#618) BREAKING CHANGE to a public attribute. The PCAP and PCAP-NG readers filled these two from opposite wire fields, so `frame.len` meant the captured length out of a `.pcap` and the on-wire length out of a `.pcapng`. * `Frame.read` wrote `len=incl_len, cap_len=orig_len` and now writes `len=orig_len, cap_len=incl_len`, matching `toolkit.pcapng.block2frame`. Code reading either attribute from a `.pcap` now gets the other field's value, and for a truncated frame that is a different number rather than a relabelling. * Which reader to move was a decision, not a typo fix: the PCAP spelling is the older of the two (`c43892af`, 2022-01-11, docstrings agreeing a day later) and the PCAP-NG one arrived 15 months later (`25f216f4`). Both were self-consistent. Wireshark's `packet-frame.c` breaks the tie -- `frame.len` is "Frame length on the wire", `frame.cap_len` is "Frame length stored into the capture file", and `frame_len < cap_len` raises `frame.len_lt_caplen`, `PI_MALFORMED` -- so the later spelling is the one that fits the names. * The data model documented the inverted meanings; its docstrings now match. * `_decode_next_layer` is handed `frame.cap_len`, not `frame.len`: it needs the octets present, which is the value it already got, so dissection is unchanged. No test pins that line and the comment there records why. * New `test_frame_length_runtime.py` covers both readers on the only frames that can tell the two fields apart -- the truncated ones #614 added. `tests/protocols/misc/ tests/toolkit/ tests/dumpkit/` passes 116 tests and 234 subtests; `tests/foundation/ tests/integration/` passes 315 and 489. The new module is 6 tests and 28 subtests, and fails on `main` with `96 != 1200`. Fixes #618
…one (#618) (#635) BREAKING CHANGE to a public attribute. The PCAP and PCAP-NG readers filled these two from opposite wire fields, so `frame.len` meant the captured length out of a `.pcap` and the on-wire length out of a `.pcapng`. * `Frame.read` wrote `len=incl_len, cap_len=orig_len` and now writes `len=orig_len, cap_len=incl_len`, matching `toolkit.pcapng.block2frame`. Code reading either attribute from a `.pcap` now gets the other field's value, and for a truncated frame that is a different number rather than a relabelling. * Which reader to move was a decision, not a typo fix: the PCAP spelling is the older of the two (`c43892af`, 2022-01-11, docstrings agreeing a day later) and the PCAP-NG one arrived 15 months later (`25f216f4`). Both were self-consistent. Wireshark's `packet-frame.c` breaks the tie -- `frame.len` is "Frame length on the wire", `frame.cap_len` is "Frame length stored into the capture file", and `frame_len < cap_len` raises `frame.len_lt_caplen`, `PI_MALFORMED` -- so the later spelling is the one that fits the names. * The data model documented the inverted meanings; its docstrings now match. * `_decode_next_layer` is handed `frame.cap_len`, not `frame.len`: it needs the octets present, which is the value it already got, so dissection is unchanged. No test pins that line and the comment there records why. * New `test_frame_length_runtime.py` covers both readers on the only frames that can tell the two fields apart -- the truncated ones #614 added. `tests/protocols/misc/ tests/toolkit/ tests/dumpkit/` passes 116 tests and 234 subtests; `tests/foundation/ tests/integration/` passes 315 and 489. The new module is 6 tests and 28 subtests, and fails on `main` with `96 != 1200`. Fixes #618
JarryShaw
added
breaking
1 participant
pcapkit/protocols/misc/pcap/frame.py:199seededpacket['bytesorder']where the reader consultspacket['byteorder']. One character — and the reason it survived is the interesting half: there was no big-endian.pcapin this repository at all, so the corrected path would have been exactly as untested as the broken one. Most of this change is the fixture.Fixes #605
The defect
misc/pcap/frame.py:176, inpack()packet['byteorder']— correctmisc/pcap/frame.py:199, inunpack()packet['bytesorder']— the typoschema/misc/pcap/frame.py:29, inbyteorder_callbackpacket.get('byteorder', sys.byteorder)The callback is attached to all four record-header fields —
ts_sec(:39),ts_usec(:41),incl_len(:43),orig_len(:45) — so the.get()never found the key and every one of them was read in the host's order rather than the file's. On a little-endian host reading a little-endian capture that fallback is right by coincidence, which is why every fixture and every CI runner here has always passed.pack()eleven lines earlier spells it correctly, which is what marks this as a slip rather than a second key deliberately named.The reported crash reproduces, and it is the second symptom
Measured on the unfixed tree against
big_endian.pcap, whose first record really holdsts_sec=1500000000,ts_usec=123456,incl_len=74:Every field byte-swapped, and the frame dated 1970-02-05 instead of 2017-07-14.
incl_lenis the payload length, so that one record consumed the whole file: three frames came back as one, with no error at all. Driven throughpcapkit.extract(), the read that follows is then handed a negative payload length:with
packet = {'bytesorder': 'big', '__length__': -1241513732, ...}in the frame — the wrong key and its consequence in one dict. So this is a correctness defect first and an availability one second, andincl_lenfeeding length arithmetic is what makes the silent half worse than the loud one.The fixture, which is the deliverable that matters
New
examples/generators/endian.py, wired intomake_samples.py. Reproducible, deterministic, no network, no committed binary blob (examples/captures/is gitignored, as for every other generated capture):big_endian.pcapa1 b2 c3 d4big_endian_nanosecond.pcapa1 b2 3c 4dlittle_endian.pcapd4 c3 b2 a1Three design points worth reviewing:
big_endian.pcap— identical timestamps, identical lengths, byte-identical packet data — so the tests can assert that the container's byte order makes no difference to what is read out, rather than only that the big-endian file matches numbers written down in a test. That property is not assertable from one file alone.pcap.py. The containers are packed by hand withstruct, becausewrpcapwrites the host's byte order and offers no way to ask for the other one — and because the container's byte order is the whole subject here, so spelling it out where it can be read beats delegating it.snaplen, soincl_len=96andorig_len=1200. That is what a snapshot limit really does, and frames 1 and 2 (where the two are equal) cannot show that the fields are read separately rather than one being read and used for both. It parses cleanly, decoding toEthernet:IPv4:UDP:Raw.Regenerating twice into a temporary directory gives identical SHA-256s, so the fixtures are reproducible rather than snapshots of one run.
Failing, then passing
tests/protocols/misc/pcap/test_frame_endian_runtime.pydrives all three fixtures throughextract(), and derives its expectations from the files themselves by walking each record chain withstructas well as writing them down, so a fixture regenerated into something else fails loudly instead of moving the goalposts. It asserts all four fields,time_epoch, the payload boundary andbytes(frame)per record, and that the two containers are read alike.A unit-tier case in
test_header_frame_unit.pybuilds a two-record big-endian capture in memory, so the regression is also caught by the fixture-free selectionunit-tests.ymlruns on every push — not only by the tier that needsmake samplesfirst.Before the fix:
The three fixture-backed failures are that
ValueError; the in-memory one isAssertionError: 3106905 != 1500000000. The one that passes istest_little_endian_twin_is_unaffected, and it passes on both trees deliberately — that is what shows the records themselves are not the variable.After the fix,
4 passed, 12 subtests passed, and the big-endian and little-endian files parse to identical values field for field:Wider runs:
tests/protocols/misc/with the changelog, tier-guard and docstring-contract suites, 131 passed, 219 subtests; the full unit tier asunit-tests.ymlselects it, 1284 passed, 5 skipped, 2836 subtests (18m43s). Rebased ontoorigin/main(4529fdb1f), one commit, andpython util/changelog_md.py --checkexits 0.Also changed
byteorder_callbacknow records that it is the definition of the key, and that itssys.byteorderfallback — which a standalone schema genuinely needs — makes a misspelled key indistinguishable from an absent one. That is what hid this for as long as it hid, and it is the part a reader offrame.pycannot see.Found, deliberately not fixed
Frame.readfillslenandcap_lenthe opposite way round from the PCAP-NG reader, and this fixture is the first thing in the repository able to show it.misc/pcap/frame.py:262-263setslen=incl_len(captured) andcap_len=orig_len(on the wire);toolkit/pcapng.py:270-271setslen=block.original_lenandcap_len=block.captured_len. So the same sliced capture read as PCAP and as PCAP-NG reports the two fields swapped relative to each other. Frame 3 makes it visible —len=96 cap_len=1200— where every pre-existing fixture hasincl_len == orig_lenand cannot.Left alone on purpose, and not pinned by the new tests either (they assert the unambiguous
frame_info.*fields instead):lenis passed to_decode_next_layeras the number of octets available to parse, so it is load-bearing as the captured length on the PCAP path. Straightening the names out means deciding which of the two readers is wrong and changing behaviour a caller may depend on, which is a separate change and its own issue — not something to smuggle into a one-character byte-order fix.Minor, and also untouched: the comment at
.github/workflows/unit-tests.yml:121-122says the Scapy extra is there "forexamples/generators/pcap.pyandlegacy.py".endian.pyneeds it too now. Nothing breaks — the extra is installed either way — but the comment is a line stale.