Skip to content

Every systemd journal field after the first binary field is discarded: entry_data.read() reads to EOF where it means to skip one newline #704

Description

@JarryShaw

Found while fixing #678 (PR #699), which fixes the exception leak two lines above this one and leaves
this alone deliberately: it is silent data loss on valid input rather than a length reaching a read, so
it changes parse output for well-formed captures and wants its own review.

Every journal field after the first binary field is discarded

pcapkit/protocols/schema/misc/pcapng.py, in SystemdJournalExportBlock.post_process:

                    length = struct.unpack('<Q', prefix)[0]  # type: int
                    entry.add(line.decode('utf-8'), entry_data.read(length))
                    entry_data.read()  # Skip trailing newline.

The comment says "skip trailing newline"; io.BytesIO.read() with no argument reads to EOF. So the
while True loop's next readline() returns b'', breaks, and everything after the binary field is
gone. Measured on 08f5b8df7 (PR #699's head; identical on f0999858e), against
pcapkit.__file__ = .../pcapkit/__init__.py printed on the run:

entry data parsed
b'BINARY\n' + <8-octet length 3> + b'abc\nAFTER=one\nMORE=two\n' [('BINARY', b'abc')] — AFTER and MORE lost
b'BEFORE=zero\nBINARY\n' + <8-octet length 3> + b'abc\n' [('BEFORE', 'zero'), ('BINARY', b'abc')] — correct
b'BEFORE=zero\nAFTER=one\nMORE=two\n' all three — correct

No warning, no error. A field before the binary one survives; every field after it does not.

Why it matters

The systemd Journal Export Format puts no ordering constraint on binary fields — a binary field is a
name line, a 64-bit little-endian length, that many octets, and a newline, and it may appear anywhere in
an entry. MESSAGE is routinely binary (it is whenever the message contains a newline or a non-UTF-8
octet), and it is conventionally written early, so the common case is the broken one: every field
after MESSAGE disappears.

The fix, and the one thing it has to decide

entry_data.read() should consume exactly the one octet the format puts there:

entry_data.read(1)  # the newline that terminates a binary field

What wants deciding is what to do when that octet is not a newline, or is absent because the entry
was cut short. The surrounding code now treats "the entry ran out" as end-of-entry with a
SchemaWarning (PR #699, for the 64-bit length prefix immediately above), and the same shape is
probably right here: a binary field whose terminator is missing or wrong is a malformed entry, and
reporting it beats both raising and silently resynchronising at the wrong offset.

Note this is not the same defect as the struct.error PR #699 fixes, and fixing that one does not
help here: it guards the length prefix being short, whereas this is the read after a
successfully-parsed value.

Scope

Not fixed in #699 for the same reason it is filed separately: recovering the discarded fields changes
the parse output of every journal export block that has a binary field followed by anything, so it is a
behaviour change on valid captures and needs a breaking argument of its own, plus a fixture that
actually exercises a multi-field journal entry — examples/captures/ has none, so #699's coverage of
this block comes entirely from hand-built buffers.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions