Skip to content

IPv4 SID option: SIDOption.sid is UInt32Field where RFC 791 gives a 16-bit Stream ID, so a parsed option re-emits two octets too wide #534

Closed

Description

@JarryShaw

Split out of #506, which fixes the option-padding defect that had been masking this one. Nothing here is fixed by #506 — the asymmetry below is still present on that branch, and this issue is the record of it.

SIDOption.sid is 32 bits where RFC 791 gives 16

pcapkit/protocols/schema/internet/ipv4.py:368

#: Stream identifier.
sid: 'int' = UInt32Field()

RFC 791 §3.1 gives the Stream ID option as four octets total — one of type, one of length, and a two-octet stream identifier:

+--------+--------+--------+--------+
|10001000|00000010|    Stream ID    |
+--------+--------+--------+--------+
  Type=136 Length=4

_make_opt_sid at pcapkit/protocols/internet/ipv4.py:1637 agrees — it writes length=4. Only the schema field disagrees, so the option over-reads a well-formed one by two octets on the way in and over-writes it by two on the way out.

Measured

A well-formed four-octet option 88040037 read off the wire:

UserWarning: packet length < 0: -2      # the over-read, named by the library itself

The value survives (sid == 0x37, length == 4), so this is not a parse failure. But re-emitting it gives six octets:

proto._make_opt_sid(OptionNumber.SID, sid).pack()   # -> 880400000037, not 88040037

Six is not a multiple of four, so the option area then reaches the 32-bit padding branch and picks up a NOP and an EOOL. The rebuilt datagram is four octets longer than the one it was read from, with ihl and total length grown to match:

octets 20.. length ihl
read off the wire 88040037 24 6
IPv4.from_data re-emits 8804000000370100 28 7

Why this is not an EXPECTED_FAILURES entry

tests/protocols/test_option_roundtrip_unit.py carried ipv4-option/SID as a Gap before #506, but the entry recorded the padding failure, not this. With the padding fixed the generator's cycle closes and the case reports 'OK' — it constructs and reconstructs through the same _make_opt_sid, so both halves emit the same six octets and match each other. A Gap for it would have to record 'OK' as a failure status, which is the one value test_round_trip_is_identity_or_a_recorded_gap reads as "no entry needed".

So #506 pins it as an assertion instead: OptionRoundTripTests.test_a_parsed_sid_option_re_emits_two_octets_too_wide, which starts from wire octets rather than from the generator's case, because it is only against the wire that the width shows.

The fix

Narrow the field to UInt16Field. Measured on a scratch tree: the option re-emits as 88040037, the datagram stays 24 octets, and the padding branch is not reached at all — which makes every assertion in that pinning test wrong at once, so the test is what turns red when this is fixed and deleting it is how the fix gets recorded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions