Skip to content

A nested schema cannot reach the enclosing packet's fields by name, so CGA Parameters raises KeyError: 'length' #445

Description

@JarryShaw

A nested schema's field callbacks receive a packet dict that does not contain the enclosing schema's fields by name. The enclosing packet is reachable, but only under a __packet__ key, and that convention is documented in exactly one comment in the whole tree. So the natural thing to write — length=lambda pkt: pkt['length'], exactly as every top-level schema writes it — raises KeyError when the schema is nested.

The live casualty

pcapkit/protocols/schema/internet/mh.py:515-521, the CGA Parameters option:

class CGAParameter(Schema):
    ...
    extensions: 'list[CGAExtension]' = OptionField(
        length=lambda pkt: pkt['length'] - 25 - len(pkt['public_key']),
        ...
    )

CGAParameter has no length field. The length this wants belongs to the enclosing CGAParametersOption (:529-536), which reaches it via ListField(length=..., item_type=SchemaField(schema=CGAParameter)).

A well-formed 40-octet option therefore cannot be parsed on f50436a8a:

raw = bytes.fromhex('11040000123400000c1e'
                    '0000000000000000000000000000086f'
                    '0000000020010db8'
                    '00'
                    '3003010203')          # len(raw) == 40
MH(io.BytesIO(raw), len(raw), extension=True)
-> KeyError: 'length'

Four SchemaWarning: packet length < 0 warnings (-17, -25, -26, -31) come out on the way, which is the same arithmetic going negative during the option scan.

Mechanism

SchemaField.pack and SchemaField.unpack (pcapkit/corekit/fields/misc.py) hand the nested schema a dict with the parent nested under a key rather than merged:

return value.pack({
    '__packet__': packet,
})

So pkt['length'] is absent and pkt['__packet__']['length'] is what exists. Confirmed by fixing just that lookup in place, which turns the KeyError into a different error (see below) rather than another KeyError.

Why this is a design problem and not one bad lambda

The __packet__ contract is real but undiscoverable:

  • It is explained in one place, pcapkit/protocols/schema/misc/pcapng.py:155-157, in a docstring belonging to an unrelated helper.
  • There are only two consumers of it in any schema module, both in schema/misc/pcapng.py (:168-170 and :354), and both defensively spell out the fallback themselves: if 'byteorder' not in packet and '__packet__' in packet: / packet.get('__packet__', {}).get('type', 'N/A'). Each reinvents the lookup because there is no helper for it.
  • Schema.unpack's own docstring documents __length__ and __option_padding__ as reserved packet keys but says nothing about __packet__.
  • A schema author cannot tell from the field definition whether their schema will ever be nested, and the same schema class can be used both ways — so "write pkt['length'] at top level, pkt['__packet__']['length'] when nested" is not a rule that can be applied locally.

Worth deciding deliberately rather than patching mh.py. Options, roughly in increasing order of ambition:

  1. Give the nested dict a chained lookup, so a name missing locally falls through to __packet__ automatically. Keeps every existing callback working and makes pkt['length'] mean the obvious thing.
  2. Provide a documented helper (e.g. outer(pkt, 'length')) and use it at the three existing sites plus mh.py.
  3. Merge the parent's fields in directly, which is simplest but lets an inner field name be shadowed by an outer one silently — probably why it was not done.

Fixing this alone does not unblock CGA

With the lookup corrected to pkt['__packet__']['length'], the same packet then fails differently:

FieldValueError: Field parameters has invalid length.

which is the ForwardMatchField length-accounting fault filed separately. Both have to be fixed for the CGA Parameters option to parse, which is why #437 reverted its half-fix rather than shipping a change of exception type — see that PR's "One option deliberately left on the generic handler". #437 pins the current behaviour in test_mh_cga_parameters_option_is_unparsable_upstream so the day this starts working is visible.

Raised out of docs/source/pep.rst, which had been carrying this defect as prose. That page tracks feature requests, not defects.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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