Repository navigation
Stop BitField packing every named bit as set (#359) - #374
Merged
Merged
Conversation
BitField.pre_process seeded its per-bit buffer with NUL bytes, wrote ASCII
b'0'/b'1' into it, then truth-tested the bytes to rebuild the value - and b'0'
is 0x30, which is truthy. Every named bit therefore came out set regardless of
its value: {'U': 0, 'P': 1, 'F': 0} packed to 0xe0.
Parsing was unaffected, so this corrupted construction only, silently, for
every protocol with a flags field: ipv4, ipv6 and its extension headers, tcp,
mh, hip, hopopt, l2tp, vlan, httpv2 and pcapng. IPv4 raised OverflowError
rather than mis-packing.
The buffer is now seeded with b'0' and converted directly, since it already
holds exactly the digits wanted.
Found while implementing the MH fast-handover messages, whose flag round-trips
could not pass without it.
There was a problem hiding this comment.
🟡 Changes recommended
BitField.pre_process can still grow its buffer on namespace overruns (e.g., IPv4 vihl), causing overflow/corrupt packing unless the slice end is clamped/validated and covered by a regression test.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes BitField.pre_process packing so named bits are not erroneously forced to 1 during construction, and adds a focused regression test suite for BitField packing/parsing behavior in corekit.
Changes:
- Seed the BitField packing buffer with ASCII
b'0'and convert the resulting bit-string buffer directly viaint(..., 2)instead of truth-testing bytes. - Add new
tests/corekit/test_fields_strings.pyto validate correct packing of cleared/set bits, subfield widths, unnamed bits, and pack/parse round-trips.
File summaries
| File | Description |
|---|---|
pcapkit/corekit/fields/strings.py |
Updates BitField.pre_process buffer initialization and conversion to fix incorrect “all named bits set” packing. |
tests/corekit/test_fields_strings.py |
Adds unit tests for BitField packing/parsing correctness and regression coverage. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Slice assignment on a bytearray grows it, so a namespace entry reaching past the end of the field silently produced an over-long bit buffer and then failed in int.to_bytes with an opaque OverflowError, nowhere near the declaration at fault. That is exactly what IPv4 hit: schema/internet/ipv4.py declared 'ihl': (4, 8) on a one-octet field, where IHL is four bits, so the version and IHL octet could not be packed at all. Corrected the declaration, and made BitField reject the mistake where it is made: bounds are checked when the field is constructed, since a subfield wider than its field is a schema error rather than something a packet can cause, and a value too wide for the bits it was given now raises FieldValueError instead of quietly widening the buffer. A sweep over every BitField declaration in the tree found ipv4's to be the only one out of bounds, and the whole unit tier imports and passes with the construction-time check in place: 293 passed, 71 subtests passed. Raised by Copilot's review on #374.
There was a problem hiding this comment.
🟢 Approval recommended
The fix directly addresses the documented packing defect, tightens validation to prevent silent corruption, and includes targeted regression tests covering the previously broken cases.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Draft. One-file fix plus its own tests, independent of the other open PRs.
The defect
BitField.pre_processbuilt a buffer of one ASCII digit per bit, wroteb'0'/b'1'into it, and then rebuilt the value by truth-testing each byte:b'0'is0x30, which is truthy — so every named bit came out set regardless of its value.{'U': 0, 'P': 1, 'F': 0}packed to0xe0. Only a bit whose position was never written at all (still NUL) came out zero.Parsing is unaffected:
post_processreads the packed bytes correctly. So this corrupted construction only, and silently, for every protocol with a flags field —ipv4,ipv6and its extension headers,tcp,mh,hip,hopopt,l2tp,vlan,httpv2,pcapng. IPv4 is the one exception that failed loudly, withOverflowError.The buffer already holds exactly the digits wanted, so it is now seeded with
b'0'and converted directly.Tests
tests/corekit/test_fields_strings.py(new): cleared bits staying cleared while a sibling is set, all-zero and all-set masks, multi-bit subfields keeping their own width, bits outside the namespace packing as zero, and a pack/parse round trip.Fail-before / pass-after, with the fix reverted and the tests present: 7 failed, 2 passed. With the fix: 6 passed, 4 subtests. Wider check with the fix in place:
tests/corekit+tests/protocols→ 240 passed, 7 subtests.How it surfaced
Found while implementing the MH fast-handover messages (types 8/9/10/11/14/15), whose flag round-trips cannot pass without it, and independently confirmed by the SCTP work, which gated a chunk-flags round-trip test on a runtime probe so it starts running by itself once this lands.
Closes #359.