diff --git a/conda/build b/conda/build index c227083464..56a6051ca2 100644 --- a/conda/build +++ b/conda/build @@ -1 +1 @@ -0 \ No newline at end of file +1 \ No newline at end of file diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index 11a19a7f29..544ab1a254 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -103,7 +103,11 @@ class Extractor(Generic[_P]): _ifnm: 'str' #: Output file name. _ofnm: 'Optional[str]' - #: Output file extension. + #: Output file extension, bare, i.e. without the leading ``.`` -- + #: ``'json'``, not ``'.json'``. Normalised by + #: :meth:`~pcapkit.foundation.extraction.Extractor.make_name`, whose + #: docstring is the contract; the engines compose a per-frame filename + #: as ``f'{name}.{ext._fext}'`` and supply the dot themselves. _fext: 'Optional[str]' #: Auto extract flag. It indicates if the extraction process should @@ -551,7 +555,9 @@ def make_name(cls, fin: 'str | IO[bytes]' = 'in.pcap', fout: 'str' = 'out', 0. input filename 1. output filename / directory name 2. output format - 3. output file extension (without ``.``) + 3. output file extension, bare, i.e. **without** the leading ``.``, so + that a caller composing a per-frame filename writes + ``f'{name}.{ext}'`` 4. if split each frame into different files Args: @@ -587,10 +593,20 @@ def make_name(cls, fin: 'str | IO[bytes]' = 'in.pcap', fout: 'str' = 'out', ofnm = None ext = None else: - ext = cls.__output__[fmt][1] - if ext is None: + registered = cls.__output__[fmt][1] + if registered is None: raise FormatError(f'unknown output format: {fmt}') + # NOTE: ``__output__`` spells its extensions with the leading dot, + # and so does every ``ext=`` handed to + # :func:`pcapkit.foundation.registry.foundation.register_dumper`. + # The value we hand back, though, is documented bare and is used + # bare -- ``Extractor._fext`` reaches the engines, which each + # compose a per-frame name as ``f'{name}.{ext._fext}'``. Leaving the + # dot on is what produced ``Frame 1..json`` (see #358), so + # normalise it away once, here, rather than at six call sites. + ext = registered[1:] if registered.startswith('.') else registered + if (parent := os.path.split(fout)[0]): os.makedirs(parent, exist_ok=True) @@ -598,7 +614,8 @@ def make_name(cls, fin: 'str | IO[bytes]' = 'in.pcap', fout: 'str' = 'out', ofnm = fout os.makedirs(ofnm, exist_ok=True) elif extension: - ofnm = fout if os.path.splitext(fout)[1] == ext else f'{fout}{ext}' + # NOTE: The dot belongs to the separator here, not to ``ext``. + ofnm = fout if os.path.splitext(fout)[1] == f'.{ext}' else f'{fout}.{ext}' else: ofnm = fout diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index 8d0ea27243..a527776196 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -177,10 +177,12 @@ def pack(self, **kwargs: 'Any') -> 'bytes': return self.__header__.pack(packet) def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_Frame': - """Unpack (parse) packet data. + r"""Unpack (parse) packet data. Args: length: Length of packet data. + \_seek_set (int): File offset before reading, forwarded to + :meth:`self.read ` through ``**kwargs``. **kwargs: Arbitrary keyword arguments. Returns: @@ -198,12 +200,17 @@ def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_Frame self.__header__ = cast('Schema_Frame', self.__schema__.unpack(self._file, length, packet)) # type: ignore[call-arg,misc] return self.read(length, **kwargs) - def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True, **kwargs: 'Any') -> 'Data_Frame': + def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True, + _seek_set: 'int' = 0, **kwargs: 'Any') -> 'Data_Frame': r"""Read each block after global header. Args: length: Length of data to be read. \_read: If the class is called in a parsing scenario. + \_seek_set: File offset of the record's own first octet, i.e. where + the stream stood before :meth:`self.unpack ` consumed + the record; see the note in the parsing branch below for why it + cannot be recovered from the stream once we get here. **kwargs: Arbitrary keyword arguments. Returns: @@ -225,7 +232,18 @@ def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True, **kwargs _irat = _epch.as_integer_ratio() try: - _time = datetime.datetime.fromtimestamp(_irat[0] / _irat[1]) + # NOTE: Anchored to UTC, and aware rather than naive. ``ts_sec`` is an + # offset from the UNIX epoch, so the instant it names does not depend + # on where the file is read; a bare ``fromtimestamp`` renders it in + # the reading host's zone and drops the offset, leaving a datetime + # that reads as a different wall-clock time on every machine and + # cannot be compared against an aware one at all. The ``except`` + # branch below has always returned an aware UTC datetime, and the + # PCAP-NG reader returns one too -- the same + # :attr:`Data_Frame.time ` + # field is filled from there by + # :func:`pcapkit.toolkit.pcapng.block2frame`, so the two have to agree. + _time = datetime.datetime.fromtimestamp(_irat[0] / _irat[1], datetime.timezone.utc) except ValueError: warn(f'PCAP: invalid timestamp: {_epch}', ProtocolWarning, stacklevel=stacklevel()) _time = datetime.datetime.fromtimestamp(0, datetime.timezone.utc) @@ -250,15 +268,27 @@ def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True, **kwargs else: # NOTE: We create a copy of the frame data here for parsing # scenarios to keep the original frame data intact. - seek_cur = self._file.tell() + # + # NOTE: The record spans ``self.length + incl_len`` octets, and by + # the time we get here :meth:`self.unpack ` has consumed + # *all* of them -- the schema's payload field carries the + # ``incl_len`` octets of packet data, not just the 16-octet record + # header. So the frame's own start cannot be reached by seeking + # backwards over ``self.length`` alone: that lands ``incl_len`` + # octets too late and captures the tail of this record followed by + # the head of the next one (see #357). It is recorded before the + # unpack instead, in :meth:`self.__post_init__ <__post_init__>`, and + # seeked to absolutely -- which is what the PCAP-NG reader has + # always done, c.f. :meth:`pcapkit.protocols.misc.pcapng.PCAPNG.read`. + seek_cur = _seek_set + self.length + _ilen # move backward to the beginning of the frame - self._file.seek(-self.length, io.SEEK_CUR) + self._file.seek(_seek_set, io.SEEK_SET) #: bytes: Raw frame data. self._data = self._read_fileng(self.length + _ilen) - # move backward to the beginning of frame's payload + # move forward to the beginning of the next frame self._file.seek(seek_cur, io.SEEK_SET) #: io.BytesIO: Source data stream. @@ -352,9 +382,13 @@ def __post_init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Opt _read = True #: io.BytesIO: Source packet stream. self._file = io.BytesIO(file) if isinstance(file, bytes) else file + # NOTE: Taken *before* the unpack, which advances the stream past the + # whole record; :meth:`self.read ` needs the record's own start and + # cannot work it out afterwards. See the note there. + _seek_set = self._file.tell() #: pcapkit.corekit.infoclass.Info: Parsed packet data. - self._info = self.unpack(length, _read=_read, **kwargs) + self._info = self.unpack(length, _read=_read, _seek_set=_seek_set, **kwargs) def __length_hint__(self) -> 'Literal[16]': """Return an estimated length for the object.""" diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index a7bc33c8d2..61ab0764d3 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -723,12 +723,17 @@ def ts_offset(self) -> 'int': @property def ts_timezone(self) -> 'timezone': - """Timezone of the current block.""" + """Timezone of the current block. + + Defaults to UTC when the capture names no ``if_tzone``, for the reason + given in :meth:`self._get_timezone <_get_timezone>`. + + """ if self._ctx is None: #raise UnsupportedCall(f"'{self.__class__.__name__}' object has no attribute 'ts_timezone'") warn(f"'{self.__class__.__name__}' object has no attribute 'ts_timezone'", AttributeWarning, stacklevel=stacklevel()) - return self._get_local_timezone() + return datetime.timezone.utc info = cast('Packet', self._info) return self._get_timezone(info.interface_id) @@ -1125,7 +1130,23 @@ def _get_payload(self) -> 'bytes': @staticmethod def _get_local_timezone() -> 'timezone': - """Get local timezone.""" + """Get local timezone. + + Warning: + Not used when reconstructing a block timestamp, and must not be: + a PCAP-NG timestamp is an offset from the UNIX epoch, so mixing the + *reading* host's zone into it makes one file parse to different + instants on different machines (see #361). + :meth:`self._get_timezone <_get_timezone>` returns + :attr:`datetime.timezone.utc` instead when the capture names no + ``if_tzone``. + + It also answers for *today* rather than for the capture's own + instant -- ``America/New_York`` reports ``-04:00`` all year, so a + December timestamp would get EDT's offset and not EST's -- which is + a second reason it cannot stand in for a capture's timezone. + + """ tzinfo = datetime.datetime.now(datetime.timezone.utc).astimezone().tzinfo if tzinfo is None: return datetime.timezone.utc @@ -1214,17 +1235,25 @@ def _get_timezone(self, interface_id: 'int' = 0) -> 'timezone': Timezone of the current block. """ + # NOTE: UTC, not the host's timezone, is the default in both branches + # below. A capture carries no timezone of its own unless it says so, and + # draft-ietf-opsawg-pcapng-02 §4.3 defines the block timestamp as an + # offset from 1970-01-01 00:00:00 UTC with no timezone term at all -- + # so substituting whatever zone the *reading* machine happens to sit in + # made the same file parse to different instants on different hosts + # (see #361). ``_get_resolution`` and ``_get_offset`` fall back to the + # format's own defaults in this situation; this is the matching one. if self._ctx is None: # raise UnsupportedCall(f"'{self.__class__.__name__}' object has no attribute '_get_timezone'") warn(f"'{self.__class__.__name__}' object has no attribute '_get_timezone'", AttributeWarning, stacklevel=stacklevel()) - return self._get_local_timezone() + return datetime.timezone.utc options = self._get_interface(interface_id).options tzone = cast('Optional[Data_IF_TZoneOption]', options.get(Enum_OptionType.if_tzone)) if tzone is None: - return self._get_local_timezone() + return datetime.timezone.utc return tzone.timezone def _get_linktype(self, interface_id: 'int' = 0) -> 'Enum_LinkType': @@ -1264,20 +1293,32 @@ def _read_timestamp(self, timestamp_high: 'int', timestamp_low: 'int', *, timestamp_raw = (timestamp_high << 32) | timestamp_low with localcontext(prec=64): + # NOTE: This *is* the UTC epoch the docstring above promises, and it + # is returned as such. It used to have ``tzone.utcoffset(None)`` + # added to it, which is wrong twice over: the block timestamp is an + # offset from the UNIX epoch, so it is timezone-independent by + # definition (draft-ietf-opsawg-pcapng-02 §4.3 spells out the + # arithmetic and has no timezone term), and the same draft §4.2 says + # of ``if_tzone`` that it "SHOULD NOT be used" and of ``if_tsoffset`` + # that it is "not intended to be used as an offset between local time + # and UTC". Adding the offset also made the two return values + # contradict each other, since ``ts_datetime`` below was always built + # from the unshifted value (see #361). timestamp_epoch = decimal.Decimal(timestamp_raw) / self._get_resolution(interface_id) + \ self._get_offset(interface_id) - ts_decimal = timestamp_epoch + decimal.Decimal( - tzone.utcoffset(None).total_seconds()) ts_ratio = timestamp_epoch.as_integer_ratio() try: + # NOTE: ``tzone`` survives only here, as the zone the instant is + # *rendered* in -- which is all a ``tzinfo`` can affect on an aware + # datetime. It never moves the instant itself. ts_datetime = datetime.datetime.fromtimestamp(ts_ratio[0] / ts_ratio[1], tzone) except ValueError: - warn(f'PCAP-NG: [Block {self._type}] invalid timestamp: {ts_decimal}', + warn(f'PCAP-NG: [Block {self._type}] invalid timestamp: {timestamp_epoch}', ProtocolWarning, stacklevel=stacklevel()) ts_datetime = datetime.datetime.fromtimestamp(0, datetime.timezone.utc) - return (ts_datetime, ts_decimal) + return (ts_datetime, timestamp_epoch) @classmethod def _make_data(cls, data: 'Data_PCAPNG') -> 'dict[str, Any]': # type: ignore[override] @@ -1300,12 +1341,22 @@ def _make_timestamp(self, timestamp: 'Optional[float | Decimal | dt_type | int]' """Make timestamp. Args: - timestamp: Timestamp in seconds since UNIX-Epoch. + timestamp: Timestamp in seconds since UNIX-Epoch, in UTC, i.e. as + :meth:`self._read_timestamp <_read_timestamp>` returns it. interface_id: Interface ID that the current block associates with. Returns: - Tuple of timestamp in higher and lower 32-bit integer value - based on the given offset and timezone conversion. + Tuple of timestamp in higher and lower 32-bit integer value, + scaled by the interface's ``if_tsresol`` and shifted by its + ``if_tsoffset``. + + Notes: + No timezone conversion happens here, and none should: the field this + builds is defined as an offset from the UNIX epoch. The docstring + used to promise a "timezone conversion" that the code never + performed, which made it look as though the read side's timezone + shift had a counterpart here (it did not, so a read followed by a + write drifted by the host's UTC offset -- see #361). """ with localcontext(prec=64): diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 3317cd5dba..71142821fd 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -252,24 +252,47 @@ def test_import_test_and_make_name_paths(self) -> None: no_ext_capture = temp / 'capture' no_ext_capture.write_bytes(b'pcap') + # #358: the returned extension is bare, i.e. carries no leading + # dot. ``Extractor.__output__`` spells its extensions *with* the dot + # and ``make_name`` normalises it away, because ``_fext`` reaches the + # engines, which each compose a per-frame name as + # ``f'{name}.{ext._fext}'`` -- leaving the dot on gave + # ``Frame 1..json`` on disk. The output filename below keeps exactly + # one dot, which is the other half of the same contract. self.assertEqual( Extractor.make_name(str(capture), str(temp / 'out'), 'json'), - (str(capture), str(temp / 'out.json'), 'json', '.json', False), + (str(capture), str(temp / 'out.json'), 'json', 'json', False), ) self.assertEqual( Extractor.make_name(str(capture.with_suffix('')), str(temp / 'raw.out'), 'tree', extension=False), - (str(no_ext_capture), str(temp / 'raw.out'), 'tree', '.txt', False), + (str(no_ext_capture), str(temp / 'raw.out'), 'tree', 'txt', False), ) self.assertEqual( Extractor.make_name(str(capture), str(temp / 'frames'), 'json', files=True), - (str(capture), str(temp / 'frames'), 'json', '.json', True), + (str(capture), str(temp / 'frames'), 'json', 'json', True), ) self.assertEqual( Extractor.make_name(str(capture), str(temp / 'ignored'), 'json', nofile=True), (str(capture), None, 'json', None, False), ) + # An extension already on ``fout`` is recognised and not appended + # twice -- the comparison is against ``f'.{ext}'`` now that ``ext`` + # is bare, and getting that wrong would give ``out.json.json``. + self.assertEqual( + Extractor.make_name(str(capture), str(temp / 'kept.json'), 'json')[1], + str(temp / 'kept.json'), + ) + # Every registered format, so a newly registered dumper spelling its + # extension either way cannot reintroduce the doubled dot. + for fmt in ('pcap', 'cap', 'plist', 'xml', 'json', 'tree', 'text', 'txt'): + ext = Extractor.make_name(str(capture), str(temp / f'all-{fmt}'), fmt)[3] + with self.subTest(format=fmt): + self.assertIsNotNone(ext) + self.assertFalse(ext.startswith('.')) + self.assertEqual(f'Frame 1.{ext}'.count('.'), 1) + stream = io.BytesIO(b'capture') stream.name = str(capture) self.assertEqual(Extractor.make_name(stream, str(temp / 'out'), 'json')[0], @@ -473,7 +496,7 @@ def test_constructor_configuration_branches_with_run_patched(self) -> None: with mock.patch.object(Extractor, 'run') as run: with mock.patch.object(Extractor, 'make_name', return_value=(str(capture), str(temp / 'out.json'), - 'json', '.json', False)): + 'json', 'json', False)): defaulted = Extractor() run.assert_called_once() self.assertEqual(defaulted._ifnm, str(capture)) diff --git a/tests/integration/_helpers.py b/tests/integration/_helpers.py index d31499562e..4046daa644 100644 --- a/tests/integration/_helpers.py +++ b/tests/integration/_helpers.py @@ -127,12 +127,15 @@ def section_counts(report: 'dict[str, Any]') -> 'collections.Counter[str]': def report_stems(directory: 'str') -> 'list[str]': """Sorted section names of the reports ``files=True`` wrote into ``directory``. - Split at the *first* dot on purpose. Per-frame reports are named - ``f'{name}.{ext}'`` where ``ext`` already carries its own leading dot - (``pcapkit/foundation/engines/pcap.py:117`` and ``:156``, and - ``pcapkit/foundation/engines/pcapng.py:311``), so they land on disk as - ``Frame 1..json``. Taking the leading component keeps these assertions - neutral about how many dots there are, rather than pinning that defect. + Split at the *first* dot, which keeps the callers' assertions about *which* + sections a report holds independent of what the extension happens to be. + + This used to be neutrality about the number of dots as well: per-frame + reports were named ``f'{name}.{ext._fext}'`` while ``_fext`` still carried + its own leading dot, so they landed on disk as ``Frame 1..json`` (#358). + That is fixed -- the extension is bare now, and + :mod:`tests.integration.test_files_output_naming` asserts the real names -- + so nothing here is working around it any more. """ return sorted(entry.name.split('.', 1)[0] for entry in pathlib.Path(directory).iterdir()) diff --git a/tests/integration/test_files_output_naming.py b/tests/integration/test_files_output_naming.py new file mode 100644 index 0000000000..bf16d0325d --- /dev/null +++ b/tests/integration/test_files_output_naming.py @@ -0,0 +1,114 @@ +# -*- coding: utf-8 -*- +"""What ``files=True`` actually writes to disk. + +#358: every per-frame report landed with two dots before its extension -- +``Frame 1..json``, ``Global Header..txt``, ``Section Header 1..plist``. The name +was composed as ``f'{name}.{ext._fext}'`` while ``_fext`` still carried its own +leading dot, so the separator was supplied twice. + +The unit-tier half of this lives in :file:`tests/foundation/test_extraction.py` +and pins :meth:`Extractor.make_name +` to returning a bare +extension. This module checks the thing the user sees: the names on disk, for +both engines and every format that reaches a file. A test of ``make_name`` alone +would keep passing if an engine started supplying the dot twice again, so the +two are complementary rather than redundant. + +""" +from __future__ import annotations + +import importlib.util +import unittest + +from tests.integration._helpers import EndToEndTestCase +from tests._support import sample_path + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Output formats that ``files=True`` can be driven with, and the extension each +#: is expected to land on. +#: +#: Three registered formats are deliberately absent, each because it raises +#: before any file is named -- so none of them can say anything about how a name +#: is composed, and all three are separate defects from #358: +#: +#: * ``'pcap'`` and ``'cap'`` -- ``PCAPIO.__init__`` wants a ``protocol`` +#: keyword that the extractor never supplies, so they fail with +#: ``TypeError: PCAPIO.__init__() missing 1 required keyword-only argument``. +#: * ``'text'`` -- :attr:`Extractor.__output__ +#: ` maps it to +#: ``dictdumper.Text``, which :mod:`dictdumper` does not export, so it fails +#: with ``AttributeError: module 'dictdumper' has no attribute 'Text'``. +FORMATS = { + 'json': '.json', + 'plist': '.plist', + 'xml': '.plist', + 'tree': '.txt', + 'txt': '.txt', +} + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class FilesOutputNamingTests(EndToEndTestCase): + """Per-frame filenames written under ``files=True``.""" + + def _names(self, capture: 'str', fmt: 'str') -> 'list[str]': + """Sorted names of the files one ``files=True`` extraction wrote.""" + out = self.out(f'{capture.rsplit(".", 1)[0]}-{fmt}') + self.extract(fin=sample_path(capture), fout=out, format=fmt, + files=True, store=False) + import pathlib + return sorted(entry.name for entry in pathlib.Path(out).iterdir()) + + def test_pcap_per_frame_names_carry_exactly_one_dot(self) -> None: + for fmt, suffix in FORMATS.items(): + with self.subTest(format=fmt): + names = self._names('arp.pcap', fmt) + + self.assertEqual(names, [f'Frame 1{suffix}', f'Frame 2{suffix}', + f'Global Header{suffix}']) + for name in names: + self.assertEqual(name.count('.'), 1, name) + self.assertNotIn('..', name) + + def test_pcapng_per_frame_names_carry_exactly_one_dot(self) -> None: + for fmt, suffix in FORMATS.items(): + with self.subTest(format=fmt): + names = self._names('test.pcapng', fmt) + + # every block type the capture holds, not just the frames + self.assertIn(f'Frame 1{suffix}', names) + self.assertIn(f'Section Header 1{suffix}', names) + self.assertIn(f'Interface Description 1{suffix}', names) + for name in names: + self.assertEqual(name.count('.'), 1, name) + self.assertNotIn('..', name) + + def test_single_file_output_still_carries_exactly_one_dot(self) -> None: + """The non-``files`` branch, which was already right and must stay right. + + ``make_name`` appends the extension itself here, so moving the dot out of + ``_fext`` had to move it into this branch's f-string; a mistake would + show up as ``report`` with no suffix, or ``report..json``. + + """ + for fmt, suffix in FORMATS.items(): + with self.subTest(format=fmt): + extractor = self.extract(fin=sample_path('arp.pcap'), + fout=self.out(f'report-{fmt}'), + format=fmt, store=False) + + self.assertEqual(extractor.output, self.out(f'report-{fmt}{suffix}')) + self.assertEqual(extractor.output.count('.'), 1) + + def test_output_name_already_carrying_the_suffix_is_left_alone(self) -> None: + extractor = self.extract(fin=sample_path('arp.pcap'), + fout=self.out('kept.json'), format='json', + store=False) + + self.assertEqual(extractor.output, self.out('kept.json')) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/misc/pcap/test_frame_runtime.py b/tests/protocols/misc/pcap/test_frame_runtime.py index c0b9e73303..7d75fcf60a 100644 --- a/tests/protocols/misc/pcap/test_frame_runtime.py +++ b/tests/protocols/misc/pcap/test_frame_runtime.py @@ -1,6 +1,7 @@ from __future__ import annotations import importlib.util +import struct import unittest from tests._support import purge_modules, sample_path @@ -50,6 +51,46 @@ def test_frame_info_records_protocol_summary(self) -> None: self.assertEqual(frame.payload.payload.name, 'Address Resolution Protocol') self.assertEqual(frame.payload.payload.payload.name, 'Unknown') + def test_extracted_frames_carry_their_own_record_bytes(self) -> None: + """#357, through the whole extractor rather than one ``Frame``. + + ``arp.pcap`` holds two records of identical length, which is the shape + that made the defect legible: frame 1's payload was frame 2's record + header onwards, and frame 2 -- having nothing left to read -- came back + as 16 octets instead of 76. The unit-tier counterpart in + :file:`tests/protocols/misc/pcap/test_header_frame_unit.py` drives + ``Frame`` directly; this one goes through + :func:`pcapkit.interface.extract`, which is the path every caller + actually uses. + + """ + from pcapkit.interface import extract + + path = sample_path('arp.pcap') + with open(path, 'rb') as stream: + raw = stream.read() + + records = [] + offset = 24 + while offset < len(raw): + incl_len, = struct.unpack_from(' None: + """A frame's raw data must be the record it parsed, at its own offset. + + #357: :meth:`Frame.read ` + rewound by ``self.length`` (16) to find the start of the record, but the + schema unpack has by then consumed the record header *and* the + ``incl_len`` octets of packet data. So the rewind landed ``incl_len`` + octets too late: every frame captured its own tail followed by the head + of the next record, and the final frame came back truncated because the + read ran past EOF. + + Checked against the record chain walked straight out of the file, so the + assertion does not depend on any of the code under test, and checked for + *every* record rather than the first -- the last one is the truncating + case. + + """ + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.pcap.header import Header + + path = sample_path('in.pcap') + with open(path, 'rb') as stream: + raw = stream.read() + + # in.pcap is little-endian (magic d4c3b2a1) with a 24-octet global header + self.assertEqual(raw[:4], b'\xd4\xc3\xb2\xa1') + records = [] + offset = 24 + while offset < len(raw): + incl_len, = struct.unpack_from(' None: + """``Frame.info.time`` names an instant, so it must not be host-local. + + #361 is about the PCAP-NG epoch, but the same family of defect sat + here: ``ts_sec`` is an offset from the UNIX epoch, and rendering it with + a bare :meth:`datetime.datetime.fromtimestamp` produced a *naive* + datetime in whatever zone the reading machine sat in. The instant was + right; the object could not be compared with an aware one, and read back + as a different wall-clock time on every host. + + """ + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.pcap.header import Header + + path = sample_path('in.pcap') + with open(path, 'rb') as stream: + raw = stream.read() + ts_sec, ts_usec = struct.unpack_from(' None: self.assertEqual(pcapng.linktype, LinkType.ETHERNET) self.assertEqual(pcapng._make_timestamp(decimal.Decimal(12)), (0, 2000)) + # #361: the epoch is 2000 units at if_tsresol=1000, i.e. 2s, plus the + # 10s if_tsoffset -- 12s, and nothing else. It used to come back as 3612, + # the same instant with this interface's if_tzone (+01:00) *added* to it, + # which is 3600s of pure error: a PCAP-NG timestamp is an offset from the + # UNIX epoch and so carries no timezone. The datetime is the one place + # ``tz`` still shows, as the zone the instant is rendered in, and it + # names the very same instant -- which the two returns did not before. ts_datetime, ts_decimal = pcapng._read_timestamp(0, 2000) self.assertEqual(ts_datetime, datetime.datetime.fromtimestamp(12, tz)) - self.assertEqual(ts_decimal, decimal.Decimal(3612)) + self.assertEqual(ts_decimal, decimal.Decimal(12)) + self.assertEqual(ts_datetime.timestamp(), float(ts_decimal)) + # and the round trip closes, which it could not while the two disagreed + self.assertEqual(pcapng._make_timestamp(ts_decimal), (0, 2000)) self.assertEqual(pcapng._read_mac_addr(b'\x00\x01\x02\x03\x04\x05'), '00:01:02:03:04:05') self.assertEqual(pcapng._read_eui_addr(bytes.fromhex('023456fffe789abc')), @@ -282,7 +293,9 @@ def test_pcapng_top_level_properties_registry_and_dispatch_paths(self) -> None: with mock.patch('pcapkit.protocols.misc.pcapng.warn') as warn: self.assertEqual(pcapng.ts_resolution, 1_000_000) self.assertEqual(pcapng.ts_offset, 0) - self.assertIsInstance(pcapng.ts_timezone, datetime.timezone) + # #361: UTC, not the reading host's zone -- the format's own + # default, to match the two above + self.assertEqual(pcapng.ts_timezone, datetime.timezone.utc) self.assertEqual(warn.call_count, 3) pcapng._info = DummyData(type=BlockType.Simple_Packet_Block, length=20, interface_id=0) @@ -320,7 +333,10 @@ def test_pcapng_top_level_properties_registry_and_dispatch_paths(self) -> None: pcapng._ctx = empty_ctx self.assertEqual(pcapng._get_resolution(0), 1_000_000) self.assertEqual(pcapng._get_offset(0), 0) - self.assertIsInstance(pcapng._get_timezone(0), datetime.timezone) + # #361: an interface that names no if_tzone gets UTC, not the reading + # host's zone -- which is what made one file parse to different instants + # on different machines + self.assertEqual(pcapng._get_timezone(0), datetime.timezone.utc) pcapng._ctx = None with self.assertRaises(UnsupportedCall): pcapng._get_linktype(0) @@ -354,7 +370,11 @@ def fromtimestamp(cls, *args): mock.patch('pcapkit.protocols.misc.pcapng.warn') as warn: timestamp, epoch = pcapng._read_timestamp(0, 1, interface_id=0) self.assertEqual(timestamp, real_datetime.fromtimestamp(0, datetime.timezone.utc)) - self.assertEqual(epoch, decimal.Decimal(7200) + decimal.Decimal(2) + decimal.Decimal('0.000000001')) + # #361: 1 unit at if_tsresol=1e9 plus the 2s if_tsoffset. The leading + # ``Decimal(7200)`` this used to carry was this interface's if_tzone + # (+02:00) leaking into the epoch; the fallback datetime above is aware + # UTC either way, so the two used to name different instants. + self.assertEqual(epoch, decimal.Decimal(2) + decimal.Decimal('0.000000001')) warn.assert_called_once() with mock.patch('pcapkit.protocols.misc.pcapng.time.time_ns', return_value=3_500_000_000): @@ -2933,6 +2953,129 @@ def test_pcapng_big_endian_sample_section_options_are_intact(self) -> None: [9, 12, 15, 7, 0]) self.assertEqual(section.options[OptionType.opt_comment].comment, 'test001') + def _under_timezone(self, zone: 'str') -> 'datetime.timedelta': + """Install ``zone`` as the process timezone and return its UTC offset. + + Restored on teardown. :mod:`pcapkit` reads the host zone at parse time + rather than at import time, so this takes effect without reimporting it. + + """ + previous = os.environ.get('TZ') + + def restore() -> 'None': + if previous is None: + os.environ.pop('TZ', None) + else: + os.environ['TZ'] = previous + time.tzset() + + self.addCleanup(restore) + os.environ['TZ'] = zone + time.tzset() + offset = datetime.datetime.now().astimezone().utcoffset() + assert offset is not None + return offset + + def test_read_timestamp_is_utc_whatever_the_host_timezone_is(self) -> None: + """A block timestamp names one instant, on every machine that reads it. + + #361: ``_read_timestamp`` added ``tzone.utcoffset(None)`` to the epoch + it returned, and ``_get_timezone`` fell back to the *reading host's* zone + whenever the capture named no ``if_tzone`` -- which + draft-ietf-opsawg-pcapng-02 §4.2 says should be the normal case, since + the option "SHOULD NOT be used". So the same file parsed to a different + absolute time on every host, by that host's UTC offset. + + The defect is invisible on a UTC machine, which is why it survived, so + this drives several zones explicitly and asserts they were really + installed -- a run that silently stayed on UTC would prove nothing and + is skipped rather than passed. + + """ + from pcapkit.protocols.misc.pcapng import PCAPNG + + if not hasattr(time, 'tzset'): # pragma: no cover + self.skipTest('time.tzset() is unavailable on this platform') + + # A synthetic interface naming no if_tzone: 2_000_000 units at the + # default if_tsresol of 1e6, i.e. 2s since the UNIX epoch, full stop. + interface = types.SimpleNamespace(linktype=0, snaplen=65535, options={}) + pcapng = object.__new__(PCAPNG) + pcapng._ctx = types.SimpleNamespace( + interfaces=[interface], + section=types.SimpleNamespace(byteorder='little'), + ) + pcapng._type = 6 # Enhanced Packet Block + + offsets = set() + for zone in ('UTC', 'Asia/Shanghai', 'America/New_York', 'Asia/Kolkata'): + with self.subTest(TZ=zone): + offsets.add(self._under_timezone(zone)) + + ts_datetime, ts_epoch = pcapng._read_timestamp(0, 2_000_000) + + self.assertEqual(ts_epoch, decimal.Decimal(2)) + self.assertEqual(ts_datetime, + datetime.datetime.fromtimestamp(2, datetime.timezone.utc)) + self.assertEqual(ts_datetime.utcoffset(), datetime.timedelta(0)) + # the two returns must name the same instant + self.assertEqual(ts_datetime.timestamp(), float(ts_epoch)) + # and a read followed by a write must not drift + self.assertEqual(pcapng._make_timestamp(ts_epoch), (0, 2_000_000)) + + # Guard against the whole test having run on UTC four times over, which + # is exactly the condition under which the defect was undetectable. + if len(offsets) < 2: # pragma: no cover + self.skipTest(f'the host resolved every zone to the same offset {offsets}; ' + 'no tzdata installed, so this test proves nothing') + self.assertTrue(any(offset for offset in offsets), + f'expected at least one non-zero UTC offset, got {offsets}') + + def test_sample_capture_timestamp_matches_its_own_raw_bytes(self) -> None: + """The same property on a real capture, against the bytes in the file. + + ``dhcp.pcapng`` is committed, and its interface carries ``if_tsresol=6`` + with neither ``if_tsoffset`` nor ``if_tzone`` -- the shape #361 is + about, where the epoch used to be shifted by whatever zone the reading + machine sat in. + + """ + from pcapkit.interface import extract + + if not hasattr(time, 'tzset'): # pragma: no cover + self.skipTest('time.tzset() is unavailable on this platform') + + path = sample_path('dhcp.pcapng') + with open(path, 'rb') as stream: + raw = stream.read() + + # independent ground truth, straight out of the block chain + self.assertEqual(raw[8:12], b'\x4d\x3c\x2b\x1a') # little-endian section + shb_len, = struct.unpack_from('