pcap/pcapng: fix frame bytes, output filenames, and timestamp timezones

conda/build

@@ -1 +1 @@
-0
+1

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,18 +593,29 @@ 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)
 
             if files:
                 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
 

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 <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 <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 <pcapkit.protocols.data.misc.pcap.frame.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 <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 <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."""

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):

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))