fix(pcapng): bound the skip after a journal binary field to one octet

pcapkit/protocols/schema/misc/pcapng.py

@@ -1758,11 +1758,33 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self':
             should have emitted as a *binary* field, so the entry is malformed
             however it is read.
 
+            A binary field's value used to be followed by a bare
+            :meth:`io.BytesIO.read` with no length -- meant to skip the one
+            newline octet the format puts there, but reading with no argument
+            reads to *end of file* instead. The loop's next ``readline()`` then
+            found nothing and ended the entry, so every field behind a binary
+            one was silently gone, however well-formed. See `#704
+            <https://github.com/JarryShaw/PyPCAPKit/issues/704>`__. The skip is
+            now exactly that one octet, and a terminator that is missing or is
+            not a newline ends the entry with a warning -- the same shape as
+            the short length prefix above -- except when the value itself was
+            already clamped to what the entry held, since that shortfall was
+            reported already and nothing is left behind it to check.
+
         """
         self = cast('Self', super().post_process(packet))
 
         data = []  # type: list[OrderedMultiDict[str, str | bytes]]
-        for entry_buffer in self.entry.split(b'\n\n'):
+        segments = self.entry.split(b'\n\n')
+        for index, entry_buffer in enumerate(segments):
+            if index < len(segments) - 1:
+                # ``split`` consumes the blank line's own newline together
+                # with the one that terminates this entry's last field; put
+                # the latter back, or a binary last field's terminator check
+                # below sees an entry that ends one octet early and warns
+                # over a newline that was in the capture all along
+                entry_buffer += b'\n'
+
             entry = OrderedMultiDict()  # type: OrderedMultiDict[str, str | bytes]
 
             entry_data = io.BytesIO(entry_buffer)
@@ -1786,15 +1808,28 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self':
 
                     length = struct.unpack('<Q', prefix)[0]  # type: int
                     available = len(entry_buffer) - entry_data.tell()
-                    if length > available:
+                    clamped = length > available
+                    if clamped:
                         warn(f'PCAP-NG: [systemd Journal Export] binary field {line!r} '
                              f'declares {length} octet(s) with {available} left in its '
                              f'entry; reading {available}', SchemaWarning,
                              stacklevel=stacklevel())
                         length = available
 
                     entry.add(self._decode_text(line), entry_data.read(length))
-                    entry_data.read()  # Skip trailing newline.
+
+                    if not clamped:
+                        # the one octet the format puts here to terminate the
+                        # field; a value already clamped to the entry's own
+                        # end left nothing behind to check, and was reported
+                        # above
+                        terminator = entry_data.read(1)
+                        if terminator != b'\n':
+                            warn(f'PCAP-NG: [systemd Journal Export] binary field '
+                                 f'{line!r} is not followed by the newline that '
+                                 f'terminates it; ending the entry', SchemaWarning,
+                                 stacklevel=stacklevel())
+                            break
 
             data.append(entry)
         self.data = data

tests/protocols/misc/test_pcapng_unit.py

@@ -4169,8 +4169,13 @@ def test_a_journal_binary_field_cut_short_of_its_length_is_reported(self) -> Non
                 schema_warnings = [entry for entry in caught
                                    if entry.category is SchemaWarning]
                 if supplied + (-(8 + supplied) % 4) >= 8:
-                    # the block's own padding made the prefix up to eight
-                    self.assertEqual(schema_warnings, [])
+                    # the block's own padding made the prefix up to eight,
+                    # giving a valid zero-length field -- but one with nothing
+                    # behind it in the buffer to hold its terminator
+                    self.assertEqual(len(schema_warnings), 1)
+                    self.assertIn('is not followed by', str(schema_warnings[0].message))
+                    self.assertEqual(len(entries), 1)
+                    self.assertEqual(entries[0]['MESSAGE'], b'')
                     continue
                 self.assertEqual(len(schema_warnings), 1)
                 self.assertIn('of the 8 it needs', str(schema_warnings[0].message))
@@ -4187,6 +4192,72 @@ def test_a_well_formed_journal_binary_field_still_reads_its_value(self) -> None:
         self.assertEqual([entry for entry in caught
                           if entry.category.__name__ == 'SchemaWarning'], [])
 
+    def test_journal_fields_following_a_binary_field_are_not_discarded(self) -> None:
+        """#704: skipping a binary field's terminator used to skip everything.
+
+        ``entry_data.read()`` with no argument reads to *end of file*, not past
+        the one newline octet the format puts there, so the outer ``while
+        True`` loop's next ``readline()`` finds nothing and ends the entry.
+        A single binary field cannot show this -- there is nothing behind it
+        to lose -- so the entry needs a *second* binary field, and a text
+        field after that, to tell a length-bounded skip from an unbounded one.
+
+        """
+        entries, caught = self._extract_journal(
+            b'BEFORE=zero\n'
+            b'FIRST\n' + struct.pack('<Q', 3) + b'one\n' +
+            b'SECOND\n' + struct.pack('<Q', 3) + b'two\n' +
+            b'AFTER=three\n')
+
+        self.assertEqual(len(entries), 1)
+        self.assertEqual(entries[0]['BEFORE'], 'zero')
+        self.assertEqual(entries[0]['FIRST'], b'one')
+        self.assertEqual(entries[0]['SECOND'], b'two')
+        self.assertEqual(entries[0]['AFTER'], 'three')
+        self.assertEqual([item for item in caught
+                          if item.category.__name__ == 'SchemaWarning'], [])
+
+    def test_a_binary_last_field_before_a_trailing_separator_is_not_warned(self) -> None:
+        """A cross-review false positive on #722: the terminator check itself.
+
+        ``self.entry.split(b'\\n\\n')`` consumes an entry's last field's own
+        terminating newline together with the blank line that separates it
+        from whatever follows -- a trailing separator an entry *may* carry
+        per ``draft-richardson-opsawg-pcapng-extras-01``. A *text* last
+        field's ``readline()`` never notices; the terminator check added for
+        #704 did, and warned over a newline that was in the capture all
+        along. The split has to give that one octet back to the segment it
+        took it from.
+
+        """
+        entries, caught = self._extract_journal(
+            b'MESSAGE\n' + struct.pack('<Q', 3) + b'abc\n' + b'\n')
+
+        self.assertEqual(len(entries), 2)
+        self.assertEqual(entries[0]['MESSAGE'], b'abc')
+        self.assertEqual(len(entries[1]), 0)
+        self.assertEqual([item for item in caught
+                          if item.category.__name__ == 'SchemaWarning'], [])
+
+    def test_a_binary_field_ending_the_first_of_two_entries_is_not_warned(self) -> None:
+        """The same false positive, with a second real entry behind the split.
+
+        Identical mechanism to the trailing-separator case above, just with
+        real content on the far side of the ``\\n\\n`` instead of nothing --
+        confirming the fix is the split giving back a stolen octet, not a
+        special case for an empty second entry.
+
+        """
+        entries, caught = self._extract_journal(
+            b'A=1\nBIN\n' + struct.pack('<Q', 3) + b'abc\n' + b'\n' + b'B=2\n')
+
+        self.assertEqual(len(entries), 2)
+        self.assertEqual(entries[0]['A'], '1')
+        self.assertEqual(entries[0]['BIN'], b'abc')
+        self.assertEqual(entries[1]['B'], '2')
+        self.assertEqual([item for item in caught
+                          if item.category.__name__ == 'SchemaWarning'], [])
+
     def test_a_journal_binary_field_declaring_more_than_its_entry_is_clamped(self) -> None:
         """A 64-bit length was either fatal or invisible, by magnitude alone.
 
