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.