diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index 2266e6f663..cb45d92856 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/pcapkit/protocols/schema/misc/pcapng.py @@ -1758,39 +1758,59 @@ 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. + Entries used to be split apart with ``self.entry.split(b'\\n\\n')`` + before a single field was read -- delimiting a *length-prefixed* + format by content, which a binary field's own bytes need no + escaping to defeat. A value that itself contains ``b'\\n\\n'`` was + cut in the middle of its own data, turning what followed it into a + bogus field in a fabricated second entry; a value 2,570 octets long + is worse, since ``struct.pack('`__. The entry is + now walked once, end to end: a length-prefixed field's bytes are + never inspected for structure, only counted out by the prefix that + names them, and a blank line -- found by *reading*, not by + splitting -- is what starts the next entry. The one-octet + terminator that must follow a binary field's value, and the warning + when it is missing, are unchanged from `#722 + `__; walking the + buffer whole rather than pre-slicing it also retires that fix's + newline restoration, which existed only to undo what the slicing + itself had taken away. + + A trailing separator -- a blank line with nothing behind it -- used + to be swallowed instead of ending the entry: with nothing left to + read, the walk stopped without recording that the separator had + been seen at all, so a rebuild lost that one octet and wrote a + :attr:`length` one short of what was read. It is now tracked + explicitly, so a blank line actually read, rather than the block's + own NUL padding or plain end of data, still starts the next entry -- + even an empty one -- matching what splitting on it always did. """ self = cast('Self', super().post_process(packet)) data = [] # type: list[OrderedMultiDict[str, str | bytes]] - 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' - + total = len(self.entry) + entry_data = io.BytesIO(self.entry) + while True: entry = OrderedMultiDict() # type: OrderedMultiDict[str, str | bytes] + # a blank line that was actually *read* -- as opposed to the + # block's own NUL padding, or simply running out of octets -- + # is the separator the format puts between entries, so it + # starts another one, even an empty one, however little is + # left behind it + separator = False - entry_data = io.BytesIO(entry_buffer) while True: - line = entry_data.readline().strip() - if not line or not line.strip(b'\x00'): + raw_line = entry_data.readline() + line = raw_line.strip() + if not line: + separator = bool(raw_line) + break + if not line.strip(b'\x00'): break line_split = line.split(b'=', maxsplit=1) @@ -1807,7 +1827,7 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': break length = struct.unpack(' available if clamped: warn(f'PCAP-NG: [systemd Journal Export] binary field {line!r} ' @@ -1820,9 +1840,16 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': 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 + # field; a value already clamped to the entry's own end + # left nothing behind to check, and was reported above. + # the reader's position is well defined either way -- + # exactly length + 1 octets past where the field name + # started -- so a bad octet here ends only this + # entry's field collection, matching #722: it does not + # abort the walk, which keeps looking for the next + # entry's separator from here. See #728's review for + # why an outer abort was considered and rejected as + # the default. terminator = entry_data.read(1) if terminator != b'\n': warn(f'PCAP-NG: [systemd Journal Export] binary field ' @@ -1832,6 +1859,8 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': break data.append(entry) + if entry_data.tell() >= total and not separator: + break self.data = data return self diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index 2121c4152b..e840a9a8cf 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -4192,15 +4192,126 @@ 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_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. + + At ``2**63`` and above :meth:`io.BytesIO.read` refuses the length outright + with a bare :exc:`OverflowError`; below that it silently returned whatever + happened to be there. Both are the same malformed prefix, so both get the + same answer: clamp to the octets the entry has left, and say so. + + """ + from pcapkit.utilities.warnings import SchemaWarning + + # ``b'BINARY\n' + 8 octets + b'abc\n'`` is 19 octets, so the block pads it + # with one NUL and five octets follow the length prefix + for declared in (6, 1 << 10, 1 << 30, 1 << 62, 1 << 63, 2 ** 64 - 1): + with self.subTest(declared=declared, expect='clamped'): + entries, caught = self._extract_journal( + b'BINARY\n' + struct.pack(' None: + """#728 review: a bad terminator used to abort the whole block. + + The rewrite that walks the entry end to end, rather than pre-slicing + it on ``b'\\n\\n'``, added a ``malformed`` flag that broke the *outer* + per-entry loop on a bad terminator -- so #722's per-entry check + silently widened into a per-block one, and a well-formed entry behind + a corrupted one was dropped entirely, which + :meth:`test_a_journal_binary_field_declaring_more_than_its_entry_is_clamped` + above cannot show because its bad terminator always lands at the + block's own end. There is nothing between the corrupted octet and + ``GOOD=1`` here -- not even the format's own separator -- so recovery + must come from the walk itself continuing, not from resynchronising on + a blank line it happens to find. + + """ + entries, caught = self._extract_journal( + b'DATA\n' + struct.pack(' None: + """The same recovery, with the block's actual entry separator present. + + A well-formed ``\\n\\n`` here is one entry's real trailing newline + plus the separator blank line behind it, each consumed by a distinct + read within *that* entry's own inner loop -- see + :meth:`test_a_binary_field_ending_the_first_of_two_entries_is_not_warned`. + A bad terminator instead ends the current entry's inner loop + immediately, before it gets a chance to look for that separator, so + each of the two octets behind the corrupted byte is read as its own + line by a fresh, otherwise-empty entry -- pinned here as the two + empty dicts between ``DATA`` and ``GOOD``. That is one entry more + than main's delimiter-based split produces for the same bytes, but it + drops nothing: the count is pinned so a future change to the walk + that starts silently discarding ``GOOD=1`` again is caught even + though it is not the last entry. + + """ + entries, caught = self._extract_journal( + b'DATA\n' + struct.pack(' 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. + the one newline octet the format puts there, so the next ``readline()`` + found nothing and ended 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( @@ -4218,16 +4329,17 @@ def test_journal_fields_following_a_binary_field_are_not_discarded(self) -> None 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. + """A blank-line separator is found by reading, not by pre-slicing. + + ``draft-richardson-opsawg-pcapng-extras-01`` lets an entry carry a + trailing separator. The old parser sliced the buffer into segments + with ``self.entry.split(b'\\n\\n')`` before reading a single field, so + the separator's own bytes were gone by the time a binary last field's + terminator was checked, and the check that landed in #722 warned over + a newline that was in the capture all along. Walking the buffer whole + never removes those bytes -- the blank line ends the current entry by + being *read*, and whatever the buffer still holds after it starts the + next one -- so there is nothing here for that check to trip over. """ entries, caught = self._extract_journal( @@ -4240,12 +4352,10 @@ def test_a_binary_last_field_before_a_trailing_separator_is_not_warned(self) -> 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. + """The same case, with a second real entry behind the separator. 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. + real content on the far side of the ``\\n\\n`` instead of nothing. """ entries, caught = self._extract_journal( @@ -4258,50 +4368,79 @@ def test_a_binary_field_ending_the_first_of_two_entries_is_not_warned(self) -> N 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. + def test_a_trailing_separator_with_no_padding_still_starts_an_entry(self) -> None: + """A trailing separator landing exactly on the last octet was dropped. + + The two tests above see their trailing separator followed by the + block's own NUL padding, which the walk also treats as ending an + entry -- so the extra empty entry that padding produces stood in for + the one the separator itself should have produced, and the gap went + unnoticed. Seven octets of field plus one of separator is eight, a + multiple of four, so :meth:`_journal_block` adds none: the blank line + is the last octet in the buffer, ``entry_data.tell()`` reaches + ``total`` in the same read that found it, and the walk used to stop + right there without recording that a separator had been seen at all. + Rebuilding from the result then wrote a :attr:`length` one octet + short of what was actually read. - At ``2**63`` and above :meth:`io.BytesIO.read` refuses the length outright - with a bare :exc:`OverflowError`; below that it silently returned whatever - happened to be there. Both are the same malformed prefix, so both get the - same answer: clamp to the octets the entry has left, and say so. + """ + entries, caught = self._extract_journal(b'ABC=12\n' + b'\n') + + self.assertEqual(len(entries), 2) + self.assertEqual(entries[0]['ABC'], '12') + self.assertEqual(len(entries[1]), 0) + self.assertEqual([item for item in caught + if item.category.__name__ == 'SchemaWarning'], []) + + def test_a_journal_binary_value_containing_a_blank_line_is_not_shredded(self) -> None: + """#723 defect A: a length-prefixed value needs no escaping for ``\\n\\n``. + + ``self.entry.split(b'\\n\\n')`` used to cut a binary field's own value + in the middle whenever that value happened to contain the entry + separator, turning the tail of the value into a bogus field in a + fabricated second entry. The value here carries two such separators; + a parser that only counts bytes out by the declared length, and never + inspects them, must return it whole and warn about nothing. """ - from pcapkit.utilities.warnings import SchemaWarning + entries, caught = self._extract_journal( + b'BEFORE=zero\n' + b'BINARY\n' + struct.pack(' None: + """#723 defect B: the separator can live inside the length prefix itself. - # 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.