fix(ipv4): three defects in the IPv4 option schemas (#552) - #559
Merged
Merged
Conversation
- `TSOption.post_process` converted `ts_data` entries to addresses with a bare
`ipaddress.ip_address`, which takes a `bool` as the `int` it subclasses, so
`ts_data=[True, 5]` packed and reported `IPv4Address('0.0.0.1')` with no
exception. It runs on the packing path too, and `IPv4.make` accepts a
caller-built option schema, so this was reachable from the public API. All
three conversions now go through `parse_ip_address`, the fifth site of the
defect #481, #500, #539 and #540 fixed before it.
- `quick_start_data_selector` sized the nested Quick-Start suboption with a
hardcoded `SchemaField(length=5)` -- the width of a Request's `ttl` and
`nonce` alone. A well-formed 8-octet option decoded its nonce as 55 rather
than 933982136 and left three octets to be read as a fabricated option, so
the datagram failed with `ProtocolError`. The length now comes from
`quick_start_option_length`, computed from the resolved suboption, and
`QuickStartReportOption` gains the RFC 4782 section 3.1 `Not Used` octet it
was missing, which had made it seven octets wide against the `length=8` both
`_make_opt_qs` and `_read_opt_qs` use.
- `_make_opt_ts` passed `data=` where the schema field is `ts_data`, so every
timestamp was dropped with an `UnknownFieldWarning` and the Timestamp option
was unbuildable through `make`. The `TYPE_CHECKING` `__init__` stub that
advertised `data` is corrected too.
Three new tests, each shown to fail without its fix; `ipv4-option/TS` deleted
from `EXPECTED_FAILURES` now that it round-trips. Full unit tier green, 1107
passed with 2666 subtests; both changed modules at 100% statement and branch
coverage.
JarryShaw
force-pushed
the
fix/552-ipv4-option-schemas
branch
from
September 20, 2026 21:08
ecac29c to
1a92f00
Compare
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.
Closes #552.
Three defects in the IPv4 option schemas, grouped because they share
pcapkit/protocols/schema/internet/ipv4.py. Every measurement below was taken onthis machine against
mainate7e9ba98f, withpcapkit.__file__asserted to bethis tree.
1.
TSOption.post_processlaundered aboolinto an addresspost_processconverted thets_dataentries that the timestamp flag makesaddresses with a bare
ipaddress.ip_address, which accepts anyintbelow2**32— andboolis anintsubclass. Measured before the fix:No exception and no warning. Nothing downstream could have caught it:
ts_data'sitem type is a
UInt32Field, which packsTrueas the1it is, so #500's guardin
_IPAddressField.pre_processis never reached. Andpost_processruns on thepacking path as well as the unpacking one, so the entries it converts are
whatever the caller passed — reachable through public
IPv4.make, which takes acaller-built option schema and packs it.
This is the fifth site of the defect #481, #500, #539 and #540 fixed before it, so
the fix is #539's: route all three conversions in the method through
parse_ip_address. After:The third conversion — the one that reads the prespecified addresses out of the
option's padding,
ipv4.py:308— is included even though #540 and #552 both judgedit unable to launder a
bool, which it cannot:remainderis aPaddingField, soit holds octets. It is routed through anyway because a bare
ipaddress.ip_addressthere still raises a plain
ValueErrorfor a tail that is not a whole number of8-octet pairs, which no
except BaseErrorcan catch — and because leaving one ofthree conversions in one method unguarded is exactly how this became a fifth site.
2. Quick-Start was sized with a hardcoded
SchemaField(length=5)Five is the width of a Quick-Start Request's
ttlandnoncealone; thetype,lengthandflagsoctets the suboption schema re-declares were unaccounted for.RFC 4782 §19.1 and §19.2 both give the option as eight octets, and
_read_opt_qsrejects any other
lengthoutright. Measured on a well-formed 8-octet option:The three unconsumed octets were read as a further, fabricated option, which is
what made the enclosing datagram unparseable. Silent corruption on the way to a
misleading failure.
The length now comes from
quick_start_option_length(schema), summed from thefields of the suboption the selector just resolved rather than written as a
literal: the registry is open —
QSOptionis anEnumSchema, so a caller mayregister a further function code — and a literal has to be kept in step by hand
with every field the suboptions declare, which is how the
5came to be wrong inthe first place. The helper refuses rather than guesses for a field whose width is
not fixed, and both shapes of that are covered by tests: a length callback
(
LSROption.route) and aSwitchField, which has none and reports zero until itsown selector resolves it (
_QSOption.data).QuickStartReportOptionwas also one octet short, missing theNot UsedoctetRFC 4782 §19.2 puts where a Request has
QS TTL— the diagram in this repo's own_read_opt_qsdocstring shows it,Length=8. It therefore packed seven octetsagainst the
length=8that_make_opt_qswrites and_read_opt_qsdemands, so aspec-correct Report read off the wire decoded its nonce one octet early. Fixing
only the selector would have baked that seven into the reader, so the missing octet
is declared here as a
PaddingField. Both functions now measure 8 and bothround-trip:
3.
_make_opt_tspassed a keyword the schema does not havedata=ts_listwhere the field ists_data;datais the attributepost_processderives.Schema.__update__answers an unknown field name with anUnknownFieldWarningand carries on, so every timestamp was dropped in silence andts_datastayed bound to its class-levelListField, whichpost_processthentried to iterate —
TypeError: 'ListField' object is not iterable. The IPv4Timestamp option was unbuildable through
make.The
TYPE_CHECKING__init__stub onTSOptionis corrected too: it advertiseddata: 'list[int]'as a constructor argument, which is what the maker was writtenagainst.
Coverage
Three new tests in
tests/protocols/internet/test_ipv4_unit.py, each shown to failwithout its fix by reverting exactly that hunk, reading pytest's exit code from a
file:
test_ipv4_timestamp_option_refuses_a_bool_as_an_addressAssertionError: FieldValueError not raisedon the publicIPv4.makepath, plus four failing subteststest_ipv4_quick_start_option_is_eight_octets_wide_on_the_wireAssertionError: 7 != 8test_ipv4_timestamp_option_is_buildable_through_makeTypeError: 'ListField' object is not iterableipv4-option/TSis deleted fromEXPECTED_FAILURES— it round-trips now, and thetable's own rule is that a fixed defect loses its entry rather than keeping a stale
one. The table goes from 55 entries to 54,
ipv4-option/TSremoved and nothingadded; measured by importing the dict out of each ref's own copy of the module and
taking
len(), since the**comprehensions contribute 30 of the 55 keys and nogrep can see them.
ipv4-option/QSstays, because the unrelatedfuncdefect atipv4.py:1178fails first; itsdefectstring drops the half this PR fixed. Fourstale
file:linereferences in the neighbouring Quick-Start entries are correctedagainst the current tree (
hopopt.py:869→918,ipv6_opts.py:881→921, and bothschema modules'
224→255), since a wrong line in that table "reads asdocumentation of a defect nobody can find".
Full unit tier: 1107 passed, 8 skipped, 2666 subtests passed, exit 0. Both
changed modules under
coverage run -m pytest:schema/internet/ipv4.py182 statements / 30 branches, 100% (was 172/24 at100%, so every added statement and branch is exercised);
internet/ipv4.py471 statements, 0 missed, one pre-existing partial branch.mypyclean,isort --checkclean,pylintreports nothing new.Not fixed here, and worth its own issues
SchemaField(length=5)is still atpcapkit/protocols/schema/internet/hopopt.py:255and.../ipv6_opts.py:255, with the same measured nonce of 55. IPv4 option schemas: a fifth bool-laundering site, a hardcoded Quick-Start length, and a dropped ts_data keyword #552 was scoped toIPv4's copy;
quick_start_option_lengthis the shape a fix for those wants._make_opt_qsreturns a bare nested schema whosefuncis set only by_QSOption.post_process, which runs on the parse path — soIPv4.makewith aQuick-Start option still raises
AttributeError: 'QuickStartRequestOption' object has no attribute 'func', in all three protocols. This is why the QSEXPECTED_FAILURESentries remain.UnknownFieldWarningshould be fatal duringmake. Items 1 and 3here were silent for exactly that reason, and so is
hip.py'scipher=(Two dropped-keyword/wrong-cast defects flagged in review and never filed (hip.py:3533, ipv6_route.py:207) #556).Measured by instrumenting
Schema.__update__over all 322 cases of the optionsweep: 16 distinct (schema, keyword) pairs reach it. Two are defects —
TSOption.data(this PR) andEncryptedParameter.cipher. The other 14 are sevenMPTCP nested schemas passing
kindandlength(seven schemas × two keywords),which
pcapkit/protocols/schema/schema.py:95-104documents as deliberate:__init__forwards
**kwargsspecifically so those keep reaching__update__rather thanraising
TypeError. Statically there are 11 such_make_mptcp_*constructors inpcapkit/protocols/transport/tcp.py, i.e. 22 keywords. So making it fatal wouldcatch all three defects and break 11 call sites that are working as designed —
worth its own issue, and the issue has to convert those sites first.