fix(engines): the pyshark engine skips non-packet PCAP-NG blocks (#1515)

pcapkit/foundation/engines/pyshark.py

@@ -25,6 +25,7 @@
     from typing import Optional
 
     from pyshark.capture.file_capture import FileCapture
+    from pyshark.packet.layers.base import BaseLayer
     from pyshark.packet.packet import Packet as PySharkPacket
 
     from pcapkit.foundation.extraction import Extractor
@@ -34,6 +35,45 @@
 logger = get_logger(__name__)
 
 
+def _frame_layer(packet: 'PySharkPacket') -> 'Optional[BaseLayer]':
+    """The ``frame`` layer of a `PyShark`_ packet, or :data:`None` when the record has none.
+
+    Looked up by name, because `PyShark`_ takes the second PDML ``<proto>`` as
+    ``frame_info``. On a packet with a comment, that is ``pkt_comment``, which has
+    no ``protocols``, and the frame layer is ``layers[0]``. By its source,
+    :program:`tshark` 4.2.2 roots a Sysdig event at ``syscall`` and a Netflix
+    custom block at ``bblog``, so those records have no ``frame`` layer.
+
+    .. _PyShark: https://kiminewt.github.io/pyshark
+
+    """
+    for layer in (packet.frame_info, *packet.layers):
+        if layer.layer_name == 'frame':
+            return layer
+    return None
+
+
+def _is_packet(packet: 'PySharkPacket') -> 'bool':
+    """Whether a `PyShark`_ packet is a packet record, rather than another kind of tshark record.
+
+    :program:`tshark` numbers every record it reads, not only packets: a PCAP-NG
+    systemd Journal Export Block, Custom Block or Sysdig Event Block is a frame
+    to it, measured with :program:`tshark` 4.6.9. The default engine counts
+    Enhanced, Simple and obsolete Packet Blocks only.
+
+    The marker is ``frame.encap_type`` on the record's :func:`_frame_layer`.
+    Wireshark adds it for ``REC_TYPE_PACKET`` records only:
+    :file:`epan/dissectors/packet-frame.c` lines 1001-1002 in 4.2.2, and 828-890
+    in 4.6.9. The summary line is not used, because it differs between those
+    versions. A record with no ``frame`` layer is not a packet either.
+
+    .. _PyShark: https://kiminewt.github.io/pyshark
+
+    """
+    frame = _frame_layer(packet)
+    return frame is not None and 'encap_type' in frame.field_names
+
+
 class PyShark(EngineBase['PySharkPacket']):
     """PyShark engine support.
 
@@ -214,10 +254,12 @@ def run(self) -> 'None':
                  f"so 'ipv4={ext._ipv4}', 'ipv6={ext._ipv6}' and 'tcp={ext._tcp}' will be ignored",
                  AttributeWarning, stacklevel=stacklevel())
 
-        # setup verbose handler
+        # setup verbose handler; a packet always has a frame layer, since
+        # read_frame skips every record that does not (#1515)
         if ext._flag_v:
             ext._vfunc = lambda e, f: print(
-                f'Frame {e._frnum:>3d}: {f.frame_info.protocols}'  # pylint: disable=protected-access
+                f'Frame {e._frnum:>3d}: '  # pylint: disable=protected-access
+                f'{cast("BaseLayer", _frame_layer(f)).protocols}'
             )
 
         # extract global header
@@ -230,6 +272,10 @@ def run(self) -> 'None':
     def read_frame(self) -> 'PySharkPacket':
         """Read frames with PyShark engine.
 
+        A tshark record that is not a packet, such as a PCAP-NG systemd Journal
+        Export Block, is skipped. Frames are numbered by count, as the default
+        engine numbers them, not by tshark's frame number.
+
         Returns:
             Parsed frame instance.
 
@@ -241,11 +287,15 @@ def read_frame(self) -> 'PySharkPacket':
         from pcapkit.toolkit.pyshark import packet2dict, tcp_traceflow
         ext = self._extractor
 
-        # fetch PyShark packet
-        packet = cast('PySharkPacket', self._extmp.next())
+        # fetch the next PyShark packet that is a packet record
+        while True:
+            packet = cast('PySharkPacket', self._extmp.next())
+            if _is_packet(packet):
+                break
+            logger.debug('pyshark: skipping tshark frame %s, not a packet record', packet.number)
 
         # verbose output
-        ext._frnum = int(packet.number)
+        ext._frnum += 1
         ext._vfunc(ext, packet)
 
         # write plist

tests/foundation/engines/test_engine_agreement_runtime.py

@@ -349,11 +349,6 @@ class TestEngineAgreement(harness.RoundTripBase):
             tuple(f'{capture}/pypcapfile/{aspect}'
                   for capture in ('http6.cap', 'stream.pcap', 'tcp.pcap', 'test.pcap')
                   for aspect in ('tcp', 'trace', 'datagrams', 'flows'))),
