fix(mh): count a CGA extension's 4-octet header in its reported length

pcapkit/protocols/internet/ipv6_opts.py

@@ -464,7 +464,7 @@ def _read_opt_type(self, kind: 'int') -> 'tuple[int, bool]':
     def _ipv6_opts_option_length(schema_len: 'int') -> 'int':
         """Compute an IPv6-Opts option's whole-option length from its on-the-wire ``Opt Data Len``.
 
-        Per :rfc:`8200#section-4.3`, an option's ``Opt Data Len`` field
+        Per :rfc:`8200#section-4.2`, an option's ``Opt Data Len`` field
         (what each ``Schema_*Option.len`` here holds) counts *"the length of
         the Option Data field of this option, in octets"* -- i.e. it
         excludes the Option Type and Opt Data Len fields themselves, so the
@@ -477,6 +477,15 @@ def _ipv6_opts_option_length(schema_len: 'int') -> 'int':
         arithmetic only has to happen once. Do NOT drop the ``+ 2``: that is
         precisely the mismatch #398 fixed.
 
+        Section 4.2 is the citation because it is what defines the TLV option
+        format, and it is where the sentence quoted above actually appears.
+        IPv6-Opts itself is the Destination Options header of
+        :rfc:`8200#section-4.6`, which carries those TLVs but says nothing
+        about their internal length arithmetic. This cited
+        :rfc:`8200#section-4.3` until #517 -- that is the Hop-by-Hop Options
+        header, which is a different header and not the one this class
+        implements.
+
         Note that only the *stored-length* read-side call sites are
         collected here -- most ``_make_opt_*`` methods recompute the wire
         ``Opt Data Len`` from ``len(value)`` rather than reading a parsed

pcapkit/protocols/internet/mh.py

@@ -2725,12 +2725,21 @@ def _mh_option_length(schema_length: 'int') -> 'int':
         Length fields"* -- so the whole option, which is what every
         ``_read_opt_*`` below reports back as the parsed option's own
         ``.length``, is two octets more. This is the exact ``+2``/``-2``
-        mismatch #398 fixed independently in six places (see
-        ``Data_PadOption.length`` vs. ``Schema_PadOption.length`` below, at
-        the surviving explanation of that fix); collecting the read-side
-        half of it into one helper is so a future fix to this arithmetic
-        only has to happen once. Do NOT drop the ``+ 2``: that is precisely
-        the mismatch #398 fixed.
+        mismatch #398 fixed independently in six places (see the ``Note:``
+        on :meth:`_read_opt_pad` below, which explains why a ``Pad1``
+        option -- the one option with no ``Option Length`` field at all --
+        is this helper's sole exception); collecting the read-side half of
+        it into one helper is so a future fix to this arithmetic only has
+        to happen once. Do NOT drop the ``+ 2``: that is precisely the
+        mismatch #398 fixed.
+
+        The ``+ 2`` is specific to an :rfc:`6275#section-6.2` mobility
+        option, whose Option Type and Option Length are one octet each. It
+        is **not** a universal contract for everything this class parses:
+        a CGA extension's Extension Type and Extension Data Length are two
+        octets each [:rfc:`4581#section-2`], so those readers use
+        :meth:`_mh_extension_length` instead. Reusing this helper for them
+        reported every parsed CGA extension two octets short (#512).
 
         Note that only the *stored-length* read-side call sites are
         collected here -- most ``_make_opt_*`` methods recompute the wire
@@ -2741,7 +2750,8 @@ def _mh_option_length(schema_length: 'int') -> 'int':
             schema_length: raw ``Option Length`` field value, as read off the wire.
 
         Returns:
-            Whole-option length, in octets, including the Type and Length fields.
+            Whole-option length, in octets, including the Option Type and
+            Option Length fields.
 
         """
         return schema_length + 2
