Repository navigation
fix(protocols): makers name the rejected argument and value in error messages - #2405
Merged
Merged
Conversation
JarryShaw
added
bug
Owner
Author
|
Supersedes #2404 for #2296 — same scope, but that PR's cross-review found a wrong-requirement message, under-tested guards, and an overclaim about coverage (see the comment there). This one pins every site individually and stays within the issue's own checklist. Thanks to @Qiu-LingYan for the first pass at this — if the fixes above land there instead, I'll close this one. |
…messages
Several _make_* builders rejected a bad caller-supplied argument with the
reader's parse-error text ("invalid format"), which gives no indication of
which argument was wrong or what value triggered it. #2296.
- sctp.py: all 11 identical `len(flags) != 1` guards across the chunk
makers, plus _make_chunk_data's empty-data guard, _make_param_hostname's
NUL-termination guard, and _make_cause_invalid_stream's reserved-length
guard.
- mh.py: make()'s unsupported-data-type guard, _make_opt_bad/_make_opt_auth's
length guards, _make_opt_badf's empty-data guard, and _make_opt_pad's
Pad1-length warning. Added the missing Raises:/Warns: docstring sections.
- tcp.py: _make_mptcp_capable now names which optional field was given
without its predecessor.
- hip.py: _make_locator's fallback branch (the one site #2276 missed).
- pcapng.py: _make_block_dsb's fallback branch (the one site #2273 missed).
_make_chunk_data's flags= example in the issue doesn't match any real
parameter on that method (it takes reserved/I/U/B/E, not flags), so its
exact wording was applied to the 11 makers that do take flags, and
_make_chunk_data got a data-shaped message instead.
Test: tests/protocols/test_maker_invalid_format_messages_2296_unit.py (new,
22 tests/82 subtests, one class per site group); confirmed 66 failed/20
passed against unpatched pcapkit, each failure a message-text mismatch.
tests/protocols/ in full: 2011 passed/51370 subtests/0 failed. mypy and
pylint findings on the 5 touched files are unchanged from main.
Four other maker-side sites still use the generic text but aren't in
#2296's checklist -- HOPOPT._make_opt_pad, IPv6_Opts._make_opt_pad,
HTTP.make, PCAPNG.make -- tracked separately rather than folded in here.
JarryShaw
force-pushed
the
fix-2296-maker-invalid-format
branch
from
October 28, 2026 03:30
e11cdb9 to
d0fd8cd
Compare
Owner
Author
|
Review: GOOD TO GO at |
JarryShaw
added
review: good-to-go
Contributor
|
Coverage: 91.72% (unit tier, Python 3.15,
Per-file detail: the |
JarryShaw
removed
the
review: good-to-go
5 tasks done
JarryShaw
added a commit
that referenced
this pull request
Oct 28, 2026
Follow-up to #2405, which left these out of #2296's checklist. #2406. - hopopt.py / ipv6_opts.py: _make_opt_pad's Pad1-length warning, matching MH._make_opt_pad's text from #2405 (alias swapped). - httpv2.py: HTTP.make's unsupported-frame-type guard. - pcapng.py: PCAPNG.make's unsupported-block-type guard (the dispatcher, not _make_block_dsb, which #2405 already fixed). Added the missing Raises: docstring section on the two make() guards, matching #2405's MH.make/_make_block_dsb docstrings. Test: tests/protocols/test_maker_invalid_format_messages_2406_unit.py (new, 9 tests/18 subtests), plus a ReadersKeepInvalidFormat pin that _read_opt_pad on both protocols still says "invalid format" -- the exact twin of the maker guard changed here. Confirmed fail-without-fix: 12 failures against unpatched pcapkit, each a message-text mismatch. tests/protocols/ in full: 2020 passed/51388 subtests/0 failed. mypy and pylint findings on the 4 touched files are unchanged from main.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
make testpasses, and a test case covers the changeWhat is the purpose of your pull request?
fix — bug fixDescription
Fixes #2296.
_make_*builders that reject a bad caller-supplied argument reported the reader's parse-error text (invalid format), giving no indication of which argument was wrong or what value triggered it.Covers every site named in the issue: all 11 identical
flagsguards plus_make_chunk_data/_make_param_hostname/_make_cause_invalid_streaminsctp.py;make/_make_opt_bad/_make_opt_auth/_make_opt_badf/_make_opt_padinmh.py(also added the missingRaises:/Warns:docstring sections);_make_mptcp_capableintcp.py; and the one surviving generic-message branch each inhip.py's_make_locatorandpcapng.py's_make_block_dsb(the branches #2276/#2273 didn't touch).Note:
#2296's own repro calls_make_chunk_data(Chunk(0), flags=b'\x00\x00'), but that method has noflagsparameter (it takesreserved/I/U/B/E) — the issue's exact wording was applied to the 11 makers that actually takeflags, and_make_chunk_datagot adata-shaped message instead.Four other maker-side sites keep the generic text but weren't in #2296's checklist —
HOPOPT._make_opt_pad,IPv6_Opts._make_opt_pad,HTTP.make,PCAPNG.make(the dispatcher, not_make_block_dsb) — tracked as a follow-up in #2406 rather than folded in here.Test:
tests/protocols/test_maker_invalid_format_messages_2296_unit.py(new, 22 tests/82 subtests, one class per site group, each pinning the full message text plus the maker's valid-input case; independently confirmed each of the 22 changed sites is caught individually by reverting it alone). Confirmed fail-without-fix: 66 failed/20 passed against unpatchedpcapkit.tests/protocols/in full: 0 failures, same skip baseline asmain. mypy and pylint findings on the 5 touched files are unchanged frommain.