@@ -4212,17 +4283,25 @@ def test_a_journal_binary_field_declaring_more_than_its_entry_is_clamped(self) -
                               str(schema_warnings[0].message))
                 self.assertEqual(entries[0]['BINARY'], b'abc\n\x00')
 
-        # and a length the entry can satisfy is read exactly, and silently --
-        # the clamp reaches only what the entry holds, padding included, so it
-        # must not fire on a field that fits
+        # and a length the entry can satisfy is read exactly -- the clamp
+        # reaches only what the entry holds, padding included, so it must not
+        # fire on a field that fits. Only ``declared == 3`` leaves the real
+        # trailing newline immediately behind the value; every other cut lands
+        # on "abc" itself or the pad octet, which the terminator check reports.
+        remainder = b'abc\n\x00'
         for declared in range(6):
             with self.subTest(declared=declared, expect='untouched'):
                 entries, caught = self._extract_journal(
                     b'BINARY\n' + struct.pack('<Q', declared) + b'abc\n')
 
-                self.assertEqual([entry for entry in caught
-                                  if entry.category is SchemaWarning], [])
-                self.assertEqual(entries[0]['BINARY'], b'abc\n\x00'[:declared])
+                schema_warnings = [entry for entry in caught
+                                   if entry.category is SchemaWarning]
+                if remainder[declared:declared + 1] == b'\n':
+                    self.assertEqual(schema_warnings, [])
+                else:
+                    self.assertEqual(len(schema_warnings), 1)
+                    self.assertIn('is not followed by', str(schema_warnings[0].message))
+                self.assertEqual(entries[0]['BINARY'], remainder[:declared])
 
     def test_a_journal_field_that_is_not_utf8_is_replaced_and_reported(self) -> None:
         """One bad octet in one field used to cost the whole extraction.