Repository navigation
IPv4._make_data: fragment offset returned unscaled (8x), and .options read unconditionally #494
Description
Found by strand 5 of the post-wave-1 consistency sweep (docs/source/pep.rst) — duplicated logic. Both defects are in IPv4._make_data, the round-trip path behind from_data() and every _make_payload call, and both were reproduced by me on 5959a437c.
This is the same shape as #483: one member of a family of sibling implementations omits a unit conversion that all the others perform.
1. offset is handed back in octets to a parameter that wants 8-octet units
RFC 791 makes the wire Fragment Offset field a count of 8-octet units. read() scales it up to octets for the data model; _make_data() must scale it back down. IPv6_Frag does. IPv4 does not:
IPv4.read: offset=int(schema.flags['offset']) * 8
IPv6_Frag.read: offset=int(schema.flags['offset']) * 8
IPv6_Frag._make_data: 'offset': data.offset // 8,
IPv4._make_data: 'offset': data.offset, <- no // 8
pcapkit/protocols/internet/ipv4.py:307 (read) and :529 (_make_data); pcapkit/protocols/internet/ipv6_frag.py:147 and :266.
IPv6_Frag._make_data carries an explicit note saying exactly why the division is needed — which is the strongest available evidence that its absence next door is an omission rather than a decision:
# NOTE: ``make`` takes ``offset`` in on-wire 8-octet units, while
# ``data.offset`` is in octets (see :meth:`read`), so scale back down to
'offset': data.offset // 8,Measured:
wire offset field (8-octet units): 5
after read(), data.offset (octets): 40
_make_data kwargs['offset'] handed back to make(): 40 <- should be 5
The schema packs that 40 straight into the same 13-bit field, so the value inflates 8x per round trip.
2. _make_data unconditionally reads data.options, which does not exist without IPv4 options
read() at ipv4.py:315-318 only calls ipv4.__update__([('options', ...)]) when hdr_len > 20. _make_data reads data.options unconditionally. So the ordinary no-options case raises:
raw = proto.make(offset=5, protocol=6, payload=b'\xaa'*8).pack()
d1 = IPv4(io.BytesIO(raw), len(raw)).info
proto._make_data(d1)AttributeError: 'IPv4' object has no attribute 'options'
How the two interact — a correction to how this was first reported
The sweep described defect 1 as costing "an 8x inflation on every fragmented-IPv4 round trip through _make_data/from_data". That overstates the current reach, and the two defects are the reason: defect 2 fires first. For a packet with no IPv4 options — the common case — _make_data raises AttributeError before it can return the unconverted offset, so defect 1 is unreachable by that route.
Defect 1 therefore bites only when the packet does carry options, putting hdr_len above 20 so that read() populates .options and _make_data gets far enough to return. Both are real; the ordering means whichever is fixed first exposes the other, so they want fixing together.
I hit a third thing while building the options case, filed separately: a Router Alert option carrying RFC 2113's only defined value (0) cannot be parsed at all, because RouterAlert(0) raises.
Existing coverage cannot catch either
tests/protocols/internet/test_ipv4_unit.py::test_ipv4_make_data_preserves_selected_fields uses a DummyDict fixture with offset=0 and never asserts values['offset']. Zero is the single value for which the missing // 8 is invisible, and the fixture supplies options, so the AttributeError never arises either.
Suggested shape
Add the // 8 at ipv4.py:529 and default options when absent. Since the fix is one half of a conversion pair that already exists correctly in ipv6_frag.py, the strand's own recommendation applies: a shared scale-to-octets / scale-from-octets pair, so the two directions cannot drift apart again. That is the mechanism behind #487 (the same quantity computed in four places, with four different wrong answers) and #483 (one sibling adapter unscaling where four scaled).
A regression test should use a non-zero offset and a packet without options, since those are precisely the two conditions the current fixture avoids.
Metadata
Metadata
Assignees
Labels
Projects
- StatusShow more project fieldsDone
Activity
fix(protocols): let from_data rebuild a parsed packet (#536)