Repository navigation
fix: IPv4._make_data mis-scales fragment offset and crashes without options - #499
Merged
Merged
Conversation
…ptions Fixes #494; two docstring fixes from #490 in the same file. - IPv4._make_data returned data.offset (octets) straight to make(), which expects the on-wire 8-octet-unit value; scale back down with `// 8`, mirroring the existing, correctly-commented IPv6_Frag._make_data. - IPv4._make_data read data.options unconditionally, even though read() only sets it when hdr_len > 20; use getattr(data, 'options', None) so the ordinary no-options case no longer raises AttributeError. This defect fired first, which is why the offset bug was unreachable via a no-options packet. - IPv4._make_opt_sec docstring documented a nonexistent `sec` parameter and omitted the five real ones (level, level_default, level_namespace, level_reversed, authorities). - IPv4._make_opt_qs docstring documented `code`/`opt`, copied from the unrelated hopopt.py/ipv6_opts.py `_make_opt_qs`; real params are `kind`/`option`. - Added a regression test using a non-zero offset and a real packet with no options -- the two conditions the existing DummyDict fixture (offset 0, options always present) could not exercise. Verified with tests/protocols/internet/test_ipv4_unit.py (13 passed), test_ipv6_extension_unit.py and test_option_roundtrip_unit.py (58 passed, 436 subtests) as blast-radius checks.
This was referenced Sep 19, 2026
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.
Fixes #494. Also fixes the two
ipv4.pydocstring items from #490 — that file is the seam between the two issues, so they land together rather than conflicting.Defect 1 —
offsethanded back unscaled, an 8x inflationRFC 791 makes the wire Fragment Offset a count of 8-octet units.
read()scales it up to octets for the data model, so_make_data()must scale it back down.IPv6_Fragdoes;IPv4did not:IPv6_Frag._make_datacarries an explicit NOTE stating why the division is needed, which is the strongest evidence this was an omission rather than a decision. The fix mirrors it, comment included.Defect 2 —
.optionsread unconditionallyread()(ipv4.py:315-318) only setsoptionswhenhdr_len > 20, but_make_datareaddata.optionsunconditionally, so the ordinary no-options packet raisedAttributeError: 'IPv4' object has no attribute 'options'.Now
getattr(data, 'options', None), matching the identical pattern already used for the same optional-field problem atpcapkit/protocols/transport/tcp.py:664.Noneis not an invented sentinel — it ismake()'s own declared default foroptions, confirmed withinspect.signature.The ordering, which the original report had wrong
Defect 2 fires first. For a packet with no options the
AttributeErrorraises before_make_datacan return the unconverted offset, so defect 1 is unreachable by that route; it bites only when the packet does carry options andhdr_len > 20. Both are real, and whichever were fixed alone would expose the other — hence one change.Defect 3 — two docstrings, from #490
Both confirmed against
inspect.signaturebefore editing:_make_opt_sec(:1350) documented a phantomsecand left all five real parameters undocumented — nowlevel,level_default,level_namespace,level_reversed,authorities._make_opt_qs(:1764) documentedcode/opt; the real names arekind/option. The origin is a copy fromhopopt.py:1656/ipv6_opts.py:1668, which define their own_make_opt_qswith exactly that naming. Every other_make_opt_*in this file already usedkind/option.Verification
Measured on this branch with
pcapkit.__file__asserted against the worktree before import:And with options present, the case where defect 1 was reachable at all:
hdr_len 28,data.offset 40,_make_datanow returns5.Revert-proof. With both fixes reverted in place, the new test fails; restored, it passes:
Tests:
test_ipv4_unit.py13 passed, 16 subtests. Blast radiustest_ipv6_extension_unit.py+test_option_roundtrip_unit.py58 passed, 436 subtests.Why the new test looks the way it does
The existing
test_ipv4_make_data_preserves_selected_fieldsuses aDummyDictfixture withoffset=0and always suppliesoptions, and never assertsvalues['offset']. Zero is the one value for which the missing// 8is invisible, and supplyingoptionshides the other defect — so neither could ever have been caught. The new test uses a realmake()/read()round trip with wire offset 5 and no options, which are exactly the two conditions the old fixture avoided.Found while verifying, deliberately not fixed here
IPv4.from_data()on a parsed packet is broken independently of this change, and remains broken after it — the failure mode merely shifts. Onmainit raisesFieldValueError: Field options has invalid value; on this branch,ProtocolUnbound: unsupported type <class 'pcapkit.protocols.misc.raw.Raw'>. Both predate this PR in substance:_make_datais only one input tofrom_data, and the remaining fault is in how_make_payload/_make_ipv4_optionshandle a parsedRawpayload and a parsed options container. That is a separate defect and belongs in its own issue rather than being folded in here.