From f0565f7942c96de4bd64b64ff7190df6395eb42b Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 8 Oct 2026 09:42:03 -0400 Subject: [PATCH] fix(pcapng)!: keep malformed journal entries and short packet blocks as captured (#1413, #1414) - Journal Export: a line with no terminating newline (e.g. a bare KEY), a binary field with a short length prefix or without its closing newline, and any entry whose parsed fields would not rebuild octet for octet now raise inside the walk, so the whole entry is kept as `entry_raw` and rebuilt verbatim, per the #1406 ruling. Lines are no longer stripped. - SPB: packet data past `original_len` and its padding is kept, with a warning, as #1384 does for the snaplen. - EPB/PB: a Block Total Length below the 32-octet minimum keeps the block body as `block_raw`, which make() writes back at the declared length. - Tests that pinned the old lossy journal parses now assert the entry is kept as captured; new regression module for both issues. --- pcapkit/protocols/misc/pcapng.py | 121 ++++++- pcapkit/protocols/schema/misc/pcapng.py | 313 ++++++++---------- ..._pcapng_kept_as_captured_1413_1414_unit.py | 132 ++++++++ tests/protocols/misc/test_pcapng_unit.py | 281 +++++----------- 4 files changed, 452 insertions(+), 395 deletions(-) create mode 100644 tests/protocols/misc/test_pcapng_kept_as_captured_1413_1414_unit.py diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index 7bd027d667..4a3e69a12d 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -660,6 +660,11 @@ class PCAPNG(ProtocolBase[Data_PCAPNG, Schema_PCAPNG], Enum_BlockType.Simple_Packet_Block, Enum_BlockType.Packet_Block) + #: Packet blocks whose fixed fields span 32 octets, so that a shorter Block + #: Total Length is kept as captured (see :meth:`_read_block_short`). + SHORT_PACKET_TYPES = (Enum_BlockType.Enhanced_Packet_Block, + Enum_BlockType.Packet_Block) + #: Blocks that resolve an interface of their section, mapped to the tag used #: when reporting an interface ID that names no such interface. A Simple #: Packet Block carries no interface ID field and always refers to the @@ -1235,7 +1240,9 @@ def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True, self._ctx = None name = self._lookup_registry(self.__block__, schema.type) - if isinstance(name, str): + if schema.type in self.SHORT_PACKET_TYPES and schema.block.length < 32: + meth = cast('BlockParser', self._read_block_short) + elif isinstance(name, str): meth_name = f'_read_block_{name}' meth = cast('BlockParser', getattr(self, meth_name, self._read_block_unknown)) @@ -1507,7 +1514,13 @@ def _get_payload(self) -> 'bytes': See Also: This is a wrapper function for :meth:`pcapkit.protocols.schema.Schema.get_payload`. + Note: + A packet block kept as captured has no payload (see + :meth:`_read_block_short`). + """ + if isinstance(self.__header__.block, Schema_UnknownBlock): + return b'' return self.__header__.block.get_payload() @staticmethod @@ -2163,6 +2176,12 @@ def _read_block_spb(self, schema: 'Schema_SimplePacketBlock', *, warn(f'PCAP-NG: [Block {header.type}] packet data of {data.captured_len} octet(s) ' f'exceeds the snaplen of {snaplen}; kept as captured', ProtocolWarning, stacklevel=stacklevel()) + # NOTE: Likewise for more packet data than the original length, padding + # aside (:issue:`1414`). + if data.captured_len > (data.original_len + 3) // 4 * 4: + warn(f'PCAP-NG: [Block {header.type}] packet data of {data.captured_len} octet(s) ' + f'exceeds the original length of {data.original_len}; kept as captured', + ProtocolWarning, stacklevel=stacklevel()) return self._decode_next_layer(data, self._get_linktype(0), data.captured_len) # type: ignore[return-value] def _read_block_nrb(self, schema: 'Schema_NameResolutionBlock', *, @@ -2487,6 +2506,71 @@ def _read_block_packet(self, schema: 'Schema_PacketBlock', *, return self._decode_next_layer(data, self._get_linktype(schema.interface_id), schema.captured_length) # type: ignore[return-value] + def _read_block_short(self, schema: 'Schema_EnhancedPacketBlock | Schema_PacketBlock | Schema_UnknownBlock', *, + header: 'Schema_PCAPNG') -> 'Data_EnhancedPacketBlock | Data_PacketBlock': + """Read a PCAP-NG EPB or packet block (obsolete) shorter than its fixed fields. + + The fixed fields of either block span 32 octets, so a smaller Block + Total Length cannot hold them. + + Args: + schema: Parsed block schema, or on the construction path the + :class:`~pcapkit.protocols.schema.misc.pcapng.UnknownBlock` that + :meth:`_make_block_short` built. + header: Parsed PCAP-NG header schema. + + Returns: + Parsed packet data, with the fixed fields the block holds and zero + for those it does not, no packet data and no options. The block body + is kept as ``block_raw``, which the block's ``make`` writes back + verbatim, so the block rebuilds at the length it declared + (:issue:`1414`). + + """ + if isinstance(schema, Schema_UnknownBlock): + body = schema.body + else: + # the octets unpacked for the fixed fields, which start with the + # Block Total Length and run past the end of the block + octets = bytes(schema) + body = octets[4:schema.length - 8] + warn(f'PCAP-NG: [Block {header.type}] block length {schema.length} is below the ' + f'32-octet minimum; kept as captured', ProtocolWarning, stacklevel=stacklevel()) + if octets[schema.length - 8:schema.length - 4] != octets[:4]: + warn(f'PCAP-NG: [Block {header.type}] block length mismatch: {schema.length} != ' + f'{octets[schema.length - 8:schema.length - 4].hex()}', ProtocolWarning, + stacklevel=stacklevel()) + + endian = '<' if self._byte == 'little' else '>' + fields = (body + bytes(20))[:20] + drop_count = 0 + if header.type == Enum_BlockType.Packet_Block: + interface_id, drop_count, timestamp_high, timestamp_low, captured_len, original_len = \ + struct.unpack(f'{endian}HHIIII', fields) + else: + interface_id, timestamp_high, timestamp_low, captured_len, original_len = \ + struct.unpack(f'{endian}IIIII', fields) + timestamp, timestamp_epoch = self._read_timestamp(timestamp_high, timestamp_low, + interface_id=interface_id) + + kwargs = { + 'type': header.type, + 'length': schema.length, + 'section_number': self._sect, + 'number': self._fnum, + 'interface_id': interface_id, + 'timestamp': timestamp, + 'timestamp_epoch': timestamp_epoch, + 'captured_len': captured_len, + 'original_len': original_len, + 'options': self._read_pcapng_options([]), + } # type: dict[str, Any] + data = (Data_PacketBlock(drop_count=drop_count, **kwargs) + if header.type == Enum_BlockType.Packet_Block + else Data_EnhancedPacketBlock(**kwargs)) # type: Data_EnhancedPacketBlock | Data_PacketBlock + data.__update__([('block_raw', body)]) + return self._decode_next_layer(data, self._get_linktype(interface_id), 0) # type: ignore[return-value] + def _read_pcapng_options(self, options_schema: 'list[Schema_Option]') -> 'Option': """Read PCAP-NG options. @@ -4029,6 +4113,8 @@ def _make_block_epb(self, block: 'Optional[Data_EnhancedPacketBlock]' = None, *, Constructed block schema. """ + if block is not None and hasattr(block, 'block_raw'): + return self._make_block_short(block) # type: ignore[return-value] if block is not None: interface_id = block.interface_id timestamp = block.timestamp_epoch @@ -4115,6 +4201,19 @@ def _make_block_spb(self, block: 'Optional[Data_SimplePacketBlock]' = None, *, length2=packet_len + 16, ) + def _make_block_short(self, block: 'Data_EnhancedPacketBlock | Data_PacketBlock') -> 'Schema_UnknownBlock': + """Make a PCAP-NG EPB or packet block (obsolete) kept as captured. + + Args: + block: Block data model, whose ``block_raw`` holds the body of a + block shorter than its fixed fields (see :meth:`_read_block_short`). + + Returns: + Constructed block schema, the body written back verbatim. + + """ + return self._make_block_unknown(data=block.block_raw) # type: ignore[union-attr] + def _make_block_nrb(self, block: 'Optional[Data_NameResolutionBlock]' = None, *, records: 'Optional[Record | list[Schema_NameResolutionRecord | tuple[Enum_RecordType, dict[str, Any]] | bytes]]' = None, options: 'Optional[Option | list[Schema_Option | tuple[Enum_OptionType, dict[str, Any]] | bytes]]' = None, @@ -4219,23 +4318,7 @@ def _make_block_systemd(self, block: 'Optional[Data_SystemdJournalExportBlock]' elif isinstance(entries, bytes): data = entries else: - temp = [] # type: list[bytes] - for entry in entries: - tmp_buf = [] # type: list[bytes] - - for key, val in entry.items(multi=True): - if isinstance(val, str): - buf = f'{key}={val}\n'.encode() - else: - buf = b'%s\n%s%s\n' % ( - key.encode(), - struct.pack(' 'Schema': + """Revise ``schema`` data after unpacking process. + + Args: + packet: Unpacked data. + + Returns: + Revised schema. + + A Block Total Length below the 32 octets of the fixed fields reads the + trailing copy past the block, so the two are not compared here; + :meth:`PCAPNG._read_block_short + ` keeps the block + as captured and compares them itself (:issue:`1414`). + + """ + if self.length < 32: + return self + return super().post_process(packet) + if TYPE_CHECKING: def __init__(self, length: 'int', interface_id: 'int', timestamp_high: 'int', timestamp_low: 'int', captured_len: 'int', original_len: 'int', @@ -1518,17 +1538,20 @@ def _spb_data_length(packet: 'dict[str, Any]') -> 'int': Returns: ``min(original_len, snaplen)``, bounded by the octets the block holds - less its 16 framing octets. A block that holds more than the snaplen - and its padding keeps the excess as packet data instead, so no octet - is dropped and the trailing Block Total Length is read where it sits - (:issue:`1384`). + less its 16 framing octets. A block that holds more than that length + and its padding -- past the snaplen (:issue:`1384`) or past + ``original_len`` (:issue:`1414`) -- keeps the excess as packet data + instead, so no octet is dropped and the trailing Block Total Length is + read where it sits. """ area = max(0, packet['length'] - 16) size = min(packet['original_len'], area) snaplen = packet.get('snaplen') - if snaplen is not None and snaplen < size and area <= (snaplen + 3) // 4 * 4: + if snaplen is not None and snaplen < size: size = snaplen + if area > (size + 3) // 4 * 4: + size = area return size @@ -1933,108 +1956,52 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Self': Revised schema. Note: - Two ways the entry data runs out mid-field are reported rather than - raised, for the reason :func:`nonnegative` gives: a bare - :exc:`struct.error` is neither one of - :mod:`pcapkit.utilities.exceptions` nor an :exc:`EOFError`, so it - would abort the whole extraction rather than this one entry. - - A line of nothing but NUL octets is the block's own 32-bit padding - and ends the entry. ``bytes.strip()`` takes only ASCII whitespace, - so those octets would survive it and be read as the *name* of a - binary field -- which would make every journal entry whose length is - not a multiple of four raise, valid or not, since the padding that - follows it has no 64-bit length prefix behind it to unpack. A - 14-octet ``MESSAGE=hello\\n`` entry is as ordinary as this block - gets. - - A name line whose 64-bit length prefix is itself cut short ends the - entry too. There is nothing to read past the end of the entry, so - stopping at it is what keeps the truncated block parsing. - - A binary field's length is the widest declared length in the format, - and nothing else bounds it against the entry holding it: at ``2**63`` - and above :meth:`io.BytesIO.read` refuses it outright with a bare - :exc:`OverflowError` (``cannot fit 'int' into an index-sized - integer``), and below that it silently returns whatever is there, so - the same malformed prefix would be either fatal or invisible - depending only on its magnitude. A length past the octets the entry - has left raises :exc:`~pcapkit.utilities.exceptions.ProtocolError` - instead, and the whole entry is then kept as the octets captured, as - the one :obj:`bytes` item of :attr:`data`, which the block's ``make`` - writes back verbatim. Clamping the field would rebuild a different - length prefix and a terminator that was never there (:issue:`1406`, - the #1325 ruling that :class:`OptionAreaField` applies to option - areas). - - Field names, keys and values are decoded with ``errors='replace'`` - rather than strictly. A non-UTF-8 octet in any of the three would - raise a bare :exc:`UnicodeDecodeError` -- a :exc:`ValueError`, so - foreign on both counts, and fatal to the whole extraction over one - bad octet in one field. ``'replace'`` is the option this module's own - :class:`~pcapkit.corekit.fields.strings.StringField` already takes for - the same problem, and a value that is not text is a value the writer - should have emitted as a *binary* field, so the entry is malformed - however it is read. - The entry is walked once, end to end, rather than split apart with ``self.entry.split(b'\\n\\n')``: that delimits 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'`` would be 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(' 'list[OrderedMultiDict[str, str | bytes]]': The journal entries. Raises: - ProtocolError: If a binary field declares a length past the octets - left in the entry. + ProtocolError: If the entry is malformed, as :meth:`post_process` + lists. """ data = [] # type: list[OrderedMultiDict[str, str | bytes]] @@ -2060,100 +2027,72 @@ def _read_entries(self) -> 'list[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 + # starts another one, even an empty one separator = False while True: raw_line = entry_data.readline() - line = raw_line.strip() - if not line: - separator = bool(raw_line) + if raw_line == b'\n': + separator = True break - if not line.strip(b'\x00'): + if not raw_line.strip(b'\x00'): break - - if not raw_line.endswith(b'\n') and raw_line.endswith(b'\x00'): - # the format requires this line's own trailing newline, - # and it is missing -- so nothing separates its data from - # the block's 32-bit alignment padding that immediately - # follows it, unlike the padding-only line above, which - # only exists as its own line *because* a real newline - # put it there. Padding is at most three octets (0-3, to - # round the entry up to the next multiple of four), so at - # most the last three octets of `line` can be it; strip - # only that many. Anything past that cannot be padding - # and is left as the data the sender actually sent. - # - # gated on the *raw*, unstripped line ending in NUL: pad - # octets are the only NULs to strip, and it is `raw_line`, - # not `line`, that names the entry's actual last octet. - # `line` is `raw_line.strip()`, which has already dropped - # ASCII whitespace from both ends -- so if the entry's - # real last octet were itself whitespace, `line`'s tail - # would no longer match `raw_line`'s, and NUL octets - # further in would be mistaken for the tail. Padding is - # always NUL, never whitespace, so a `raw_line` ending in - # anything else -- ordinary trailing whitespace included - # -- proves the true padding is zero, and nothing here - # may be stripped. - # the outer gate above already requires `raw_line` -- - # and therefore `line`, since `bytes.strip()` cannot - # remove a trailing NUL -- to end in one, so this loop - # always runs at least once and `pad_octets` is never 0 - # here; there is no zero-padding case left to guard - # against once the gate has passed. - stripped = line - pad_octets = 0 - while pad_octets < 3 and stripped.endswith(b'\x00'): - stripped = stripped[:-1] - pad_octets += 1 - warn(f'PCAP-NG: [systemd Journal Export] entry field {line!r} ' - f'has no terminating newline; treating its last ' - f'{pad_octets} NUL octet(s) as the block\'s own alignment ' - 'padding, not data', SchemaWarning, stacklevel=stacklevel()) - line = stripped + if not raw_line.endswith(b'\n'): + raise ProtocolError(f'PCAP-NG: [systemd Journal Export] entry field {raw_line!r} ' + 'has no terminating newline') + line = raw_line[:-1] line_split = line.split(b'=', maxsplit=1) if len(line_split) == 2: key, value = line_split entry.add(self._decode_text(key), self._decode_text(value)) - else: - prefix = entry_data.read(8) - if len(prefix) < 8: - warn(f'PCAP-NG: [systemd Journal Export] binary field {line!r} ' - f'declares its length in {len(prefix)} octet(s) of the 8 it ' - f'needs; ending the entry', SchemaWarning, - stacklevel=stacklevel()) - break - - length = struct.unpack(' available: - raise ProtocolError(f'PCAP-NG: [systemd Journal Export] binary field {line!r} ' - f'declares {length} octet(s) with {available} left in its entry') - - entry.add(self._decode_text(line), entry_data.read(length)) - - # the one octet the format puts here to terminate the - # field. the reader's position is well defined -- exactly - # length + 1 octets past where the field name started -- - # so a bad octet here ends only this entry's field - # collection: it does not abort the walk, which keeps - # looking for the next entry's separator from here. - 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 + continue + + prefix = entry_data.read(8) + if len(prefix) < 8: + raise ProtocolError(f'PCAP-NG: [systemd Journal Export] binary field {line!r} ' + f'declares its length in {len(prefix)} octet(s) of the 8 it needs') + + length = struct.unpack(' available: + raise ProtocolError(f'PCAP-NG: [systemd Journal Export] binary field {line!r} ' + f'declares {length} octet(s) with {available} left in its entry') + entry.add(self._decode_text(line), entry_data.read(length)) + + if entry_data.read(1) != b'\n': + raise ProtocolError(f'PCAP-NG: [systemd Journal Export] binary field {line!r} ' + 'is not followed by the newline that terminates it') data.append(entry) if entry_data.tell() >= total and not separator: break return data + @staticmethod + def dump_entries(entries: 'list[OrderedMultiDict[str, str | bytes]]') -> 'bytes': + """Serialise journal entries, without the block's 32-bit padding. + + Args: + entries: The journal entries. + + Returns: + Each field as ``KEY=value\\n``, or for a binary value as the name, + a newline, its 64-bit little-endian length, the value and a newline, + with one blank line between entries. + + """ + temp = [] # type: list[bytes] + for entry in entries: + tmp_buf = [] # type: list[bytes] + for key, val in entry.items(multi=True): + if isinstance(val, str): + tmp_buf.append(f'{key}={val}\n'.encode()) + else: + tmp_buf.append(b'%s\n%s%s\n' % (key.encode(), struct.pack(' 'str': """Decode a journal field name, key or value, reporting what did not decode. @@ -2470,6 +2409,26 @@ class PacketBlock(BlockType, code=Enum_BlockType.Packet_Block): #: Block total length. length2: 'int' = UInt32Field(callback=byteorder_callback) + def post_process(self, packet: 'dict[str, Any]') -> 'Schema': + """Revise ``schema`` data after unpacking process. + + Args: + packet: Unpacked data. + + Returns: + Revised schema. + + A Block Total Length below the 32 octets of the fixed fields reads the + trailing copy past the block, so the two are not compared here; + :meth:`PCAPNG._read_block_short + ` keeps the block + as captured and compares them itself (:issue:`1414`). + + """ + if self.length < 32: + return self + return super().post_process(packet) + if TYPE_CHECKING: def __init__(self, length: 'int', interface_id: 'int', drop_count: 'int', timestamp_high: 'int', timestamp_low: 'int', captured_length: 'int', diff --git a/tests/protocols/misc/test_pcapng_kept_as_captured_1413_1414_unit.py b/tests/protocols/misc/test_pcapng_kept_as_captured_1413_1414_unit.py new file mode 100644 index 0000000000..e844def35a --- /dev/null +++ b/tests/protocols/misc/test_pcapng_kept_as_captured_1413_1414_unit.py @@ -0,0 +1,132 @@ +# -*- coding: utf-8 -*- +"""GitHub issues #1413 and #1414: malformed PCAP-NG blocks rebuild byte-exactly. + +* #1413 -- a Journal Export binary field filling its entry with no closing + newline gained one on rebuild (28 octets in, 32 out), and a bare ``KEY`` with + no ``=`` and no newline was dropped (16 in, 12 out). Under the #1406 ruling the + whole entry is now kept as captured, as ``entry_raw``. +* #1414 -- a Simple Packet Block holding more data than its ``original_len`` + lost the excess (24 in, 20 out), and an Enhanced Packet Block or Packet Block + below its 32-octet minimum rebuilt at 32. Both now keep the octets captured. + +:mod:`pcapkit` is imported inside each test, after +:func:`~tests._support.reimport_once_per_class`. + +""" + +import struct +import unittest +import warnings +from decimal import Decimal + +from tests.protocols.misc.test_pcapng_roundtrip2_unit import PCAPNGTestCase + + +def _block(type_: 'int', body: 'bytes') -> 'bytes': + """A little-endian block of ``type_`` around ``body``.""" + length = len(body) + 12 + return struct.pack(' 'bytes': + """A systemd Journal Export Block holding ``entry``, zero-padded to 32 bits.""" + return _block(9, entry + bytes(-len(entry) % 4)) + + +class TestPCAPNGKeptAsCaptured(PCAPNGTestCase): + """Pin that malformed journal entries and packet blocks rebuild as captured.""" + + def _parse_warned(self, octets: 'bytes') -> 'tuple[object, list[str]]': + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + parsed = self._parse(octets) + return parsed, [str(item.message) for item in caught] + + def test_malformed_journal_entries_are_kept_and_rebuild(self) -> None: + """#1413: the issue's two shapes, plus a bare key after a good field.""" + for entry, reason, size in ((b'BIN\n' + struct.pack(' None: + """#1414: ``original_len=3`` with 8 data octets keeps all 8.""" + octets = _block(3, struct.pack(' None: + """An SPB of ``original_len=3`` with one pad octet still reads 3 octets.""" + octets = _block(3, struct.pack(' None: + """#1414: every Block Total Length below 32 rebuilds at its own length.""" + from pcapkit.const.pcapng.block_type import BlockType + + for type_ in (BlockType.Enhanced_Packet_Block, BlockType.Packet_Block): + for length in range(12, 32, 4): + with self.subTest(type=type_, length=length): + octets = _block(int(type_), bytes(length - 12)) + parsed, messages = self._parse_warned(octets) + self.assertTrue(any(f'block length {length} is below the 32-octet minimum' in text + for text in messages), messages) + self.assertFalse(any('block length mismatch' in text for text in messages), messages) + self.assertEqual(parsed.info.block_raw, octets[8:-4]) # type: ignore[attr-defined] + self.assertEqual(parsed.info.captured_len, 0) # type: ignore[attr-defined] + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + self.assertRebuilds(octets) + + def test_short_epb_keeps_the_fields_it_holds(self) -> None: + """A 28-octet EPB holds its interface ID, timestamp and captured length.""" + octets = _block(6, struct.pack(' None: + """The block after a short EPB is read where it sits, and the EPB rebuilds.""" + from pcapkit.foundation.extraction import Extractor + from pcapkit.protocols.misc.pcapng import PCAPNG + from tests.protocols.misc.test_pcapng_unit import NamedBuffer + + shb = struct.pack(' Non self.assertEqual([entry.category.__name__ for entry in caught if entry.category.__name__ == 'SchemaWarning'], []) - def test_a_journal_binary_field_cut_short_of_its_length_is_reported(self) -> None: + def _assert_journal_kept(self, entry: bytes, reason: str): + """Assert that ``entry`` is kept as captured, for ``reason``; return the warnings.""" + from pcapkit.utilities.warnings import ProtocolWarning + + entries, caught = self._extract_journal(entry) + kept = [item for item in caught if item.category is ProtocolWarning + and 'kept as captured' in str(item.message)] + self.assertEqual(len(kept), 1, [str(item.message) for item in caught]) + self.assertIn(reason, str(kept[0].message)) + self.assertEqual(entries, ()) + return caught + + def test_a_journal_binary_field_cut_short_of_its_length_is_kept_as_captured(self) -> None: """The reviewer's case: fewer than eight octets for a 64-bit length. ``struct.unpack('= 8: - # 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)) - self.assertEqual(len(entries), 1) - self.assertEqual(len(entries[0]), 0) + self._assert_journal_kept(b'MESSAGE\n' + bytes(supplied), 'is not followed by') + else: + self._assert_journal_kept(b'MESSAGE\n' + bytes(supplied), 'of the 8 it needs') def test_a_well_formed_journal_binary_field_still_reads_its_value(self) -> None: """The guard is a shortfall check, not a refusal of binary fields.""" @@ -4460,107 +4458,46 @@ def test_a_journal_binary_field_declaring_more_than_its_entry_is_kept_as_capture and the extraction says so and carries on (#1406). """ - from pcapkit.utilities.warnings import ProtocolWarning, SchemaWarning + 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='kept'): - 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_kept_as_captured` - 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. + self._assert_journal_kept(entry, 'is not followed by') - """ - entries, caught = self._extract_journal( - b'DATA\n' + struct.pack(' None: + """A binary field's value followed by anything but its newline (#1413). - self.assertEqual(len(entries), 2) - self.assertEqual(entries[0]['DATA'], b'abc') - self.assertEqual(entries[1]['GOOD'], '1') - schema_warnings = [item for item in caught - if item.category.__name__ == 'SchemaWarning'] - self.assertEqual(len(schema_warnings), 1) - self.assertIn('is not followed by', str(schema_warnings[0].message)) - - def test_a_bad_terminator_before_a_real_separator_still_reaches_the_next_entry(self) -> 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. + The walk used to end only that entry's field collection and carry on, + which dropped the bad octet from the rebuild. The whole entry is kept as + captured instead, with or without a separator behind the bad octet. """ - entries, caught = self._extract_journal( - b'DATA\n' + struct.pack(' None: + """#1413: a value filling the entry exactly, with no closing newline.""" + self._assert_journal_kept(b'BIN\n' + struct.pack(' None: """#704: skipping a binary field's terminator used to skip everything. @@ -4701,73 +4638,40 @@ def test_a_journal_binary_length_prefix_beginning_with_a_blank_line_is_not_shred self.assertEqual([item for item in caught if item.category.__name__ == 'SchemaWarning'], []) - def test_a_journal_field_that_is_not_utf8_is_replaced_and_reported(self) -> None: + def test_a_journal_field_that_is_not_utf8_is_reported_and_kept_as_captured(self) -> None: """One bad octet in one field used to cost the whole extraction. :exc:`UnicodeDecodeError` is a :exc:`ValueError`, so it is neither in - :mod:`pcapkit.utilities.exceptions` nor an :exc:`EOFError`. Asserted for - all three sites that decode -- a field name, a key and a value. + :mod:`pcapkit.utilities.exceptions` nor an :exc:`EOFError`. The octet is + reported, and since the replaced text would not rebuild it, the entry is + kept as captured. Asserted for all three sites that decode -- a field + name, a key and a value. """ from pcapkit.utilities.warnings import SchemaWarning - cases = { - 'key': (b'ME\xffSAGE=hello\n', 'ME�SAGE', 'hello'), - 'value': (b'MESSAGE=hel\xfflo\n', 'MESSAGE', 'hel�lo'), - 'binary field name': (b'BIN\xffARY\n' + struct.pack(' None: - """#794: no trailing newline lets the block's own padding read as data. - - The block pads its content to a 32-bit boundary with NULs, and the - existing padding-only-line guard only fires when the real content - ahead of it ended with its own newline, putting the padding on a - line of its own. Without that newline, ``readline()`` runs straight - through the field's value and into the padding behind it, returning - both together -- and ``bytes.strip()`` still will not touch the NUL - octets, since they are not ASCII whitespace. Swept over every - padding count the alignment rule can produce (0-3 octets), keyed by - how many octets ``MESSAGE=...`` needs trimmed from ``entry`` to land - on each remainder mod four. - - """ - from pcapkit.utilities.warnings import SchemaWarning - - cases = { - 0: b'MESSAGE=abcd', # 12 octets, already a multiple of four - 1: b'MESSAGE=abc', # 11 octets, one NUL pads it to 12 - 2: b'MESSAGE=hi', # 10 octets, two NULs pad it to 12 - 3: b'MESSAGE=hello', # 13 octets, three NULs pad it to 16 - } - for pad_octets, entry in cases.items(): - with self.subTest(pad_octets=pad_octets): - entries, caught = self._extract_journal(entry) + def test_a_journal_entry_missing_its_final_newline_is_kept_as_captured(self) -> None: + """#794, #1413: no trailing newline, so nothing rebuilds the last field. - self.assertEqual(len(entries), 1) - self.assertEqual(entries[0]['MESSAGE'], entry.split(b'=', 1)[1].decode()) + The block's own NUL padding then sits on the same line as the field, so + reading it as data returned padding, and stripping it lost the missing + newline. Swept over every padding count the alignment rule can produce + (0-3 octets), and over a bare ``KEY`` with no ``=``. - schema_warnings = [item for item in caught - if item.category is SchemaWarning] - if pad_octets == 0: - # nothing was stripped, so there is nothing to warn about - self.assertEqual(schema_warnings, []) - else: - self.assertEqual(len(schema_warnings), 1) - self.assertIn('has no terminating newline', str(schema_warnings[0].message)) - self.assertIn(f'last {pad_octets} NUL octet(s)', str(schema_warnings[0].message)) + """ + for entry in (b'MESSAGE=abcd', b'MESSAGE=abc', b'MESSAGE=hi', b'MESSAGE=hello', + b'KEY', b'KEYS', b'A=1\nKEY'): + with self.subTest(entry=entry): + self._assert_journal_kept(entry, 'has no terminating newline') def test_a_terminated_journal_value_ending_in_a_nul_octet_is_kept_whole(self) -> None: """A legitimate value ending in NUL is not the same shape as padding. @@ -4786,44 +4690,21 @@ def test_a_terminated_journal_value_ending_in_a_nul_octet_is_kept_whole(self) -> self.assertEqual([item for item in caught if item.category.__name__ == 'SchemaWarning'], []) - def test_a_terminator_less_line_whose_raw_tail_is_whitespace_has_zero_padding(self) -> None: - """Cross-review on #795: gating on ``raw_line``, not ``line``, matters. - - ``line`` is ``raw_line.strip()`` -- ASCII whitespace already gone from - both ends. The first cut of #794's fix checked ``raw_line`` for the - missing newline but then counted padding off ``line``, so whenever - the entry's real last octet was itself ASCII whitespace (not the - newline the format wants, but whitespace all the same), ``strip()`` - had already eaten it, exposing whatever NUL octets came *before* it - as if they were now the tail -- and those get read as block padding - that never existed. Padding is NUL, never whitespace, and - ``bytes.strip()`` does not touch NUL, so a ``raw_line`` ending in - whitespace is proof on its own that the true padding is zero: the - block's own last octet, unpadded, IS that whitespace. Swept over - every octet :meth:`bytes.strip` treats as ASCII whitespace -- space, - CR, tab, vtab and formfeed -- against a value that ends in NUL for a - genuine reason of its own -- a NUL is valid UTF-8 -- rather than by - accident. + def test_a_terminator_less_line_whose_raw_tail_is_whitespace_is_kept_as_captured(self) -> None: + """Cross-review on #795: a whitespace tail is not a newline. - """ - from pcapkit.utilities.warnings import SchemaWarning + ``bytes.strip()`` would take the whitespace and expose NUL octets + before it as if they were padding. The line has no terminating newline, + so the entry is kept as captured, whatever octet it ends in. Swept over + every octet :meth:`bytes.strip` treats as ASCII whitespace other than + the newline. - cases = { - 'space': (b'MESSAGE=hi\x00 ', 'hi\x00'), - 'CR': (b'MESSAGE=hi\x00\r', 'hi\x00'), - 'tab': (b'MESSAGE=\x00\x00\x00\t', '\x00\x00\x00'), - 'vtab': (b'MESSAGE=hi\x00\x0b', 'hi\x00'), - 'formfeed': (b'MESSAGE=\x00\x00\x00\x0c', '\x00\x00\x00'), - } - - for tail, (entry, expected) in cases.items(): + """ + for tail, entry in {'space': b'MESSAGE=hi\x00 ', 'CR': b'MESSAGE=hi\x00\r', + 'tab': b'MESSAGE=\x00\x00\x00\t', 'vtab': b'MESSAGE=hi\x00\x0b', + 'formfeed': b'MESSAGE=\x00\x00\x00\x0c'}.items(): with self.subTest(tail=tail): - entries, caught = self._extract_journal(entry) - - self.assertEqual(len(entries), 1) - self.assertEqual(entries[0]['MESSAGE'], expected) - self.assertEqual([item for item in caught - if item.category is SchemaWarning], []) + self._assert_journal_kept(entry, 'has no terminating newline') def test_a_journal_binary_value_ending_in_nul_octets_is_unreachable_by_794(self) -> None: """A binary field's value is read by its own length, never by line.