diff --git a/CHANGELOG.md b/CHANGELOG.md index 595c1a77e7..2b953c59d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,8 @@ This is the resolution of #548, which reported `TransType.L2TP` (115) as registe - **Fixed** -- `httpv1`'s `_RE_METHOD` was unanchored and `re.match` anchors only at the start, so it prefix-matched, and the request-line reader then passed the whole `para1` to `Method.get` rather than the captured `method` group. Together those meant `b'Get'` matched on the single character `G`, satisfied the guard that decides a start-line is a request, and handed the entire mixed-case token to a lookup that raised on it. Fixing either half alone still gives a wrong answer -- normalising the lookup would parse `b'Get'` as `GET` off a one-character match, and passing the group would parse it as a method named `G`. The pattern is now anchored at both ends and the captured group is what is looked up, so a token that is not a method is a malformed request line rather than a mis-parsed one. Method tokens are case-sensitive per [RFC 9110 Section 9.1](https://datatracker.ietf.org/doc/html/rfc9110#section-9.1), so no `re.I` was added: `GET` parses, `Get` and `get` are rejected (#583). - **Fixed** -- `_RE_STATUS` in the same reader carried the same unanchored prefix defect, found by auditing `_RE_METHOD`'s siblings, and it escaped as the wrong exception type. That pattern is only a guard -- the value is taken from `int(para2)` on the raw token -- so a prefix match let a malformed status past the guard and then out of `int()` uncaught, where `_read_http_header` documents `ProtocolError`. Measured: a status of `200x` raised `ValueError: invalid literal for int() with base 10: b'200x'`, and one of `2000` raised `ValueError: 2000 is not a valid StatusCode`; both are now `ProtocolError`. [RFC 9112 Section 4](https://datatracker.ietf.org/doc/html/rfc9112#section-4) gives `status-code = 3DIGIT`, exactly three, so the anchor is what the grammar already said -- the production lives in HTTP/1.1 because `status-code` is part of its `status-line`, while [RFC 9110 Section 15](https://datatracker.ietf.org/doc/html/rfc9110#section-15) covers the code semantics and the IANA registry rather than the syntax. `_RE_VERSION` was audited at the same time and is safe as it stands, because both of its call sites read the captured group rather than the raw token (#583). - **Fixed** -- `get()`'s documented `default` was ignored on the integer path throughout the generated `pcapkit.const` tree, because `get` delegated the lookup to the enum call and `_missing_` has no access to the caller's `default` -- so `Hardware.get(99999, 0)` raised `ValueError: 99999 is not a valid Hardware` instead of returning the fallback it was handed. The integer path now consults `default` before letting the lookup error escape. `-1`, the placeholder the generated signature already carried, is what separates "no default was supplied" from "a default was supplied and should be used", so a caller that asked for no fallback still gets the error rather than a silent substitution. The sweep #584 asked for puts the scope at 110 of the 118 integer registries, not the three the issue named; the two carrying a bespoke integer fallback of their own, `pcapng` `OptionType` and `reg` `AppType`, are deliberately left alone, since neither drops a default by raising. Not reachable from wire data -- every value a wire field can carry already resolves -- so this is a contract fix rather than a parse fix. Applied to the nine vendor templates as well as the 113 generated modules, and a new test renders the shared template and compares it against the module generated from it, so a regeneration cannot quietly undo it (#584). +- **Fixed** -- reading a big-endian classic PCAP byte-swapped every record header field, and then crashed. `Frame.unpack` seeded the file's declared byte order under the key `bytesorder` where the frame schema's `byteorder_callback` reads `byteorder`, so the lookup never found it 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. Against a big-endian capture, measured before the fix, frame 1 of `big_endian.pcap` read `ts_sec=3106905`, `ts_usec=1088553216` and `incl_len=1241513984` for a record whose real values are `1500000000`, `123456` and `74`, dating the frame to 1970-02-05 rather than to 2017-07-14. `incl_len` is the payload length, so that first record then consumed the whole file and the second was read with a negative payload length, raising `ValueError: read length must be non-negative or -1` out of the schema -- which is the reported crash, and it is the *second* symptom rather than the first. The sibling `Frame.pack` eleven lines earlier spelled the key correctly, which is what marks this as a slip rather than a second key, and the fallback is what made a misspelled key indistinguishable from an absent one; `byteorder_callback` now records that it is the definition of the key and why the fallback hides a typo (#605). +- **Added** -- `examples/generators/endian.py`, and the byte-order tests that read what it writes. There was no big-endian `.pcap` in the repository at all, which is why #605 survived its own code review: the one-character fix leaves the corrected path exactly as untested as the broken one. The generator writes three captures -- `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`) -- carrying the *same three records* in each container, so the tests can assert that the 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. Frame 3 is captured short, 1200 octets on the wire cut to a 96-octet `snaplen`, so `incl_len` and `orig_len` differ and cannot both be satisfied by one byte-swapped value. `test_frame_endian_runtime.py` drives all three through `extract()` and walks each file's record chain with `struct` to derive its own expectations; a unit-tier case in `test_header_frame_unit.py` builds a two-record big-endian capture in memory instead, so the regression is also caught by the fixture-free selection CI runs on every push. All four fail on the unfixed tree -- the three fixture-backed ones by that `ValueError`, the in-memory one by `AssertionError: 3106905 != 1500000000` -- while the little-endian twin passes on both trees, which is what shows the records themselves are not the variable (#605). - **Changed** -- `tests/protocols/transport/test_tcp_udp_unit.py` now reaches the MP_JOIN dispatchers through `TCP()` itself, instead of assigning a Python `set` to `_flags` on a bare `TCP.__new__(TCP)`. A `set` answers the membership tests `_make_mptcp_join` and `_read_mptcp_join` use, so every flag branch ran and both TCP modules read 100% statement and branch coverage -- while the attribute had neither the `aenum.IntFlag` type production assigns nor the ordering that governs when it exists at all, which is how #587 stayed invisible behind that number and how the `cast('Enum_Flags', 0)` no-op behind it went unnoticed too. Measured on the rewrite, against the 17 tests of that file: revert #587's hoist and two of them fail with `AttributeError: 'TCP' object has no attribute '_flags'` where all 17 passed before; restore the `cast` and two fail with `TypeError: argument of type 'int' is not a container or iterable`, again where all 17 passed. The library is unchanged and the file's tests still pass, so the coverage numbers do not move -- the point is what the same numbers are now worth (#603). - **Fixed** -- documentation. `mptcp_dss_ack_selector`'s note said a corrected field-width lambda "would not have worked" and that fixing it belonged to `pcapkit.corekit.fields.numbers`, which is exactly where #598 then fixed it; the same paragraph sat in `test_tcp_mptcp_length_arithmetic_unit.py`'s module docstring, whose other stale claim was that MP_JOIN "cannot be built through the public `TCP()` constructor at all", true only until #587. A callable-length `NumberField` packs and unpacks both DSS widths now, and wire *absence* was never the obstacle either: `MPTCPDSS.ssn`, `dl_len` and `checksum` have always been `ConditionalField` on the sibling `M` flag, so the class already relied on that wrapper to keep a field off the wire. The `SwitchField` form is kept for the narrower reason the note now gives -- `ConditionalField`'s `length` forwards to the wrapped field without consulting the condition, so it is safe here only because `Schema.pack` and `Schema.unpack` special-case that wrapper by name, whereas a `SwitchField` always resolves to a concrete field. Replacing it would be a behaviour change and is not made (#603). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index efdc2ff061..985d6578c1 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -814,6 +814,48 @@ pull requests between #326 and #509. well as the 113 generated modules, and a new test renders the shared template and compares it against the module generated from it, so a regeneration cannot quietly undo it (#584). +* **Fixed** -- reading a big-endian classic PCAP byte-swapped every record header + field, and then crashed. ``Frame.unpack`` seeded the file's declared byte order + under the key ``bytesorder`` where the frame schema's ``byteorder_callback`` + reads ``byteorder``, so the lookup never found it 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. + Against a big-endian capture, measured before the fix, frame 1 of + ``big_endian.pcap`` read ``ts_sec=3106905``, ``ts_usec=1088553216`` and + ``incl_len=1241513984`` for a record whose real values are ``1500000000``, + ``123456`` and ``74``, dating the frame to 1970-02-05 rather than to + 2017-07-14. ``incl_len`` is the payload length, so that first record + then consumed the whole file and the second was read with a negative payload + length, raising ``ValueError: read length must be non-negative or -1`` out of + the schema -- which is the reported crash, and it is the *second* symptom + rather than the first. The sibling ``Frame.pack`` eleven lines earlier spelled + the key correctly, which is what marks this as a slip rather than a second key, + and the fallback is what made a misspelled key indistinguishable from an absent + one; ``byteorder_callback`` now records that it is the definition of the key + and why the fallback hides a typo (#605). +* **Added** -- ``examples/generators/endian.py``, and the byte-order tests that + read what it writes. There was no big-endian ``.pcap`` in the repository at all, + which is why #605 survived its own code review: the one-character fix leaves the + corrected path exactly as untested as the broken one. The generator writes three + captures -- ``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``) -- + carrying the *same three records* in each container, so the tests can assert + that the 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. Frame 3 is + captured short, 1200 octets on the wire cut to a 96-octet ``snaplen``, so + ``incl_len`` and ``orig_len`` differ and cannot both be satisfied by one + byte-swapped value. ``test_frame_endian_runtime.py`` drives all three through + ``extract()`` and walks each file's record chain with ``struct`` to derive its + own expectations; a unit-tier case in ``test_header_frame_unit.py`` builds a + two-record big-endian capture in memory instead, so the regression is also + caught by the fixture-free selection CI runs on every push. All four fail on the + unfixed tree -- the three fixture-backed ones by that ``ValueError``, the + in-memory one by ``AssertionError: 3106905 != 1500000000`` -- while the + little-endian twin passes on both trees, which is what shows the records + themselves are not the variable (#605). * **Changed** -- ``tests/protocols/transport/test_tcp_udp_unit.py`` now reaches the MP_JOIN dispatchers through ``TCP()`` itself, instead of assigning a Python ``set`` to ``_flags`` on a bare ``TCP.__new__(TCP)``. A ``set`` answers the diff --git a/examples/generators/endian.py b/examples/generators/endian.py new file mode 100644 index 0000000000..b5c0fb05a4 --- /dev/null +++ b/examples/generators/endian.py @@ -0,0 +1,257 @@ +# -*- coding: utf-8 -*- +"""Generate the byte-order ``.pcap`` sample fixtures. + +Classic PCAP declares its own byte order in the magic number of the global +header, and every 32-bit field after that -- the global header's own, and the +four in each record header -- is written in *that* order rather than in the +host's. A reader therefore carries a branch for it, and until this module there +was no ``.pcap`` fixture in the repository to take the big-endian side of that +branch: every capture under ``examples/captures/`` was little-endian, so on a +little-endian runner the branch was never taken and reading the host's order +instead of the file's produced the right answer anyway. + +That is how GitHub issue #605 survived: ``Frame.unpack`` seeded the byte order +under the key ``bytesorder`` where ``byteorder_callback`` in +:file:`pcapkit/protocols/schema/misc/pcap/frame.py` reads ``byteorder``, so the +lookup always missed and always fell back to :data:`sys.byteorder`. Reading a +big-endian capture then byte-swapped ``ts_sec``, ``ts_usec``, ``incl_len`` and +``orig_len`` -- and because ``incl_len`` is the payload length, the first record +swallowed the rest of the file and the read that followed was handed a negative +payload length, raising +``ValueError: read length must be non-negative or -1`` out of +:file:`pcapkit/protocols/schema/schema.py`. A one-character fix, with nothing in +the suite able to tell whether it worked. + +=============================== ========================================== +Fixture Exercises +=============================== ========================================== +``big_endian.pcap`` magic ``a1 b2 c3 d4`` -- big-endian, + microsecond timestamps +``little_endian.pcap`` magic ``d4 c3 b2 a1`` -- the same three + records in the little-endian container +``big_endian_nanosecond.pcap`` magic ``a1 b2 3c 4d`` -- big-endian, + nanosecond timestamps +=============================== ========================================== + +The little-endian twin is the point of the set rather than a spare. It carries +the *same three records* as ``big_endian.pcap`` -- identical timestamps, +identical lengths, byte-identical packet data -- so a test can assert that the +two files parse to the same values instead of only that the big-endian one +parses to values hardcoded in the test. That is the property the byte-order +branch exists to provide, and it is not assertable from one file alone. + +The packet data comes from :mod:`scapy`, as it does in :file:`pcap.py`, so the +frames carry real headers and real checksums. The *containers* are packed here +by hand with :mod:`struct`, for two reasons: :func:`scapy.utils.wrpcap` writes +the host's byte order and offers no way to ask for the other one, and the byte +order of the container is the whole subject of these fixtures -- spelling it out +where it can be read is worth more than delegating it. + +Frame 3 is captured short: 1200 octets on the wire, ``snaplen`` 96 in the +global header, so ``incl_len`` is 96 and ``orig_len`` is 1200. That is what a +snapshot limit really does, and it makes the fixture prove something a set of +full-length frames cannot -- that the two fields are read separately, rather +than one of them being read and used for both. Frames 1 and 2 have them equal +and so cannot tell the difference. + +""" + +from __future__ import annotations + +import hashlib +import pathlib +import struct +from typing import TYPE_CHECKING, NamedTuple + +from scapy.all import ICMP, IP, UDP, Ether, Raw # pylint: disable=no-name-in-module + +if TYPE_CHECKING: + from typing import Literal + +__all__ = ['generate'] + +#: Repository root, i.e. the grandparent of the directory holding this file. +ROOT = pathlib.Path(__file__).resolve().parents[2] +#: Default destination directory for the generated captures. +SAMPLE = ROOT / 'examples' / 'captures' + +#: PCAP file magic numbers, keyed by endianness and nanosecond-resolution flag. +#: The same table as ``_MAGIC_NUM`` in +#: :file:`pcapkit/protocols/misc/pcap/header.py`, which is the reader these +#: fixtures are written for; it is repeated rather than imported so that a +#: fixture stays generatable without :mod:`pcapkit` importing cleanly. +MAGIC_NUMBER = { + ('big', False): b'\xa1\xb2\xc3\xd4', + ('big', True): b'\xa1\xb2\x3c\x4d', + ('little', False): b'\xd4\xc3\xb2\xa1', + ('little', True): b'\x4d\x3c\xb2\xa1', +} + +#: PCAP version, as every capture in this repository carries. +VERSION = (2, 4) +#: Snapshot length declared by the global header. Frames 1 and 2 are shorter +#: than this and are captured whole; frame 3 is longer and is cut to it. +SNAPLEN = 96 +#: Data link type, i.e. ``LinkType.ETHERNET``. Spelled as the number the file +#: holds, for the same reason as :data:`MAGIC_NUMBER`. +NETWORK = 1 + +#: Capture start time, fixed so that regenerating gives identical files. The +#: same instant :file:`pcap.py` starts its captures at. +EPOCH = 1500000000 + +#: The fixtures this module writes, as ``(file name, byte order, nanosecond +#: flag)``. The two microsecond files carry byte-identical records in the two +#: containers; the nanosecond one holds the same packets, timed more finely. +FIXTURES = ( + ('big_endian.pcap', 'big', False), + ('little_endian.pcap', 'little', False), + ('big_endian_nanosecond.pcap', 'big', True), +) # type: tuple[tuple[str, Literal['big', 'little'], bool], ...] + + +class _Record(NamedTuple): + """One record of a capture: a record header's four fields, and its data. + + The record header is *not* stored as octets here, because the octets are + what differs between the three fixtures. :func:`_pack` turns these numbers + into a header in whichever byte order it was asked for. + + """ + + #: Timestamp seconds, i.e. ``ts_sec``. + ts_sec: 'int' + #: Timestamp fraction: microseconds, or nanoseconds in a nanosecond- + #: resolution file. Written to ``ts_usec`` either way. + ts_usec: 'int' + #: Length of the packet as it was on the wire, i.e. ``orig_len``. Equal to + #: ``len(packet)`` unless the capture was cut short by ``snaplen``. + orig_len: 'int' + #: The packet data actually stored, i.e. ``incl_len`` octets of it. + packet: 'bytes' + + +def _filler(length: 'int', tag: 'bytes') -> 'bytes': + """Deterministic opaque payload bytes. + + The same construction :file:`pcap.py` uses, so that these fixtures are as + reproducible as the rest and need no network and no clock. + + Args: + length: Number of octets required. + tag: Seed distinguishing one payload from another. + + Returns: + Exactly ``length`` octets, the same on every machine and every run. + + """ + out = bytearray() + counter = 0 + while len(out) < length: + out += hashlib.sha256(b'%s/%d' % (tag, counter)).digest() + counter += 1 + return bytes(out[:length]) + + +def _records(nanosecond: 'bool' = False) -> 'list[_Record]': + """Build the three records every fixture in this module carries. + + Args: + nanosecond: Whether the timestamp fractions are nanoseconds. Only the + fraction changes: a nanosecond capture holds the same packets at + the same second, timed more finely. + + Returns: + The records, in capture order. + + """ + # Fractions chosen so that no field reads the same after a byte swap, and + # so that a swap gives a number nowhere near a plausible one: 123456 is + # 0x0001e240, which read the wrong way round is 0x40e20100 -- 1088553216 + # microseconds, i.e. eighteen minutes into a second. + fractions = (123456789, 987654321, 456789123) if nanosecond else (123456, 654321, 456789) + + # frame 1: an ICMP echo request, 74 octets on the wire + echo = (Ether(src='00:0c:29:19:dc:61', dst='00:0c:29:7d:1d:b4') + / IP(src='10.20.30.131', dst='10.20.30.130', ttl=64, id=0x4e21) + / ICMP(type=8, id=0x3f21, seq=1) + / Raw(load=_filler(32, b'endian-echo'))) + + # frame 2: a small UDP datagram, 66 octets on the wire + datagram = (Ether(src='00:0c:29:19:dc:61', dst='00:0c:29:7d:1d:b4') + / IP(src='10.20.30.131', dst='10.20.30.130', ttl=64, id=0x4e22) + / UDP(sport=41234, dport=9000) + / Raw(load=_filler(24, b'endian-datagram'))) + + # frame 3: a 1200-octet datagram, cut to SNAPLEN by the snapshot limit + bulk = (Ether(src='00:0c:29:19:dc:61', dst='00:0c:29:7d:1d:b4') + / IP(src='10.20.30.131', dst='10.20.30.130', ttl=64, id=0x4e23) + / UDP(sport=41234, dport=9000) + / Raw(load=_filler(1158, b'endian-bulk'))) + + frames = [bytes(echo), bytes(datagram), bytes(bulk)] + if [len(frame) for frame in frames] != [74, 66, 1200]: + raise RuntimeError(f'unexpected frame lengths: {[len(frame) for frame in frames]}') + + return [ + _Record(EPOCH, fractions[0], len(frames[0]), frames[0]), + _Record(EPOCH + 1, fractions[1], len(frames[1]), frames[1]), + _Record(EPOCH + 2, fractions[2], len(frames[2]), frames[2][:SNAPLEN]), + ] + + +def _pack(byteorder: 'Literal["big", "little"]', nanosecond: 'bool', + entries: 'list[_Record]') -> 'bytes': + """Pack a whole capture file, global header and records. + + Args: + byteorder: Byte order to write every 32-bit field in. + nanosecond: Whether to declare nanosecond-resolution timestamps, which + is a property of the magic number rather than of the records. + entries: The records to write, in capture order. + + Returns: + The file's octets. + + """ + endian = '>' if byteorder == 'big' else '<' + + out = bytearray(MAGIC_NUMBER[(byteorder, nanosecond)]) + out += struct.pack(f'{endian}HHiIII', VERSION[0], VERSION[1], 0, 0, SNAPLEN, NETWORK) + + for entry in entries: + out += struct.pack(f'{endian}IIII', entry.ts_sec, entry.ts_usec, + len(entry.packet), entry.orig_len) + out += entry.packet + + return bytes(out) + + +def generate(dest: 'pathlib.Path | None' = None) -> 'list[pathlib.Path]': + """Write the byte-order ``.pcap`` sample fixtures. + + Args: + dest: Destination directory; ``examples/captures/`` under the repository + root, if not given. Created if it does not exist. + + Returns: + The paths written, in the order they were written. + + """ + dest = SAMPLE if dest is None else pathlib.Path(dest) + dest.mkdir(parents=True, exist_ok=True) + + written = [] # type: list[pathlib.Path] + for name, byteorder, nanosecond in FIXTURES: + path = dest / name + path.write_bytes(_pack(byteorder, nanosecond, _records(nanosecond))) + written.append(path) + + return written + + +if __name__ == '__main__': + for sample in generate(): + data = sample.read_bytes() + print('%-32s %6d octets magic %s' % ( + sample.relative_to(ROOT), len(data), data[:4].hex(' '))) diff --git a/examples/generators/make_samples.py b/examples/generators/make_samples.py index c07c24a452..b74aad545a 100644 --- a/examples/generators/make_samples.py +++ b/examples/generators/make_samples.py @@ -18,6 +18,8 @@ Module Fixtures =================== ========================================================== :file:`pcap.py` the ``.pcap`` captures the unit and runtime tests read +:file:`endian.py` the big-endian ``.pcap`` captures, and their little-endian + twin, that the byte-order tests read :file:`pcapng.py` the ``.pcapng`` captures the regression tests read :file:`legacy.py` the extra captures ``examples/legacy_smoke/`` reads :file:`options.py` the ``options-*.pcap`` option-coverage captures @@ -50,7 +52,7 @@ #: own construction output, so a failure in it is a statement about the library #: rather than about the fixture -- and reading it after the others have already #: printed keeps that distinction visible in the log. -GENERATORS = ('pcap', 'pcapng', 'legacy', 'options') +GENERATORS = ('pcap', 'endian', 'pcapng', 'legacy', 'options') def load(name: 'str') -> 'ModuleType': diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index 2b67d3c4e9..ad71f70bc7 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -196,7 +196,7 @@ def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_Frame """ if cast('Optional[Schema_Frame]', self.__header__) is None: packet = kwargs.get('__packet__', {}) # packet data - packet['bytesorder'] = self._ghdr.magic_number.byteorder + packet['byteorder'] = self._ghdr.magic_number.byteorder self.__header__ = cast('Schema_Frame', self.__schema__.unpack(self._file, length, packet)) # type: ignore[call-arg,misc] return self.read(length, **kwargs) diff --git a/pcapkit/protocols/schema/misc/pcap/frame.py b/pcapkit/protocols/schema/misc/pcap/frame.py index 28293ec62e..1bff3d2c55 100644 --- a/pcapkit/protocols/schema/misc/pcap/frame.py +++ b/pcapkit/protocols/schema/misc/pcap/frame.py @@ -25,6 +25,20 @@ def byteorder_callback(field: 'Field', packet: 'dict[str, Any]') -> 'None': field: Field instance. packet: Packet data. + Notes: + ``byteorder`` is the key a caller has to seed, and this function is what + defines it: :meth:`Frame.pack ` + and :meth:`Frame.unpack ` + both write it from the global header's magic number. The fallback to + :data:`sys.byteorder` is for a schema packed or unpacked on its own, with + no global header to ask -- which also means a *misspelled* key looks + exactly like an absent one and reports nothing. That is what hid GitHub + issue #605: ``unpack`` wrote ``bytesorder``, so every field here was read + in the host's order rather than the file's, which is right by coincidence + on a little-endian capture and byte-swapped on a big-endian one. See + :file:`tests/protocols/misc/pcap/test_frame_endian_runtime.py` for the + fixtures that now take the other side of the branch. + """ field._byteorder = packet.get('byteorder', sys.byteorder) diff --git a/tests/protocols/misc/pcap/test_frame_endian_runtime.py b/tests/protocols/misc/pcap/test_frame_endian_runtime.py new file mode 100644 index 0000000000..bb3df1d1d0 --- /dev/null +++ b/tests/protocols/misc/pcap/test_frame_endian_runtime.py @@ -0,0 +1,245 @@ +# -*- coding: utf-8 -*- +"""A big-endian classic PCAP, read on a little-endian host. + +Classic PCAP declares its byte order in the magic number of the global header, +and the four fields of every record header -- ``ts_sec``, ``ts_usec``, +``incl_len`` and ``orig_len`` -- are written in that order rather than in the +reading host's. Nothing in this suite read a big-endian ``.pcap`` before, because +no such capture existed here: every fixture was little-endian, so on a +little-endian runner a reader that ignored the file's declared order and used +:data:`sys.byteorder` produced exactly the right answer. + +That is what GitHub issue #605 was. +:meth:`Frame.unpack ` seeded the +order under the key ``bytesorder``, while ``byteorder_callback`` in +:file:`pcapkit/protocols/schema/misc/pcap/frame.py` reads ``byteorder``, so the +``.get()`` never found the key and always fell back to the host's order -- the +sibling :meth:`Frame.pack ` +eleven lines earlier spelled it correctly, which is what marks it as a slip +rather than a second key. Measured on the unfixed tree against +``big_endian.pcap``, whose first record really holds ``ts_sec=1500000000``, +``ts_usec=123456`` and ``incl_len=74``: frame 1 came back with +``ts_sec=3106905``, ``ts_usec=1088553216`` and ``incl_len=1241513984``, i.e. +every field byte-swapped, dated 1970-02-05 rather than 2017-07-14. And because +``incl_len`` is the payload length, that first record swallowed the rest of the +file and the read that followed was handed a negative payload length, raising +``ValueError: read length must be non-negative or -1`` from +:file:`pcapkit/protocols/schema/schema.py`. So the defect was a wrong answer +first and a crash second. + +The fixtures come from :file:`examples/generators/endian.py`, and the set is +built around one property: ``big_endian.pcap`` and ``little_endian.pcap`` carry +the *same three records* -- same timestamps, same lengths, byte-identical packet +data -- in the two containers. A test against the big-endian file alone can only +check it against numbers written down here; against its twin it can check that +the byte order of the container makes no difference to what is read out of it, +which is the property the byte-order branch exists to provide. + +Expectations are taken from the files themselves with :mod:`struct` as well as +written down, so a fixture regenerated into something else fails here rather +than quietly moving the goalposts. + +""" +from __future__ import annotations + +import importlib.util +import struct +import unittest +from decimal import Decimal + +from tests._support import purge_modules, sample_path + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: The three records every fixture in :file:`examples/generators/endian.py` +#: carries, as ``(ts_sec, ts_usec, incl_len, orig_len)``. Frame 3 is captured +#: short -- 1200 octets on the wire, cut to the 96-octet ``snaplen`` -- so +#: ``incl_len`` and ``orig_len`` differ there, which frames 1 and 2 cannot show: +#: a reader that read one of the two fields and used it for both would satisfy +#: them and fail here. +MICROSECOND_RECORDS = ( + (1500000000, 123456, 74, 74), + (1500000001, 654321, 66, 66), + (1500000002, 456789, 96, 1200), +) + +#: The same three records of ``big_endian_nanosecond.pcap``, whose magic number +#: (``a1 b2 3c 4d``) declares both big-endian *and* nanosecond timestamps, so +#: ``ts_usec`` counts nanoseconds. +NANOSECOND_RECORDS = ( + (1500000000, 123456789, 74, 74), + (1500000001, 987654321, 66, 66), + (1500000002, 456789123, 96, 1200), +) + + +def record_chain(raw: 'bytes', endian: 'str') -> 'list[tuple[int, tuple[int, int, int, int]]]': + """Walk a classic PCAP's record chain straight out of its octets. + + This is deliberately independent of :mod:`pcapkit`: it is what the + assertions below compare the library's answer against, so it must not share + the code under test. The same idiom as + :file:`tests/protocols/misc/pcap/test_frame_runtime.py`, widened to take the + byte order and to return all four fields. + + Args: + raw: The whole file. + endian: :mod:`struct` byte-order prefix, ``'>'`` or ``'<'``. + + Returns: + One entry per record, as ``(offset of the record, (ts_sec, ts_usec, + incl_len, orig_len))``, where the offset is that of the record header's + first octet. + + """ + chain = [] + offset = 24 # the global header is 24 octets + while offset < len(raw): + fields = struct.unpack_from(f'{endian}IIII', raw, offset) + chain.append((offset, fields)) + offset += 16 + fields[2] # 16-octet record header, then incl_len octets + return chain + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class PcapFrameByteOrderRuntimeTests(unittest.TestCase): + """#605, through :func:`pcapkit.interface.extract`.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def read(self, name: 'str') -> 'tuple[bytes, list]': + """Read a fixture's octets and its frames. + + Args: + name: Bare fixture file name. + + Returns: + The file's octets, and the frames + :func:`pcapkit.interface.extract` parsed out of it. + + """ + from pcapkit.interface import extract + + path = sample_path(name) + with open(path, 'rb') as stream: + raw = stream.read() + + extractor = extract(fin=path, store=True, nofile=True) + return raw, list(extractor.frame) + + def assertRecords(self, name: 'str', magic: 'bytes', endian: 'str', + expected: 'tuple[tuple[int, int, int, int], ...]', + divisor: 'int') -> 'None': + """Assert a fixture parses to the record headers it actually holds. + + Args: + name: Bare fixture file name. + magic: Magic number the fixture must carry, so that a fixture + regenerated into a different byte order fails loudly here + instead of making the rest of the assertions vacuous. + endian: :mod:`struct` byte-order prefix the file is written in. + expected: The four record-header fields of each record. + divisor: What ``ts_usec`` is a fraction of a second in -- 1000000 + for a microsecond capture, 1000000000 for a nanosecond one. + + """ + raw, frames = self.read(name) + + self.assertEqual(raw[:4], magic) + chain = record_chain(raw, endian) + # the fixture holds what this module says it holds + self.assertEqual(tuple(fields for _, fields in chain), expected) + # and reading it yields one frame per record, not one frame for the file + self.assertEqual(len(frames), len(expected)) + + for frame, (offset, fields) in zip(frames, chain): + ts_sec, ts_usec, incl_len, orig_len = fields + with self.subTest(frame=frame.info.number): + info = frame.info.frame_info + self.assertEqual(info.ts_sec, ts_sec) + self.assertEqual(info.ts_usec, ts_usec) + self.assertEqual(info.incl_len, incl_len) + self.assertEqual(info.orig_len, orig_len) + + # the timestamp the caller actually reads, which is where a + # swapped ts_sec shows up as an instant in 1970 + self.assertEqual(frame.info.time_epoch, + ts_sec + Decimal(ts_usec) / divisor) + self.assertEqual(frame.info.time.year, 2017) + + # and the payload boundary, which is what incl_len decides: a + # swapped incl_len runs the first record to the end of the file + self.assertEqual(frame.info.packet, + raw[offset + 16:offset + 16 + incl_len]) + self.assertEqual(bytes(frame), raw[offset:offset + 16 + incl_len]) + + def test_big_endian_record_headers_are_read_in_the_files_byte_order(self) -> None: + """#605: ``a1 b2 c3 d4``, microsecond timestamps. + + On the unfixed tree this raises ``ValueError: read length must be + non-negative or -1`` before reaching any assertion: frame 1's swapped + ``incl_len`` of 1241513984 consumes the whole file, and the read that + follows is handed a negative payload length. + + """ + self.assertRecords('big_endian.pcap', b'\xa1\xb2\xc3\xd4', '>', + MICROSECOND_RECORDS, 1_000_000) + + def test_big_endian_nanosecond_record_headers_are_read_in_the_files_byte_order(self) -> None: + """#605 for ``a1 b2 3c 4d``, the big-endian nanosecond magic number. + + The nanosecond flag and the byte order come out of the same magic + number, and both have to be honoured at once: ``ts_usec`` is read + big-endian and then divided by a thousand million rather than by a + million. + + """ + self.assertRecords('big_endian_nanosecond.pcap', b'\xa1\xb2\x3c\x4d', '>', + NANOSECOND_RECORDS, 1_000_000_000) + + def test_little_endian_twin_is_unaffected(self) -> None: + """The control: the same three records in the little-endian container. + + This passes on the unfixed tree too, and that is what it is for. It + pins the expectations of the big-endian tests to a file whose byte + order was never in question, so a failure there cannot be blamed on the + records themselves being wrong. + + """ + self.assertRecords('little_endian.pcap', b'\xd4\xc3\xb2\xa1', '<', + MICROSECOND_RECORDS, 1_000_000) + + def test_the_two_containers_are_read_alike(self) -> None: + """The two byte orders are two spellings of the same three records. + + Asserted field by field rather than by comparing the parsed objects, + which carry frame numbers and nested protocol instances that do not + compare equal. The packet data is compared too: it is the one part of a + record that is *not* byte-swapped between the two files, so if it ever + differs, the fixtures have diverged and neither test above means what it + says. + + """ + big_raw, big_frames = self.read('big_endian.pcap') + little_raw, little_frames = self.read('little_endian.pcap') + + self.assertEqual(len(big_raw), len(little_raw)) + self.assertEqual(len(big_frames), len(little_frames)) + self.assertNotEqual(big_raw, little_raw) # the containers do differ + + for big, little in zip(big_frames, little_frames): + with self.subTest(frame=big.info.number): + self.assertEqual(big.info.frame_info.ts_sec, little.info.frame_info.ts_sec) + self.assertEqual(big.info.frame_info.ts_usec, little.info.frame_info.ts_usec) + self.assertEqual(big.info.frame_info.incl_len, little.info.frame_info.incl_len) + self.assertEqual(big.info.frame_info.orig_len, little.info.frame_info.orig_len) + self.assertEqual(big.info.time_epoch, little.info.time_epoch) + self.assertEqual(big.info.time, little.info.time) + self.assertEqual(big.info.protocols, little.info.protocols) + self.assertEqual(big.info.packet, little.info.packet) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/misc/pcap/test_header_frame_unit.py b/tests/protocols/misc/pcap/test_header_frame_unit.py index d6713a2644..4f370b5573 100644 --- a/tests/protocols/misc/pcap/test_header_frame_unit.py +++ b/tests/protocols/misc/pcap/test_header_frame_unit.py @@ -399,6 +399,78 @@ def test_frame_time_is_timezone_aware_utc(self) -> None: datetime.datetime.fromtimestamp(float(frame.info.time_epoch), datetime.timezone.utc)) + def test_frame_header_is_read_in_the_files_byte_order(self) -> None: + """#605: a big-endian record header, read on a little-endian host. + + :meth:`Frame.unpack ` + seeded the file's byte order under the key ``bytesorder``, where + ``byteorder_callback`` in + :file:`pcapkit/protocols/schema/misc/pcap/frame.py` reads ``byteorder``, + so the lookup always missed and always fell back to + :data:`sys.byteorder`. On a little-endian host reading a little-endian + capture that is the right answer by coincidence, which is why every + fixture here passed; on a big-endian capture all four record-header + fields came back byte-swapped. + + The capture is built here rather than read from + :file:`examples/captures/`, so this stays in the unit tier and runs in + the selection :file:`.github/workflows/unit-tests.yml` uses -- the + fixture-backed counterpart, which goes through + :func:`pcapkit.interface.extract` against + :file:`examples/generators/endian.py`'s captures, is + :file:`tests/protocols/misc/pcap/test_frame_endian_runtime.py` and runs + only once the fixtures exist. + + Two records, because the swap is a wrong answer *and* a crash: frame 1's + ``incl_len`` of 60 reads as 1006632960, which consumes the rest of the + file, and frame 2 is then asked to read a negative payload length -- + ``ValueError: read length must be non-negative or -1`` out of + :file:`pcapkit/protocols/schema/schema.py`. + + """ + from pcapkit.const.reg.linktype import LinkType + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.pcap.header import Header + + records = ( + (1500000000, 123456, b'\x02\x00\x00\x00' + bytes(range(56))), + (1500000001, 654321, b'\x02\x00\x00\x00' + bytes(range(40))), + ) + + # magic a1b2c3d4: big-endian, microsecond timestamps + raw = b'\xa1\xb2\xc3\xd4' + struct.pack('>HHiIII', 2, 4, 0, 0, 65535, + int(LinkType.NULL)) + for ts_sec, ts_usec, packet in records: + raw += struct.pack('>IIII', ts_sec, ts_usec, len(packet), len(packet)) + raw += packet + + stream = io.BytesIO(raw) + header = Header(stream) + self.assertEqual(header.byteorder, 'big') + self.assertFalse(header.nanosecond) + + offset = 24 + for number, (ts_sec, ts_usec, packet) in enumerate(records, start=1): + # the engine reads every frame off one handle, so a record whose + # length was read in the wrong order desynchronises the ones after it + self.assertEqual(stream.tell(), offset) + + frame = Frame(stream, num=number, header=header.info) + info = frame.info.frame_info + + self.assertEqual(info.ts_sec, ts_sec) + self.assertEqual(info.ts_usec, ts_usec) + self.assertEqual(info.incl_len, len(packet)) + self.assertEqual(info.orig_len, len(packet)) + self.assertEqual(frame.info.time_epoch, + ts_sec + Decimal(ts_usec) / 1_000_000) + self.assertEqual(frame.info.packet, packet) + + offset += 16 + len(packet) + self.assertEqual(stream.tell(), offset) + + self.assertEqual(offset, len(raw)) + if __name__ == '__main__': unittest.main()