@@ -2837,6 +2847,18 @@ def _read_opt_pad(self, schema: 'Schema_PadOption', *,
         Returns:
             Constructed option data.
 
+        Note:
+            A ``Pad1`` option occupies a single octet and carries no
+            ``Option Length`` field, so its
+            :attr:`~pcapkit.protocols.data.internet.mh.PadOption.length` is
+            ``1`` rather than ``length + 2``. That one-octet wire shape is
+            enforced by
+            :class:`~pcapkit.protocols.schema.internet.mh.PadOption` itself,
+            which sizes both the length octet and the padding data from the
+            option type [:rfc:`6275#section-6.2.5`]; ``clen`` is therefore
+            always ``0`` here for a parsed ``Pad1``, and the check below only
+            guards a schema built by hand.
+
         """
         code, clen = schema.type, schema.length
 
@@ -2850,7 +2872,7 @@ def _read_opt_pad(self, schema: 'Schema_PadOption', *,
         if code == Enum_Option.Pad1:
             size = 1
         else:
-            size = clen + 2
+            size = self._mh_option_length(clen)
 
         data = Data_PadOption(
             type=schema.type,
@@ -6179,6 +6201,40 @@ def _read_opt_dlif_lladdr(self, schema: 'Schema_DLIFLinkLayerAddressOption', *,
         )
         return data
 
+    @staticmethod
+    def _mh_extension_length(schema_length: 'int') -> 'int':
+        """Compute a CGA extension's whole-structure length from its on-the-wire ``Extension Data Length``.
+
+        This is the CGA-extension counterpart of :meth:`_mh_option_length`, and
+        it deliberately adds ``4`` rather than ``2``. A CGA extension is **not**
+        an :rfc:`6275#section-6.2` mobility option: per :rfc:`4581#section-2`,
+        which defines the TLV format and formally updates :rfc:`3972`, its
+        ``Extension Type`` is a *"16-bit identifier of the type of the Extension
+        Field"* and its ``Extension Data Length`` a *"16-bit unsigned integer.
+        Length of the Extension Data field of this option, in octets"*. So the
+        fixed header is two 2-octet fields, not two 1-octet ones, and the whole
+        structure is ``4`` octets more than the stored length -- a fact
+        :rfc:`5535#section-5` states from the other direction for the one
+        non-experimental assigned type, whose ``Ext Len`` is the *"[l]ength of
+        the Extension in octets, not including the first 4 octets"*.
+
+        Passing these lengths through :meth:`_mh_option_length` reported every
+        parsed CGA extension two octets short (#512): an 8-octet extension with
+        an ``Extension Data Length`` of ``4`` came back as ``6``. Note
+        :meth:`_make_cga_extensions` has always measured ``len(schema.pack())``
+        instead, so the write side was already right and only the read side
+        disagreed with the wire.
+
+        Args:
+            schema_length: raw ``Extension Data Length`` field value, as read off the wire.
+
+        Returns:
+            Whole-extension length, in octets, including the Extension Type and
+            Extension Data Length fields.
+
+        """
+        return schema_length + 4
+
     def _read_cga_extensions(self, extensions_schema: 'list[Schema_CGAExtension]') -> 'Extension':
         """Read CGA extensions.
 
@@ -6236,7 +6292,7 @@ def _read_ext_none(self, schema: 'Schema_UnknownExtension', *,
         """
         data = Data_UnknownExtension(
             type=schema.type,
-            length=self._mh_option_length(schema.length),
+            length=self._mh_extension_length(schema.length),
             data=schema.data,
         )
         return data
@@ -6283,7 +6339,7 @@ def _read_ext_multiprefix(self, schema: 'Schema_MultiPrefixExtension', *,
         """
         data = Data_MultiPrefixExtension(
             type=schema.type,
-            length=self._mh_option_length(schema.length),
+            length=self._mh_extension_length(schema.length),
             flag=bool(schema.flags['P']),
             prefixes=tuple(schema.prefixes),
         )
@@ -6328,7 +6384,7 @@ def _read_ext_exp(self, schema: 'Schema_ExperimentalExtension', *,
         """
         data = Data_ExperimentalExtension(
             type=schema.type,
-            length=self._mh_option_length(schema.length),
+            length=self._mh_extension_length(schema.length),
             data=schema.data,
         )
         return data

tests/protocols/internet/test_mh_unit.py

@@ -1939,6 +1939,14 @@ def test_mh_experimental_cga_extensions_round_trip(self) -> None:
         The RFC assigns the codepoints and gives their extension data no structure
         at all, so an opaque payload is the whole of the correct parse rather than
         a placeholder for a better one. All three share a handler.
+
+        The ``data.length`` assertion below read ``len(payload) + 2`` until #512.
+        That contradicted the ``len(schema.pack())`` assertion four lines above it
+        -- the same test proved the wire was ``len(payload) + 4`` and then asserted
+        the parsed length was two octets less. The ``+ 4`` is the wire's own count:
+        a CGA extension's Extension Type and Extension Data Length are two octets
+        each [:rfc:`4581#section-2`], not one each as an :rfc:`6275#section-6.2`
+        mobility option's are.
         """
         from pcapkit.const.mh.cga_extension import CGAExtension
         from pcapkit.protocols.internet.mh import MH
@@ -1954,12 +1962,169 @@ def test_mh_experimental_cga_extensions_round_trip(self) -> None:
 
                     data = proto._read_ext_exp(schema, extensions=None)  # type: ignore[arg-type]
                     self.assertEqual(data.type, code)
-                    self.assertEqual(data.length, len(payload) + 2)
+                    self.assertEqual(data.length, len(payload) + 4)
+                    self.assertEqual(data.length, len(schema.pack()))
                     self.assertEqual(data.data, payload)
 
                     again = proto._make_ext_exp(code, data)  # type: ignore[arg-type]
                     self.assertEqual(again.pack(), schema.pack())
 
+    def test_mh_cga_extension_length_counts_the_four_octet_header(self) -> None:
+        """Every CGA extension reader must report the octets actually on the wire (#512).
+
+        All three readers -- :meth:`~pcapkit.protocols.internet.mh.MH._read_ext_none`,
+        :meth:`~pcapkit.protocols.internet.mh.MH._read_ext_multiprefix` and
+        :meth:`~pcapkit.protocols.internet.mh.MH._read_ext_exp` -- passed their
+        ``Extension Data Length`` through
+        :meth:`~pcapkit.protocols.internet.mh.MH._mh_option_length`, which adds the
+        ``2`` octets an :rfc:`6275#section-6.2` mobility option's Option Type and
+        Option Length occupy. A CGA extension's header is twice that: per
+        :rfc:`4581#section-2` the ``Extension Type`` is a *"16-bit identifier"* and
+        the ``Extension Data Length`` a *"16-bit unsigned integer"* counting
+        *"[t]he length of the Extension Data field of this option, in octets"*.
+        :rfc:`5535#section-5` says the same from the other side: its ``Ext Len`` is
+        the length *"not including the first 4 octets"*.
+
+        So every parsed extension reported a length two octets short of what it
+        consumed. ``len(schema.pack())`` is the arbiter here -- the write side has
+        always measured the real thing, which is why only the read side was wrong.
+        """
+        from pcapkit.const.mh.cga_extension import CGAExtension
+        from pcapkit.protocols.internet.mh import MH
+        from pcapkit.protocols.schema.internet import mh as schema
+
+        proto = object.__new__(MH)
+
+        # the exact measurement from #512: 8 octets on the wire, Extension Data
+        # Length 4. Those type octets are 0x0012, so a dispatched read would reach
+        # _read_ext_multiprefix; the unknown reader is called directly here because
+        # it is the reader the issue measured.
+        wire = bytes.fromhex('00120004deadbeef')
+        self.assertEqual(len(wire), 8)
+        unknown = schema.UnknownExtension.unpack(wire)
+        self.assertEqual(unknown.length, 4)
+        self.assertEqual(len(unknown.pack()), 8)
+        self.assertEqual(
+            proto._read_ext_none(unknown, extensions=None).length, 8)  # type: ignore[arg-type]
+
+        cases = (
+            ('unknown', proto._read_ext_none,
+             lambda n: schema.UnknownExtension(
+                 type=CGAExtension.get(0x0001), length=n, data=bytes(n))),
+            ('experimental', proto._read_ext_exp,
+             lambda n: schema.ExperimentalExtension(
+                 type=CGAExtension.Exp_FFFF, length=n, data=bytes(n))),
+        )
+        for name, reader, build in cases:
+            for size in (0, 1, 4, 16, 255):
+                with self.subTest(extension=name, data=size):
+                    built = build(size)
+                    packed = built.pack()
+                    self.assertEqual(len(packed), size + 4)
+
+                    # the reported length is the octet count, not the data count
+                    parsed = reader(built, extensions=None)  # type: ignore[arg-type]
+                    self.assertEqual(parsed.length, len(packed))
+                    self.assertEqual(parsed.length, size + 4)
+
+                    # and it survives a real unpack of those same octets
+                    reparsed = reader(type(built).unpack(packed),  # type: ignore[arg-type]
+                                      extensions=None)
+                    self.assertEqual(reparsed.length, len(packed))
+
+        # multi-prefix carries a 4-octet flag word plus 8 octets per prefix
+        for count in (0, 1, 2, 5):
+            with self.subTest(extension='multiprefix', prefixes=count):
+                built = proto._make_ext_multiprefix(  # type: ignore[arg-type]
+                    CGAExtension.Multi_Prefix, flag=True, prefixes=list(range(count)))
+                packed = built.pack()
+                self.assertEqual(built.length, 4 + count * 8)
+                self.assertEqual(len(packed), built.length + 4)
+
+                parsed = proto._read_ext_multiprefix(built, extensions=None)  # type: ignore[arg-type]
+                self.assertEqual(parsed.length, len(packed))
+
+        # the dispatching entry point agrees too, for every registered reader
+        extensions = proto._read_cga_extensions([
+            schema.UnknownExtension(type=CGAExtension.get(0x0001), length=2, data=b'xx'),
+            schema.ExperimentalExtension(type=CGAExtension.Exp_FFFD, length=3, data=b'yyy'),
+            proto._make_ext_multiprefix(  # type: ignore[arg-type]
+                CGAExtension.Multi_Prefix, flag=False, prefixes=[7]),
+        ])
+        self.assertEqual(
+            [ext.length for ext in extensions.values()],
+            [2 + 4, 3 + 4, 12 + 4],
+        )
+
+    def test_mh_option_and_extension_length_helpers_do_not_share_a_header_width(self) -> None:
+        """The two wire-unit helpers are distinct because the two headers are (#512).
+
+        :meth:`~pcapkit.protocols.internet.mh.MH._mh_option_length` is correct and
+        stays correct -- an :rfc:`6275#section-6.2` mobility option's Option Type and
+        Option Length are one octet each. What was wrong was reusing it for CGA
+        extensions, whose two header fields are 16 bits each
+        [:rfc:`4581#section-2`]. This pins both contracts side by side so the next
+        reader picks the right one, and pins the ``+2`` against being "fixed" to
+        ``+4`` to suit the three callers that were the actual defect.
+        """
+        from pcapkit.protocols.internet.mh import MH
+
+        # the numbers quoted in #512
+        self.assertEqual(MH._mh_option_length(4), 6)
+        self.assertEqual(MH._mh_extension_length(4), 8)
+
+        for stored in (0, 1, 2, 4, 12, 16, 255):
+            with self.subTest(stored=stored):
+                self.assertEqual(MH._mh_option_length(stored), stored + 2)
+                self.assertEqual(MH._mh_extension_length(stored), stored + 4)
+                self.assertEqual(
+                    MH._mh_extension_length(stored) - MH._mh_option_length(stored), 2)
+
+        # a real mobility option still round-trips through the 2-octet helper
+        from pcapkit.protocols.schema.internet import mh as schema
+        option = schema.BindingRefreshAdviceOption.unpack(bytes.fromhex('02020064'))
+        self.assertEqual(MH._mh_option_length(option.length), 4)
+        self.assertEqual(MH._mh_option_length(option.length), len(option.pack()))
+
+    def test_mh_pad_option_reader_delegates_to_the_option_length_helper(self) -> None:
+        """``_read_opt_pad`` must route through the helper, not open-code ``clen + 2`` (#517).
+
+        #509 extracted
+        :meth:`~pcapkit.protocols.internet.mh.MH._mh_option_length` so that "a future
+        fix to this arithmetic only has to happen once", and converted the equivalent
+        branch in both ``hopopt.py`` and ``ipv6_opts.py``. The ``mh.py`` branch was
+        left open-coding ``clen + 2``, so the arithmetic still existed twice in the
+        very file that introduced the helper.
+
+        A value assertion cannot catch that -- ``clen + 2`` and the helper return the
+        same number, which is why the conversion is behaviour-neutral and why
+        ``test_mh_padding_options_parse_from_the_wire`` passed throughout. What is
+        testable is the *delegation*: patch the helper and a converted reader follows
+        it, while an open-coded one ignores it.
+        """
+        from pcapkit.const.mh.option import Option
+        from pcapkit.protocols.internet.mh import MH
+        from pcapkit.protocols.schema.internet import mh as schema
+
+        proto = object.__new__(MH)
+        padn = schema.PadOption(type=Option.PadN, length=5)
+
+        # unpatched: the helper's real answer
+        self.assertEqual(proto._read_opt_pad(padn, options=None).length, 7)  # type: ignore[arg-type]
+
+        # patched: a converted reader reports the sentinel, an open-coded one says 7
+        with mock.patch.object(MH, '_mh_option_length',
+                               staticmethod(lambda n: 1000 + n)):
+            self.assertEqual(
+                proto._read_opt_pad(padn, options=None).length, 1005)  # type: ignore[arg-type]
+
+        # Pad1 is the documented exception: one octet, and it must not call the helper
+        pad1 = schema.PadOption(type=Option.Pad1, length=0)
+        with mock.patch.object(MH, '_mh_option_length',
+                               staticmethod(lambda n: 1000 + n)):
+            self.assertEqual(
+                proto._read_opt_pad(pad1, options=None).length, 1)  # type: ignore[arg-type]
+
     def test_mh_pmipv6_options_round_trip_byte_for_byte(self) -> None:
         """Every mobility option this module decodes must survive a round trip.