-        Gap(1515, 'the pyshark engine counts a Systemd Journal Export Block as a frame: tshark '
-               'numbers it as one, and read_frame takes every tshark frame '
-               '(pcapkit/foundation/engines/pyshark.py:245-248)',
-            ('FRAMES', 'RECORDS'), ('6 frames != 5', 'frame 5: timestamp '),
-            ('test.pcapng/pyshark/frames', 'test.pcapng/pyshark/timestamps')),
         Gap(1501, 'the dpkt engine hands reassembly the Decimal timestamp dpkt reads from a '
                'nanosecond PCAP (pcapkit/foundation/engines/dpkt.py:309), where every other '
                'engine hands it a float (pcapkit/toolkit/pcap.py:73)',

tests/foundation/engines/test_pyshark_non_packet_unit.py

@@ -0,0 +1,141 @@
+# -*- coding: utf-8 -*-
+"""The PyShark engine reads packet records only, as the default engine does. C.f. #1515.
+
+:program:`tshark` numbers every record wiretap hands it, and a PCAP-NG systemd
+Journal Export Block, Custom Block or Sysdig Event Block is one. The default
+engine counts Enhanced, Simple and obsolete Packet Blocks only, so a capture
+holding a journal block read as one frame more through ``pyshark``, and every
+later frame was off by one.
+
+The same lookup serves the verbose handler. ``pyshark`` takes the second PDML
+``<proto>`` as ``frame_info``, and on a packet that carries a comment that is
+``pkt_comment``, which has no ``protocols``. So ``verbose=True`` raised
+:exc:`AttributeError` on frame 1 of :file:`examples/captures/test.pcapng`.
+
+The packets here are stand-ins shaped like what ``pyshark`` 0.6 builds from
+:program:`tshark` 4.6.9's PDML. The field names are the ones measured on
+:file:`examples/captures/test.pcapng` and on a capture holding a Custom Block and
+a Sysdig Event Block. ``pyshark`` itself is never imported, so this runs where it
+is absent or cannot run.
+
+"""
+
+import contextlib
+import importlib.util
+import io
+import types
+import unittest
+
+from tests._support import reimport_once_per_class
+
+RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper')
+HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS)
+
+#: ``frame`` layer fields of a packet record; only these carry ``encap_type``.
+PACKET_FRAME = ('section_number', 'interface_id', 'interface_name', 'encap_type', 'time',
+                'time_epoch', 'number', 'len', 'cap_len', 'protocols')
+#: ``frame`` layer fields of a systemd Journal Export or Sysdig Event record.
+RECORD_FRAME = ('section_number', 'time', 'time_utc', 'time_epoch', 'number', 'len', 'cap_len',
+                'protocols')
+#: ``frame.protocols`` of frame 1 of :file:`examples/captures/test.pcapng`.
+PROTOCOLS = 'eth:ethertype:ip:udp:dhcp'
+
+
+def _layer(name: str, fields: 'tuple[str, ...]' = (), **values: str) -> types.SimpleNamespace:
+    return types.SimpleNamespace(layer_name=name, field_names=list(fields), **values)
+
+
+def _packet(number: int, comment: bool = False) -> types.SimpleNamespace:
+    """A packet record; with a comment, ``pyshark`` takes ``pkt_comment`` as ``frame_info``."""
+    frame = _layer('frame', PACKET_FRAME, protocols=PROTOCOLS)
+    eth = _layer('eth', ('dst', 'src', 'type'))
+    if comment:
+        note = _layer('pkt_comment', ('frame_comment',))
+        return types.SimpleNamespace(number=str(number), frame_info=note, layers=[frame, eth])
+    return types.SimpleNamespace(number=str(number), frame_info=frame, layers=[eth])
+
+
+def _record(number: int, frame: 'tuple[str, ...] | None', root: str) -> types.SimpleNamespace:
+    """A non-packet record; ``frame=None`` for a root that is not ``frame`` at all."""
+    head = _layer('frame', frame) if frame is not None else _layer(root, ('num',))
+    layers = [_layer(root, ('message',))] if frame is not None else []
+    return types.SimpleNamespace(number=str(number), frame_info=head, layers=layers)
+
+
+#: Every non-packet record shape, and where it was seen.
+RECORDS = {
+    'systemd journal (4.6.9)': ('systemd_journal', RECORD_FRAME),
+    'custom block (4.6.9)': ('DATA', ('section_number', 'interface_id', 'time', 'number', 'len')),
+    'sysdig event (4.6.9)': ('sysdig', RECORD_FRAME),
+    # 4.2.2 roots a Sysdig event at ``syscall`` and a Netflix custom block at
+    # ``bblog`` instead of ``frame`` (epan/dissectors/packet-frame.c:858-869, :884-890).
+    'sysdig event (4.2.2)': ('syscall', None),
+    'black box log (4.2.2)': ('bblog', None),
+}
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+class TestPySharkNonPacketRecords(unittest.TestCase):
+    """:meth:`PyShark.read_frame` skips every record that is not a packet."""
+
+    def setUp(self) -> None:
+        reimport_once_per_class(self)
+
+    @staticmethod
+    def _engine(records: 'list[types.SimpleNamespace]') -> 'tuple[object, types.SimpleNamespace]':
+        from pcapkit.foundation.engines.pyshark import PyShark
+
+        stream = iter(records)
+        extractor = types.SimpleNamespace(_frnum=0, _vfunc=lambda e, f: None, _flag_q=True,
+                                          _flag_t=False, _flag_d=True, _frame=[])
+        engine = PyShark.__new__(PyShark)  # skip __init__, which imports pyshark
+        engine._extractor = extractor
+        engine._extmp = types.SimpleNamespace(next=lambda: next(stream))
+        return engine, extractor
+
+    def test_a_journal_record_is_not_a_frame(self) -> None:
+        first, last = _packet(1, comment=True), _packet(3)
+        engine, ext = self._engine([first, _record(2, RECORD_FRAME, 'systemd_journal'), last])
+
+        self.assertIs(engine.read_frame(), first)
+        self.assertEqual(ext._frnum, 1)
+        self.assertIs(engine.read_frame(), last)
+        self.assertEqual(ext._frnum, 2, 'frames are numbered as the default engine numbers them')
+        self.assertEqual(ext._frame, [first, last])
+        with self.assertRaises(StopIteration):
+            engine.read_frame()
+
+    def test_every_record_without_an_encapsulation_is_skipped(self) -> None:
+        for label, (root, frame) in RECORDS.items():
+            with self.subTest(record=label):
+                record = _record(2, frame, root)
+                engine, ext = self._engine([_packet(1), record, _packet(3)])
+                frames = [engine.read_frame(), engine.read_frame()]
+                self.assertNotIn(record, frames)
+                self.assertEqual([f.number for f in frames], ['1', '3'])
+                self.assertEqual(ext._frnum, 2)
+
+    def test_a_trailing_record_ends_the_capture(self) -> None:
+        engine, ext = self._engine([_packet(1), _record(2, RECORD_FRAME, 'systemd_journal')])
+        engine.read_frame()
+        with self.assertRaises(StopIteration):
+            engine.read_frame()
+        self.assertEqual(ext._frnum, 1)
+
+    def test_verbose_mode_prints_a_commented_packet(self) -> None:
+        engine, ext = self._engine([_packet(1, comment=True), _packet(2)])
+        vars(ext).update(_exlyr='none', _exptl='null', _exctx=None, _flag_r=False, _flag_v=True,
+                         _ifnm='test.pcapng', record_header=lambda: None)
+        stream = engine._extmp
+        engine._expkg = types.SimpleNamespace(FileCapture=lambda *args, **kwargs: stream)
+        engine.run()  # installs the verbose handler
+
+        with contextlib.redirect_stdout(io.StringIO()) as out:
+            engine.read_frame()
+            engine.read_frame()
+        self.assertEqual(out.getvalue().splitlines(),
+                         [f'Frame   1: {PROTOCOLS}', f'Frame   2: {PROTOCOLS}'])
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/foundation/engines/test_runtime_engines.py

