fix(pcapng): keep the captured octets every packet block declares (#646)

pcapkit/protocols/misc/pcapng.py

@@ -105,6 +105,7 @@
 from pcapkit.protocols.data.misc.pcapng import WireGuardKeyLog as Data_WireGuardKeyLog
 from pcapkit.protocols.data.misc.pcapng import ZigBeeAPSKey as Data_ZigBeeAPSKey
 from pcapkit.protocols.data.misc.pcapng import ZigBeeNWKKey as Data_ZigBeeNWKKey
+from pcapkit.protocols.data.protocol import Packet as Data_Packet
 from pcapkit.protocols.protocol import ProtocolBase as Protocol
 from pcapkit.protocols.schema.misc.pcapng import PCAPNG as Schema_PCAPNG
 from pcapkit.protocols.schema.misc.pcapng import BlockType as Schema_BlockType
@@ -677,9 +678,99 @@ def name(self) -> 'str':
 
     @property
     def length(self) -> 'int':
-        """Header length of corresponding protocol."""
+        """Block total length of corresponding protocol.
+
+        Note:
+            This is the wire's *Block Total Length* -- the whole block, trailing
+            length field included -- and not a header length. The two are the
+            same thing for a protocol whose payload runs to the end of its
+            buffer, which is why
+            :attr:`ProtocolBase.length <pcapkit.protocols.protocol.ProtocolBase.length>`
+            does not distinguish them, but a PCAP-NG block carries a trailer.
+            :attr:`self.packet <packet>` is overridden accordingly; see there and
+            #646.
+
+        """
         return self._info.length
 
+    # NOTE: A plain property, where the inherited one is a
+    # :func:`~pcapkit.utilities.compat.cached_property`. That is deliberate and it
+    # is not an oversight of the base class's caching: the inherited one caches
+    # because it *reads the stream*, and a second read would consume octets that
+    # are no longer there, whereas this one only walks buffers the schema layer has
+    # already filled and so costs a handful of dict lookups.
+    #
+    # Caching it would reintroduce, by a different route, the staleness this change
+    # exists to remove. :meth:`self.unpack <unpack>` now reports the payload
+    # through this property, so a cache would make a second ``unpack`` on the same
+    # instance return the *first* call's octets -- ``get_payload`` never even
+    # reached -- where the code before #646 recomputed from the schema every time.
+    # Nothing in the tree calls ``unpack`` twice on one instance today
+    # (``__post_init__`` is its only caller), so this is an invariant being kept
+    # rather than a bug being fixed; it was held before and there is no reason for
+    # it to stop holding. A data descriptor also wins over ``__dict__``, so a stale
+    # entry left by the inherited ``cached_property`` cannot shadow this either.
+    @property
+    def packet(self) -> 'Data_Packet':
+        """Header and payload octets of the current block.
+
+        A PCAP-NG block is not a header followed by a payload. The captured
+        octets sit in the *middle* of a packet block, ahead of the option list
+        and the trailing Block Total Length, so no single split point expresses
+        the shape and the inherited
+        :attr:`ProtocolBase.packet <pcapkit.protocols.protocol.ProtocolBase.packet>`
+        -- which reads :attr:`self.length <length>` octets of header and takes
+        everything after as payload -- cannot produce it. Since
+        :attr:`self.length <length>` is the Block Total Length, that split
+        consumed the entire block as header and left the payload empty: every
+        packet block reported ``packet == b''`` while ``captured_len`` declared
+        hundreds of octets, and dumping such a block through
+        :class:`~pcapkit.dumpkit.pcap.PCAPIO` wrote a record header promising
+        octets it then did not write. See #646.
+
+        The payload is therefore the block schema's
+        :attr:`~pcapkit.protocols.schema.schema.Schema.__payload__` field, which
+        is where the schema layer put the captured octets, and the header is the
+        block octets ahead of it. The three block types that carry captured
+        octets -- :attr:`PACKET_TYPES <PCAPNG.PACKET_TYPES>`, i.e. the Enhanced
+        Packet Block, the Simple Packet Block and the obsolete Packet Block --
+        each place the payload at a different offset, so the offset is summed
+        from the octets the schema actually unpacked rather than hard-coded per
+        block type. A block declaring no payload field is all header and no
+        payload, as before.
+
+        """
+        block = self.__header__.block
+        payload_name = self._get_payload_name(block)
+        if payload_name is None:
+            return Data_Packet(header=self._data, payload=b'')
+
+        # The octets ahead of the payload field, in wire order: the outer
+        # schema's fields up to the one holding the block -- its 4-octet Block
+        # Type -- and then the block's own fields up to the payload. The block is
+        # found by identity rather than by name so that renaming the field
+        # cannot silently fold the whole block body into the header; if it is not
+        # found at all, the header loses the Block Type and nothing else, leaving
+        # the payload -- the part #646 is about -- exact either way.
+        names = list(self.__header__.__fields__)
+        stop = next((idx for idx, name in enumerate(names)
+                     if getattr(self.__header__, name, None) is block), 0)
+
+        block_names = list(block.__fields__)
+        # Safe: ``_get_payload_name`` returned a name only because it is a field.
+        block_stop = block_names.index(payload_name)
+
+        header = bytearray()
+        for name in names[:stop]:
+            header += self.__header__.__buffer__.get(name, b'')
+        for name in block_names[:block_stop]:
+            header += block.__buffer__.get(name, b'')
+
+        return Data_Packet(
+            header=bytes(header),
+            payload=block.get_payload(payload_name),
+        )
+
     @property
     def context(self) -> 'Context':
         """Context of current PCAP-NG block."""
@@ -900,13 +991,14 @@ def unpack(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_PCAPN
             self.__header__ = cast('Schema_PCAPNG', self.__schema__.unpack(self._file, length, packet))  # type: ignore[call-arg,misc]
 
         data = self.read(length, **kwargs)
-        block_schema = self.__header__.block
-        payload_name = getattr(block_schema, '__payload__', None)
-        if payload_name in getattr(block_schema, '__fields__', {}):
-            packet = block_schema.get_payload()
-        else:
-            packet = b''
-        data.__update__(packet=packet)
+
+        # NOTE: One source of truth for the captured octets. This used to extract
+        # the payload here as well, and ``ProtocolBase.__init__`` then overwrote
+        # the result with ``self.packet.payload`` -- which was empty, because the
+        # inherited ``packet`` split the block at its Block Total Length. Reading
+        # it through the property instead means the value injected there is the
+        # value computed here, rather than a second attempt at it. See #646.
+        data.__update__(packet=self.packet.payload)
         return data
 
     def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True,
@@ -1128,6 +1220,26 @@ def _get_payload(self) -> 'bytes':
         """
         return self.__header__.block.get_payload()
 
+    @staticmethod
+    def _get_payload_name(block: 'Schema_BlockType | bytes') -> 'Optional[str]':
+        """Get the name of the field carrying a block's captured packet octets.
+
+        Args:
+            block: Parsed block schema.
+
+        Returns:
+            The name of the block schema's payload field, or :obj:`None` when the
+            block declares none -- which is every block type outside
+            :attr:`PACKET_TYPES <PCAPNG.PACKET_TYPES>`, and also a block whose
+            body was handed over as raw :obj:`bytes` at construction time rather
+            than as a schema.
+
+        """
+        name = getattr(block, '__payload__', None)
+        if name is None or name not in getattr(block, '__fields__', {}):
+            return None
+        return name
+
     @staticmethod
     def _get_local_timezone() -> 'timezone':
         """Get local timezone.

pcapkit/protocols/protocol.py

@@ -470,7 +470,27 @@ def protochain(self) -> 'ProtoChain':
     # packet data
     @cached_property
     def packet(self) -> 'Data_Packet':
-        """Data_Packet data of the protocol."""
+        """Data_Packet data of the protocol.
+
+        Note:
+            The split relies on :attr:`self.length <length>` being the length of
+            the octets *preceding* the payload, and on the payload running from
+            there to the end of the buffer. Both hold for a protocol laid out as
+            a header followed by its payload, which is nearly all of them.
+
+            A protocol that is not laid out that way has to override this: one
+            whose :attr:`~length` counts something else, or one carrying a
+            *trailer* after the payload, gets a header that eats the payload and
+            a payload of ``b''``. That is what
+            :class:`~pcapkit.protocols.misc.pcapng.PCAPNG` did to every packet
+            block -- its :attr:`~pcapkit.protocols.misc.pcapng.PCAPNG.length` is
+            the wire's Block Total Length and the captured octets sit ahead of
+            the option list and the trailing length field -- and
+            :meth:`ProtocolBase.__init__` injects this payload into every parsed
+            ``_info``, so the empty value reached the dumpers and corrupted the
+            files they wrote. See #646.
+
+        """
         try:
             return self._read_packet(header=self.length)
         except UnsupportedCall:

tests/protocols/misc/test_pcapng_unit.py

@@ -1878,51 +1878,85 @@ def test_pcapng_remaining_constructor_branches_and_custom_dispatch(self) -> None
                              __packet__={'byteorder': 'little'})
         self.assertEqual(len(packed), 20)
 
+        # NOTE: ``unpack`` injects the captured octets by reading ``self.packet``
+        # rather than by extracting them a second time of its own (#646), so these
+        # stubs have to present the schema surface that property reads: the outer
+        # schema's ``__fields__``/``__buffer__`` up to the field holding the block,
+        # and the block's own up to its payload field.
         unpacker = object.__new__(PCAPNG)
         unpacker.__header__ = None
         payload_block = types.SimpleNamespace(
             __payload__='packet_data',
             __fields__={'packet_data': object()},
+            __buffer__={'packet_data': b'payload'},
             get_payload=mock.Mock(return_value=b'payload'),
         )
         unpacker.__schema__ = types.SimpleNamespace(
-            unpack=mock.Mock(return_value=types.SimpleNamespace(block=payload_block)))
+            unpack=mock.Mock(return_value=types.SimpleNamespace(
+                block=payload_block,
+                __fields__={'type': object(), 'block': object()},
+                __buffer__={'type': b'\x03\x00\x00\x00'},
+            )))
         unpacker._file = io.BytesIO(b'0123456789ab')
         unpacker._ctx = types.SimpleNamespace(section=types.SimpleNamespace(byteorder='little'))
         unpacker._byte = 'big'
         unpacker.read = mock.Mock(return_value=DummyData(length=12))
         data = unpacker.unpack(12, __packet__={})
         self.assertEqual(data['packet'], b'payload')
         self.assertEqual(unpacker._byte, 'little')
