corekit: a negative field length only warns in Schema.unpack, instead of raising ProtocolError

[JarryShaw]

Split off #799's review. httpv2.py:223's frame-length guard has been fixed
there so a buffer under nine octets, or a declared length exceeding the
buffer, is rejected uniformly before the schema layer runs. That closes the
outer-header truncation class, but not a related one one level deeper:
pkt['__length__'] in Schema.unpack (schema.py:894-899) is decremented by
each field's nominal width regardless of how many octets the buffer
actually had, and going negative is only ever a SchemaWarning — never a
raise. A field whose own length=lambda pkt: pkt['__length__']-style
callback resolves to that negative number builds a struct template like
'-5s', and struct.calcsize raises a bare struct.error for it: not a
ProtocolError, not a ValueError, uncatchable by ordinary caller code.

Reproduction, httpv2 specifically (buffer clears the fixed 9-octet header,
but the frame type's own fixed-width payload fields don't fit in what's
left):

import io
from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2
data = b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff' * 7  # GOAWAY, 16 octets
HTTPv2(io.BytesIO(data), 16)
# struct.error: bad char in struct format

Same shape at other sizes/types: GOAWAY at 9-16 octets (stream+error are
eight fixed octets alone), PUSH_PROMISE at 9-12, and any PADDED
DATA/HEADERS/PUSH_PROMISE whose pad_len exceeds what remains.

pkt['__length__'] is generic machinery, not an httpv2 particular — at
least these schema modules also key a field's length off it and would need
checking for the same latent crash before any fix lands:

• pcapkit/protocols/schema/application/ftp.py
• pcapkit/protocols/schema/application/httpv1.py
• pcapkit/protocols/schema/application/httpv2.py
• pcapkit/protocols/schema/application/ngap.py
• pcapkit/protocols/schema/internet/hip.py
• pcapkit/protocols/schema/internet/ipv6_route.py
• pcapkit/protocols/schema/internet/mh.py
• pcapkit/protocols/schema/link/ethernet.py
• pcapkit/protocols/schema/misc/pcapng.py
• pcapkit/protocols/schema/transport/sctp.py

Proposed fix: make a negative resolved field length raise ProtocolError
(or a dedicated subclass) in Schema.unpack and/or
FieldBase.length/Field.unpack, in place of the current
warn(f'packet length < 0: ...', SchemaWarning, ...). That closes the
inner-payload class the same way #799 closed the outer-header one, and lets
HTTP._guess_version's last arm in pcapkit/protocols/application/http.py
drop its struct.error suppression for good. Scoped as its own issue
because the change is in shared schema/field machinery, not a single
protocol, and needs the blast-radius check above before it lands.

[JarryShaw]

Labelled bug,fix,breaking. breaking is not precautionary — I measured it, and the measurement also shows one of the two fixes proposed here would be wrong.

The body offers the raise "in Schema.unpack and/or FieldBase.length/Field.unpack". Those are different conditions, and only the second is safe. schema.py:898-900 warns when the running counter packet['__length__'] goes negative — which happens on inputs that parse perfectly well today:

PROVENANCE: /tmp/v801/pcapkit/__init__.py
SETTINGS declared 15, buffer 9  (exact header) : PARSED, no warnings
SETTINGS declared 15, buffer 13 (4 payload)    : PARSED, warnings=['SchemaWarning', 'SchemaWarning']

That second case succeeds while warning twice. Convert that site to a raise and it starts rejecting parses that work now — so the naive reading of this issue is a regression, not a fix.

The condition that is actually broken is a resolved field length being negative, which is what reaches struct.calcsize('-5s'). field.py:273 is where the template is built; schema.py:857's length = field.length is where the negative value is consumed. Raising there rejects exactly the inputs that crash today and nothing else.

So the scope should read: raise on a negative resolved field length; leave the running-counter warning alone, or tighten the counter warning separately with its own evidence. Worth stating in the body before anyone picks this up, because the two sites are four lines apart and the wrong one is the more obvious.

Two corrections to the citations while I was in there — both off by a small amount against main (477ed00c4), the same class of drift that has bitten twice today:

• schema.py:894-899 → the warn block is at :898-900, and the packet['__length__'] -= length it follows is at :896.
• The blast-radius list is right in substance. Worth adding that pcapng.py is the one file on it that another open PR (feat(corekit,schema,utilities)!: enforce @final at runtime on Info and Schema (#778) #788) is also editing, so whoever takes this should check the ordering against that.

Not blocked — it is actionable now, and it is what would let #802's _guess_version narrowing be safe. No agent is on it yet.

[JarryShaw]

Correcting myself: this does need blocked, and my "not blocked — it is actionable now" two comments up was wrong. I reasoned from the issue text instead of checking which files the open PRs hold.

Checkable blocker: #788 merged.

gh pr view 788 -R JarryShaw/PyPCAPKit --json state,mergedAt

#788 edits both files this fix has to change:

#788 (94416199e):  pcapkit/protocols/schema/schema.py        +121/-8
                   pcapkit/protocols/schema/misc/pcapng.py   +10/-1
#802, #803, #657:  neither

The raise belongs at schema.py:857 (length = field.length) and/or pcapkit/corekit/fields/field.py:273 (where the '-5s' template is built), and pcapng.py is on this issue's own blast-radius list. #788 rewrites Schema.__new__/__init_subclass__ in the same file and hoists super().__init_subclass__() in the other, so landing this first means one of the two rewrites silently discards the other's edits — no conflict marker, the later write just wins.

#788 is green at 94416199e, awaiting a cross-review verdict, and I have already verified it merges cleanly into current main with its tests passing, so this hold should be short.

Not needs: decision: the scope question I raised above — raise on a resolved field length, leave the running-counter warning alone — I answered with measurement rather than leaving it open, and the evidence is in that comment. Flagging one thing for your eye rather than blocking on it: this is breaking in shared schema machinery with ten schema modules on the blast-radius list, so if you would rather it stay a warning and have _guess_version keep its struct.error suppression instead, say so and I will close this as wontfix rather than spend a worker on it.

[JarryShaw]

Unblocked — #788 merged at 21:40:55Z as ccf5f623b, so pcapkit/protocols/schema/schema.py and schema/misc/pcapng.py are free. main is now f046b38f8.

Dispatching a worker. Re-stating the scope correction from above, because the obvious reading of this issue is a regression rather than a fix:

• Raise on a negative resolved field length — consumed at schema.py:857 (length = field.length), turned into a '-5s' template at pcapkit/corekit/fields/field.py:273.
• Do not touch the running-counter warning at schema.py:898-900. Measured: SETTINGS declared 15, buffer 13 parses successfully today while warning twice, so converting that site would reject working input.

One addition from #802's cross-review, which matters for scoping: read()'s schema.length > length guard cannot prevent these escapes, because the crash happens inside Schema.unpack during unpack() and read() never runs. So this is not a read() fix and must not be written as one.

Residual to close, measured at 3d85e56f6: 1136 bare struct.error escapes via directly-constructed HTTPv2 — GOAWAY at buflen 9–16, PUSH_PROMISE 9–12, any over-padded DATA/HEADERS/PUSH_PROMISE. Unchanged from base; the HTTP() route is already clean.