@@ -432,8 +432,10 @@ def test_pyshark_run_read_frame_and_close(self) -> None:
         class FakeCapture:
             def __init__(self) -> None:
                 self.closed = False
-                self.packet = types.SimpleNamespace(number='7',
-                                                    frame_info=types.SimpleNamespace(protocols='eth:ip:tcp'))
+                # a packet record: its frame layer carries frame.encap_type (#1515)
+                frame = types.SimpleNamespace(layer_name='frame', field_names=['encap_type'],
+                                              protocols='eth:ip:tcp')
+                self.packet = types.SimpleNamespace(number='7', frame_info=frame, layers=[])
 
             def next(self):
                 return self.packet
@@ -466,8 +468,9 @@ def close(self) -> None:
             with mock.patch('pcapkit.toolkit.pyshark.tcp_traceflow', return_value='trace'):
                 packet = engine.read_frame()
         self.assertIs(packet, capture.packet)
-        self.assertEqual(extractor._frnum, 7)
-        self.assertEqual(sink.paths[-1], 'out/Frame 7.json')
+        # numbered by count, as the default engine numbers frames, not by tshark's '7'
+        self.assertEqual(extractor._frnum, 1)
+        self.assertEqual(sink.paths[-1], 'out/Frame 1.json')
         extractor._trace.tcp.assert_called_once_with('trace')
         self.assertEqual(extractor._frame, [packet])
 
