utilities: reject @prepare's extra arguments, and stop treating a declared zero length as EOF - #461
Conversation
…lared zero length as EOF - prepare() called func(cls, data, length, packet) unconditionally, so the *args/**kwargs its own docstring promised implementors were never forwarded: a misspelled or unsupported argument (or `length` supplied both positionally and by keyword) was silently discarded, no error, no warning. Census: @prepare decorates exactly one function in the tree, Schema.unpack, whose real signature never had more than these four params, and no call site anywhere passes extras. Chosen fix: drop the promise and raise TypeError on any leftover positional or keyword argument, the same as an ordinary over-called function would. (#454) - prepare() raised a bare EOFError whenever length == 0, conflating "the frame reader's stream is genuinely exhausted" with "this schema (often nested) was declared to have nothing to read". Fix: only raise when length was *derived* by measuring what's left in data (length was None going in); a *declared* zero -- explicit, even if zero -- unpacks to an empty instance instead. The still-fatal case now raises StreamEOFError (pcapkit.utilities.exceptions), a subclass of EOFError so existing `except (EOFError, StopIteration)` handlers are unaffected. (#458) Build/test: brazil-build n/a (upstream OSS repo); full suite 982 passed, 17 skipped, 0 failed (999 collected) vs baseline 977/17/0 (994 collected) at da24227 -- the delta is exactly the 5 new tests added.
|
Standing in for Copilot review (out of tokens). Reviewed CI
Base drift
#454 -- extras silently discardedCensus claim verified independently: The new
#458 -- bare EOFError on zero length
Independently confirmed Traced every route into
Test countsInterpreter:
Delta is exactly the 5 new tests in Verified each of the 5 new tests independently fails against All match the PR description's quoted text exactly. VerdictGOOD TO MERGE at |
`StreamEOFError(BaseError, EOFError)` was added at exceptions.py:420 without a matching autoexception entry, so it was the only exception in that module absent from the reference. `EOFError` had no category section, so this adds one after `struct.error`, mirroring StructError's directive and keeping the document's section order aligned with the module's class order.
main gained #461 (closing #458: prepare's @prepare decorator now distinguishes a declared zero length from a derived one, raising StreamEOFError only for the latter) since this branch's previous merge, on top of #462 (closing #459, already handled). Between the two, all three remaining httpv2-frame entries this PR's own fix had exposed -- DATA, HEADERS, CONTINUATION -- now round-trip cleanly too. Verified directly against the round-trip harness: all three return 'OK'. Rewrote the HTTP/2 section's comment block to summarise all six frames' history (PUSH_PROMISE/PING via #445 itself, DATA/HEADERS/ CONTINUATION via #461, SETTINGS via #462) now that none of them need an entry. Also merged origin/main (fa12895, #461) -- clean auto-merge, no conflicts, confirmed against #464's changes to tests/protocols/internet/test_mh_unit.py (different hunks, and its own test run clean: 62 passed, 272 subtests, 0 failed). Verified: mypy pcapkit -> 124 errors/40 files (main at fa12895: 125, one more than its previous 124 -- unrelated to this branch, and this branch stays one fewer than whatever main's own count is, from the same pre-existing type: ignore cleanup as before). Round-trip harness: 7 passed, 363 subtests passed, 0 failed. mh-extension/* re-verified 'OK' again on this merge (all four).
1 participant
Closes #454
Closes #458
Both defects live in
pcapkit/utilities/decorators.py'spreparedecorator, on top of #450 (da2422728), which fixed a third, related defect in the same lines (args[0..3]read unconditionally). Branched fromda2422728.#454 -- extras silently discarded
Mechanism:
prepare's docstring told implementors the decorated function receives*args: 'typing.Any', **kwargs: 'Any', but the wrapper calledfunc(cls, data, length, packet)(decorators.py:238atda2422728) -- neither was ever forwarded. A misspelled or unsupported argument produced a successful parse and no signal. A narrower case:lengthsupplied both positionally and by keyword silently kept the positional and dropped the keyword, becausekwargs.pop('length', ...)only runs when the positional slot (args[2]) was not supplied (decorators.py:215-216).Census (the evidence for the design choice):
@preparedecorates exactly one function in the whole tree --Schema.unpack(pcapkit/protocols/schema/schema.py:664) -- whose real signature has never had more than(cls, data, length=None, packet=None); it does not itself declare*args, **kwargs, despite what the decorator's docstring promised on its behalf. Every.unpack(...)call site acrosspcapkit/andtests/was censused (git grep -n "\.unpack(") and none passes more than the three trailing positionals (data, length, packet) or any keyword beyondlength/packet.Design chosen: given the census, forwarding (
func(cls, data, length, packet, *args[4:], **kwargs)) would only ever handSchema.unpackarguments its real signature can't accept, raisingTypeErrorfrom inside the call anyway. So I took the issue's second option: dropped*args, **kwargsfrom the documented signature, and made the wrapper raiseTypeErrorexplicitly for any leftover positional (args[4:]) or un-consumed keyword -- which also catches the duplicate-length/packetcase for free, since the leftover keyword surfaces the same way. Seedecorators.py:231-244.#458 -- zero-length raises a bare EOFError
Mechanism:
decorators.py:227-228atda2422728was unconditional --if length == 0: raise EOFError-- regardless of why length was zero. Traced where that zero comes from:pcapkit/foundation/engines/pcap.py,pcapng.py) always constructs the top-levelFrame/block with no explicitlength, so it'sNonegoing in and gets derived by measuring what's left in the underlying file (data.seek(0, SEEK_END) - current). A derived zero here means the capture is genuinely exhausted.SchemaField.unpack(pcapkit/corekit/fields/misc.py:619) callsself._schema.unpack(file, self.length, {...})with an explicit, declared length -- which can legitimately be0(e.g. an HTTP/2 frame schema sized by alengthfield that evaluates to zero, per corekit: let a nested schema's field callbacks reach the enclosing schema #457/A nested schema cannot reach the enclosing packet's fields by name, so CGA Parameters raises KeyError: 'length' #445). This is the path the issue's reproduction and provenance point at.ListField/OptionField(pcapkit/corekit/fields/collections.py:169,403,420) only call.unpack(file, length, packet)insidewhile length > 0:loops, so they never reach this withlength == 0.NoPayload(pcapkit/protocols/misc/null.py) overrides__post_init__entirely and never callsSchema.unpack, so the very common "next layer has zero bytes" case (verified empirically with a zero-payload UDP packet) never went through@prepareat all and was unaffected either way.Design chosen: distinguish "declared" (caller passed an explicit length, even
0) from "derived" (caller passedNone, and the fallback computed0from what's actually left in the stream) -- the issue's first option. Only a derived zero raises now (decorators.py:253,264). The still-fatal case raises a newpcapkit.utilities.exceptions.StreamEOFError(BaseError, EOFError)(added inexceptions.py,quiet=Trueper theStructError(..., quiet=True, eof=True)precedent atprotocol.py:983) instead of a bareEOFError, so a caller can now catch it specifically while every existingexcept (EOFError, StopIteration)(pcapkit/foundation/extraction.py:687,1006,1030) keeps working unchanged, sinceStreamEOFErroris still anEOFError.EOFError catchers censused (
git grep -n "EOFError" pcapkit/): onlypcapkit/foundation/extraction.py(threeexcept (EOFError, StopIteration)sites) and the decorator's own raise site. None of them inspect the exception's type beyond that tuple check, so subclassing is transparent to all of them.Tests
Both halves get coverage in
tests/utilities/test_decorators.py, each demonstrated failing before and passing after (captured by reverting the source files toda2422728and rerunning just these five tests):test_prepare_raises_typeerror_for_extra_positional_and_keyword_arguments-- before:AssertionError: TypeError not raised; after: passes.test_prepare_raises_typeerror_for_length_given_both_positionally_and_by_keyword-- before:AssertionError: TypeError not raised; after: passes.test_prepare_raises_stream_eof_error_not_a_bare_eof_error-- before:AttributeError: module 'pcapkit.utilities.exceptions' has no attribute 'StreamEOFError'; after: passes.test_prepare_still_raises_eof_for_a_truncated_stream-- before: sameAttributeError(module has noStreamEOFError); after: passes, confirms the frame-exhausted case is unchanged.test_prepare_accepts_a_declared_zero_length_schema-- before:EOFErrorraised atdecorators.py:228(the exactEmpty.unpack(b'', 0, {})repro from @prepare raises a bare EOFError for any zero-length schema, so an empty nested area cannot unpack #458); after: passes, returns the unpacked (empty) instance.Also independently re-verified against the real
Schema/schema_finalclasses (not just the test doubles above), reproducing the issue's ownProbe/Emptyexamples directly, and against a real zero-payload UDP packet to confirmNoPayloadis unaffected.No
EXPECTED_FAILURESentries intests/protocols/test_option_roundtrip_unit.pywere touched or need to be -- that file's own internal consistency check would fail loudly if a recorded case's status had flipped, and the full-suite failure count is identical (0) before and after, so nothing there changed status.Suite counts
Baseline at
da2422728(source and tests both reverted to that commit, fixtures generated once viaexamples/generators/make_samples.pyand reused for both runs): 994 collected, 977 passed, 17 skipped, 0 failed (655.58s).After this change: 999 collected, 982 passed, 17 skipped, 0 failed (619.50s). The delta is exactly the 5 new tests above, added and passing -- no regressions.
Run with
PYTHONSAFEPATH=1,PYTHONPATHset to the worktree, interpreter.venv/bin/python(3.14.7);pcapkit.__file__printed and confirmed to resolve into this worktree for every run.Coordination: this PR touches only
pcapkit/utilities/decorators.py,pcapkit/utilities/exceptions.py, andtests/utilities/test_decorators.py-- it does not touchpcapkit/corekit/fields/misc.py(#457),pcapkit/protocols/schema/schema.py(#456),pcapkit/protocols/internet/mh.py/examples/generators/options.py(#437), ortests/protocols/test_option_roundtrip_unit.py.