-        payload_block.get_payload.assert_called_once_with()
-
+        payload_block.get_payload.assert_called_once_with('packet_data')
+        # Only the outer fields ahead of the block reach the header: the 4-octet
+        # block type, and none of the block's own, the payload being its first.
+        self.assertEqual(unpacker.packet.header, b'\x03\x00\x00\x00')
+
+        # The same instance, unpacked a second time with a different block in place:
+        # an already-set ``__header__`` must skip re-unpacking the schema, and the
+        # payload must be recomputed from the block that is there now rather than
+        # served from the first call. That second half is why ``PCAPNG.packet`` is a
+        # plain property and not a ``cached_property`` -- caching it would return
+        # ``b'payload'`` here and never call ``get_payload`` at all. See #646.
         cached_block = types.SimpleNamespace(
             __payload__='packet_data',
             __fields__={'packet_data': object()},
+            __buffer__={'packet_data': b'cached'},
             get_payload=mock.Mock(return_value=b'cached'),
         )
-        unpacker.__header__ = types.SimpleNamespace(block=cached_block)
+        unpacker.__header__ = types.SimpleNamespace(
+            block=cached_block,
+            __fields__={'type': object(), 'block': object()},
+            __buffer__={'type': b'\x03\x00\x00\x00'},
+        )
         unpacker.read = mock.Mock(return_value=DummyData(length=12))
         self.assertEqual(unpacker.unpack(12)['packet'], b'cached')
         unpacker.__schema__.unpack.assert_called_once()