@@ -480,7 +483,7 @@ def close(self) -> None:
         with mock.patch('pcapkit.toolkit.pyshark.packet2dict', return_value={'packet': True}):
             with mock.patch('pcapkit.toolkit.pyshark.tcp_traceflow', return_value=None):
                 self.assertIs(engine.read_frame(), capture.packet)
-        self.assertEqual(stream_sink.records[-1][1], 'Frame 7')
+        self.assertEqual(stream_sink.records[-1][1], 'Frame 1')
         stream_mode._trace.tcp.assert_not_called()
 
         disabled, _ = make_extractor(_flag_q=True, _flag_t=False, _flag_d=False)

tests/foundation/test_extraction_record_header_unit.py

@@ -134,7 +134,9 @@ def close(self) -> None:
         return fmt, records
 
     def test_pcap_output_opens_with_the_global_header(self) -> None:
-        packet = types.SimpleNamespace(number='1')
+        # a packet record, as pyshark builds one: its frame layer carries frame.encap_type (#1515)
+        frame = types.SimpleNamespace(layer_name='frame', field_names=['encap_type'])
+        packet = types.SimpleNamespace(number='1', frame_info=frame, layers=[])
         fmt, records = self._run(sample_path('in.pcap'), [packet])
         self.assertEqual(fmt, 'json')
         self.assertEqual(list(records), ['Global Header', 'Frame 1'])

tests/toolkit/test_pyshark_unit.py

@@ -617,9 +617,8 @@ def test_journal_block_then_two_tcp_packets(self) -> None:
 
     def test_the_engine_passes_its_frame_count(self) -> None:
         """:meth:`PyShark.read_frame` hands :func:`tcp_traceflow` its own frame
-        count. Without #1531 that count is still tshark's number, so the journal
-        block shifts it here (2 and 3); with #1531 it is 1 and 2. Either way the
-        traced index is the engine's count, never ``packet.number`` read again.
+        count. The journal block is skipped and not counted (#1515), so the two
+        packets trace as 1 and 2, never as tshark's ``packet.number``.
 
         """
         import types
@@ -651,7 +650,7 @@ def test_the_engine_passes_its_frame_count(self) -> None:
             if packet is not journal:
                 counts.append(extractor._frnum)
         self.assertEqual(traced, counts)
-        self.assertIn(traced, ([2, 3], [1, 2]))
+        self.assertEqual(traced, [1, 2])
 
 
 @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')