From 1f340cb13adc5df0807f24b6cfde457389925fb7 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 23 Sep 2026 17:05:20 -0400 Subject: [PATCH] fix(pcapng): bound the skip after a journal binary field to one octet Every systemd Journal Export field behind the first binary field was silently discarded. - Problem: `SystemdJournalExportBlock.post_process` skipped a binary field's trailing newline with a bare `entry_data.read()`, which reads to EOF rather than past one octet, so the entry-parsing loop's next `readline()` found nothing and ended the entry there. - Fix: bound the skip to `entry_data.read(1)`. A terminator that is missing or is not that newline ends the entry with a `SchemaWarning`, mirroring the short-length-prefix guard above it; skipped when the value was already clamped to the entry's own end. - Cross-review found a false positive: `self.entry.split(b'\n\n')` eats a real entry's last field's terminator along with the blank-line separator, so a binary last field followed by a trailing separator or a second entry warned over a newline that was actually present. Re-append `b'\n'` to every split segment but the last to restore it. - Two existing fixtures relied on the old unbounded read to end cleanly without a real terminator; updated their expectations. Added fixtures for the multi-binary-field discard, the trailing-separator false positive, and the two-entries false positive. `coverage run -m pytest tests/protocols/misc/test_pcapng_unit.py`: pcapng.py schema module 546 stmts / 78 branches, 100% both; 81 tests (1753 subtests) pass. --- pcapkit/protocols/schema/misc/pcapng.py | 41 +++++++++- tests/protocols/misc/test_pcapng_unit.py | 95 ++++++++++++++++++++++-- 2 files changed, 125 insertions(+), 11 deletions(-) diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index 99de4fea1..2266e6f66 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/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 + `__. 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,7 +1808,8 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': length = struct.unpack(' 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, @@ -1794,7 +1817,19 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': 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 diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index 6de7128fe..2121c4152 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/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(' 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(' 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(' 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(' None: """One bad octet in one field used to cost the whole extraction.