-        cached_block.get_payload.assert_called_once_with()
+        cached_block.get_payload.assert_called_once_with('packet_data')
 
         no_ctx_unpacker = object.__new__(PCAPNG)
         no_ctx_unpacker.__header__ = None
         no_payload_block = types.SimpleNamespace(
             __payload__='payload',
             __fields__={},
+            __buffer__={},
             get_payload=mock.Mock(return_value=b'unused'),
         )
         no_ctx_unpacker.__schema__ = types.SimpleNamespace(
-            unpack=mock.Mock(return_value=types.SimpleNamespace(block=no_payload_block)))
+            unpack=mock.Mock(return_value=types.SimpleNamespace(
+                block=no_payload_block,
+                __fields__={'type': object(), 'block': object()},
+                __buffer__={'type': b'\x01\x00\x00\x00'},
+            )))
         no_ctx_unpacker._file = io.BytesIO(b'0123456789ab')
+        no_ctx_unpacker._data = b'0123456789ab'
         no_ctx_unpacker._ctx = None
         no_ctx_unpacker._byte = 'big'
         no_ctx_unpacker.read = mock.Mock(return_value=DummyData(length=12))
         self.assertEqual(no_ctx_unpacker.unpack(12)['packet'], b'')
         self.assertEqual(no_ctx_unpacker._byte, 'big')
         no_payload_block.get_payload.assert_not_called()
+        # A block declaring no payload field is all header: the raw block octets
+        # verbatim, and no payload.
+        self.assertEqual(no_ctx_unpacker.packet.header, b'0123456789ab')
+        self.assertEqual(no_ctx_unpacker.packet.payload, b'')
 
         self.assertEqual(pcapng._make_block_shb(major_version=2, section_length=-1).minor, 0)
         self.assertEqual(pcapng._make_block_shb(major_version=1, minor_version=5,