fix(protocols)!: pad two octets with one PadN, not two Pad1 (#1793)

pcapkit/protocols/internet/hopopt.py

@@ -1356,20 +1356,19 @@ def _make_pad_options(self, offset: 'int') -> 'tuple[list[Schema_PadOption], int
             A ``PadN`` option spends two octets on its own type and
             ``Opt Data Len`` fields before any padding data, so occupying
             ``pad_len`` octets means an ``Opt Data Len`` of ``pad_len - 2``.
-            One or two octets are padded with ``Pad1`` options instead, that
-            being the only form which can occupy a single octet
-            [:rfc:`8200#section-4.2`]; two octets need two separate ``Pad1``
-            schemas, since a schema instance is mutable and must not be shared
-            between two entries of the option list.
+            One octet is padded with a ``Pad1`` option instead, that being the
+            only form which can occupy a single octet. Two octets are one
+            ``PadN`` with an ``Opt Data Len`` of ``0``, not two ``Pad1``:
+            :rfc:`8200#section-4.2` says a ``PadN`` should be used rather than
+            multiple ``Pad1`` options.
 
         """
         if offset % 8 == 0:
             return [], 0
 
         pad_len = 8 - (offset % 8)
-        if pad_len <= 2:
-            return [self._make_opt_pad(Enum_Option.Pad1, length=0)  # type: ignore[arg-type]
-                    for _ in range(pad_len)], pad_len
+        if pad_len == 1:
+            return [self._make_opt_pad(Enum_Option.Pad1, length=0)], pad_len  # type: ignore[arg-type]
         return [self._make_opt_pad(Enum_Option.PadN, length=pad_len - 2)], pad_len  # type: ignore[arg-type]
 
     def _make_hopopt_options(self, options: 'list[Schema_Option | tuple[Enum_Option, dict[str, Any]] | bytes] | Option') -> 'tuple[list[Schema_Option | bytes], int]':

pcapkit/protocols/internet/ipv6_opts.py

@@ -1345,20 +1345,19 @@ def _make_pad_options(self, offset: 'int') -> 'tuple[list[Schema_PadOption], int
             A ``PadN`` option spends two octets on its own type and
             ``Opt Data Len`` fields before any padding data, so occupying
             ``pad_len`` octets means an ``Opt Data Len`` of ``pad_len - 2``.
-            One or two octets are padded with ``Pad1`` options instead, that
-            being the only form which can occupy a single octet
-            [:rfc:`8200#section-4.2`]; two octets need two separate ``Pad1``
-            schemas, since a schema instance is mutable and must not be shared
-            between two entries of the option list.
+            One octet is padded with a ``Pad1`` option instead, that being the
+            only form which can occupy a single octet. Two octets are one
+            ``PadN`` with an ``Opt Data Len`` of ``0``, not two ``Pad1``:
+            :rfc:`8200#section-4.2` says a ``PadN`` should be used rather than
+            multiple ``Pad1`` options.
 
         """
         if offset % 8 == 0:
             return [], 0
 
         pad_len = 8 - (offset % 8)
-        if pad_len <= 2:
-            return [self._make_opt_pad(Enum_Option.Pad1, length=0)  # type: ignore[arg-type]
-                    for _ in range(pad_len)], pad_len
+        if pad_len == 1:
+            return [self._make_opt_pad(Enum_Option.Pad1, length=0)], pad_len  # type: ignore[arg-type]
         return [self._make_opt_pad(Enum_Option.PadN, length=pad_len - 2)], pad_len  # type: ignore[arg-type]
 
     def _make_ipv6_opts(self, options: 'list[Schema_Option | tuple[Enum_Option, dict[str, Any]] | bytes] | Option') -> 'tuple[list[Schema_Option | bytes], int]':

pcapkit/protocols/internet/mh.py

@@ -7742,20 +7742,19 @@ def _make_pad_options(self, offset: 'int') -> 'tuple[list[Schema_PadOption], int
             A ``PadN`` option spends two octets on its own type and
             ``Option Length`` fields before any padding data, so occupying
             ``pad_len`` octets means an ``Option Length`` of ``pad_len - 2``.
-            One or two octets are padded with ``Pad1`` options instead, that
-            being the only form which can occupy a single octet
-            [:rfc:`6275#section-6.2.2`]; two octets need two separate ``Pad1``
-            schemas, since a schema instance is mutable and must not be shared
-            between two entries of the option list.
+            One octet is padded with a ``Pad1`` option instead, that being the
+            only form which can occupy a single octet. Two octets are one
+            ``PadN`` with an ``Option Length`` of ``0``, not two ``Pad1``:
+            :rfc:`6275#section-6.2.2` says a ``PadN`` should be used rather than
+            multiple ``Pad1`` options.
 
         """
         if offset % 8 == 0:
             return [], 0
 
         pad_len = 8 - (offset % 8)
-        if pad_len <= 2:
-            return [self._make_opt_pad(Enum_Option.Pad1, length=0)  # type: ignore[arg-type]
-                    for _ in range(pad_len)], pad_len
+        if pad_len == 1:
+            return [self._make_opt_pad(Enum_Option.Pad1, length=0)], pad_len  # type: ignore[arg-type]
         return [self._make_opt_pad(Enum_Option.PadN, length=pad_len - 2)], pad_len  # type: ignore[arg-type]
 
     def _pad_mh_message(self, data: 'Schema_Packet | bytes') -> 'Schema_Packet | bytes':

tests/protocols/internet/test_ipv6_extension_unit.py

@@ -1068,17 +1068,19 @@ def _assert_option_constructors_cover_branchy_values(self, protocol_cls: type) -
         self.assertEqual(schema_length, 14)
         self.assertEqual(type(schema_options[0]).__name__, 'UnassignedOption')
 
-        # NOTE: Two ``Pad1`` options are emitted when the shortfall is exactly 2
-        # octets, i.e. when ``2 + opt_len`` is 6 past a multiple of 8. That is a
-        # 4-octet option, not the 6-octet one this case used to pass -- 2 + 6 is
-        # already aligned and now needs no padding at all.
-        pad1_options, pad1_length = make_options([
+        # NOTE: A shortfall of exactly 2 octets, i.e. ``2 + opt_len`` 6 past a
+        # multiple of 8, is one ``PadN`` of ``Opt Data Len`` 0 rather than two
+        # ``Pad1`` (#1793). That is a 4-octet option, not the 6-octet one this
+        # case used to pass -- 2 + 6 is already aligned and needs no padding.
+        pad2_options, pad2_length = make_options([
             bytes([Option.CALIPSO]) + b'123',
         ])
-        # 4 octets of CALIPSO + two 1-octet Pad1 = 6; 2 + 6 = 8
-        self.assertEqual(pad1_length, 6)
+        # 4 octets of CALIPSO + a 2-octet PadN = 6; 2 + 6 = 8
+        self.assertEqual(pad2_length, 6)
         self.assertEqual([type(item).__name__ if not isinstance(item, bytes) else 'bytes'
-                          for item in pad1_options], ['bytes', 'PadOption', 'PadOption'])
+                          for item in pad2_options], ['bytes', 'PadOption'])
+        self.assertEqual(pad2_options[1].type, Option.PadN)
+        self.assertEqual(pad2_options[1].pack(), b'\x01\x00')
 
         # NOTE: Likewise, the option that needs no padding is the 6-octet one; the
         # 8-octet option this case used to pass leaves 2 + 8 = 10 and now draws 6
@@ -1113,7 +1115,7 @@ def _assert_option_constructors_cover_branchy_values(self, protocol_cls: type) -
         for label, (built, built_len) in {
                 'bytes + padn': (options, total_length),
                 'schema + padn': (schema_options, schema_length),
-                'two pad1': (pad1_options, pad1_length),
+                'two-octet padn': (pad2_options, pad2_length),
                 'no padding': (no_pad_options, no_pad_length),
                 'from dict': (dict_options, dict_length),
         }.items():
@@ -2287,8 +2289,9 @@ def _assert_constructed_header_round_trips(self, protocol_cls: type) -> None:
 
         The option lengths are chosen for the padding they draw: the whole
         header, its two fixed octets included, has to be a multiple of 8, so an
-        option of ``8n + 5`` octets needs a single ``Pad1``, one of ``8n + 4``
-        needs two, and anything else needs a ``PadN``.
+        option of ``8n + 5`` octets needs a single ``Pad1``, and anything else
+        short of the boundary needs a ``PadN`` -- including one of ``8n + 4``,
+        whose two spare octets are a ``PadN`` of ``Opt Data Len`` 0.
 
         """
         from pcapkit.const.ipv6.option import Option
@@ -2298,9 +2301,9 @@ def _assert_constructed_header_round_trips(self, protocol_cls: type) -> None:
             # a 5-octet option leaves one octet, which only a Pad1 can fill
             ('one pad1', [(Option.get(250), {'data': b'abc'})], 8,
              [(Option.get(250), 5), (Option.Pad1, 1)]),
-            # a 4-octet option leaves two, i.e. two Pad1 rather than a PadN
-            ('two pad1', [(Option.Router_Alert, {'alert': 0})], 8,
-             [(Option.Router_Alert, 4), (Option.Pad1, 1), (Option.Pad1, 1)]),
+            # a 4-octet option leaves two, i.e. one PadN of length 0 (#1793)
+            ('two-octet padn', [(Option.Router_Alert, {'alert': 0})], 8,
+             [(Option.Router_Alert, 4), (Option.PadN, 2)]),
             # three or more spare octets are a PadN, whose own two octets of
             # overhead have to come out of the padding it is asked to occupy
             ('padn', [(Option.Tunnel_Encapsulation_Limit, {'limit': 3})], 8,

tests/protocols/internet/test_mh_flags_reserved_unit.py

@@ -52,6 +52,14 @@
     'PerSessionBitRateAttribute': ('3b020000000000003a0e00000000000003063fff000003e8', 14),
 }
 
+#: ``site -> octets make() emits``, where they differ from :data:`FRAMES`. Two
+#: octets of fresh padding are one ``PadN`` of length 0 (#1793), whereas these
+#: frames end in two ``Pad1``, which a parsed header keeps on rebuild.
+MADE = {
+    'DynamicIPMulticastSelectorOption': '3b0100000000000036048f7f00000100',
+    'IPv4TrafficOffloadSelectorOption': '3b0100000000000035047fffffff0100',
+}
+
 MESSAGES = {
     'BindingRevocationMessage': 'Binding_Revocation_Message',
     'FastBindingAcknowledgmentMessage': 'Fast_Binding_Acknowledgment',
@@ -169,7 +177,7 @@ def test_make_writes_the_reserved_bits(self) -> None:
         for site, (frame, width) in FRAMES.items():
             with self.subTest(site=site):
                 built = self._make(site, value=(1 << width) - 1)
-                self.assertEqual(built.hex(), frame)
+                self.assertEqual(built.hex(), MADE.get(site, frame))
                 model, attr = self._site(self._parse(built).info, site)  # type: ignore[attr-defined]
                 self.assertEqual(getattr(model, attr), (1 << width) - 1)
 

tests/protocols/internet/test_mh_unit.py

@@ -2052,8 +2052,9 @@ def test_mh_constructed_header_round_trips(self) -> None:
 
         brr = Packet.Binding_Refresh_Request
         cases = (
-            # 2 octets of reserved + a 6-octet PadN = 8, so 6 + 8 = 14 needs 2 more
-            ('padn then two pad1', [(Option.PadN, {'length': 4})], 16),
+            # 2 octets of reserved + a 6-octet PadN = 8, so 6 + 8 = 14 needs 2
+            # more, which are one PadN of Option Length 0 (#1793)
+            ('padn then a two-octet padn', [(Option.PadN, {'length': 4})], 16),
             # 2 + 8 = 10, and 6 + 10 = 16 is already aligned
             ('padn, already aligned', [(Option.PadN, {'length': 6})], 16),
             # 2 + 4 = 6, and 6 + 6 = 12 needs 4 more octets

tests/protocols/internet/test_mpl_seed_rebuild_unit.py

@@ -76,7 +76,9 @@ def test_address_seed_keyword_selects_source_address_kind(self) -> None:
             with self.subTest(proto=name):
                 built = cls(options=[(Option.MPL_Option, {'seed': SRC, 'seq': 7, 'max': True})],
                             next=TransType.IPv6_NoNxt, payload=b'')
-                self.assertEqual(built.data, HEADERS[0])
+                # HEADERS[0] ends in two Pad1, which a parsed header keeps; two
+                # octets of fresh padding are one PadN of length 0 (#1793)
+                self.assertEqual(built.data, HEADERS[0][:-2] + b'\x01\x00')
 
     def test_seed_that_does_not_fit_its_kind_is_rejected(self) -> None:
         from pcapkit.const.ipv6.option import Option

tests/protocols/internet/test_padn_two_octets_1793_unit.py

@@ -0,0 +1,121 @@
+# -*- coding: utf-8 -*-
+"""Two octets of option padding are one ``PadN``, not two ``Pad1``.
+
+GitHub issue #1793: HOPOPT, IPv6-Opts and MH each filled a two-octet shortfall
+with two ``Pad1`` options. :rfc:`8200#section-4.2`, and :rfc:`6275#section-6.2.2`
+for mobility options, say a ``PadN`` should be used rather than multiple
+``Pad1``, so two octets are now one ``PadN`` of length ``0``: the octets
+``01 00``. One octet is still a ``Pad1``, and parsed padding is still rebuilt as
+it was read -- only freshly built padding changed.
+
+Every case builds its own octets in memory and reads no capture. Classes are
+imported inside each test, after :func:`~tests._support.reimport_once_per_class`.
+
+"""
+
+import importlib
+import io
+import unittest
+import warnings
+
+from tests._support import reimport_once_per_class
+
+#: Protocol module, class name and option enum module of each padding builder.
+PROTOCOLS = (
+    ('pcapkit.protocols.internet.hopopt', 'HOPOPT', 'pcapkit.const.ipv6.option'),
+    ('pcapkit.protocols.internet.ipv6_opts', 'IPv6_Opts', 'pcapkit.const.ipv6.option'),
+    ('pcapkit.protocols.internet.mh', 'MH', 'pcapkit.const.mh.option'),
+)
+
+#: Parsed headers whose padding is two ``Pad1`` options, which have to rebuild
+#: unchanged: a Router Alert in an options header, and a Binding Refresh Request
+#: carrying a 6-octet ``PadN``.
+TWO_PAD1 = {
+    'HOPOPT': '3b00' '05020000' '00' '00',
+    'IPv6_Opts': '3b00' '05020000' '00' '00',
+    'MH': '3b01000000000000' '010400000000' '00' '00',
+}
+
+
+class TestTwoOctetPadding(unittest.TestCase):
+    """Pin the wire form of two octets of HOPOPT, IPv6-Opts and MH padding."""
+
+    def setUp(self) -> None:
+        reimport_once_per_class(self)
+
+    def _protocols(self) -> 'list[tuple[type, type]]':
+        return [(getattr(importlib.import_module(mod), name),
+                 importlib.import_module(enum).Option)
+                for mod, name, enum in PROTOCOLS]
+
+    def test_two_octets_are_one_zero_length_padn(self) -> None:
+        for cls, option in self._protocols():
+            proto = object.__new__(cls)
+            # the offset counts the fixed header too, so 6 and 14 are two short
+            for offset in (6, 14):
+                with self.subTest(protocol=cls.__name__, offset=offset):
+                    with warnings.catch_warnings():
+                        warnings.simplefilter('error')
+                        pads, pad_len = proto._make_pad_options(offset)
+                    self.assertEqual(pad_len, 2)
+                    self.assertEqual([pad.type for pad in pads], [option.PadN])
+                    self.assertEqual(pads[0].pack(), b'\x01\x00')
+
+    def test_one_and_three_octets_keep_their_form(self) -> None:
+        for cls, option in self._protocols():
+            proto = object.__new__(cls)
+            with self.subTest(protocol=cls.__name__, octets=1):
+                pads, pad_len = proto._make_pad_options(7)
+                self.assertEqual(pad_len, 1)
+                self.assertEqual([(pad.type, pad.pack()) for pad in pads],
+                                 [(option.Pad1, b'\x00')])
+            with self.subTest(protocol=cls.__name__, octets=3):
+                pads, pad_len = proto._make_pad_options(5)
+                self.assertEqual(pad_len, 3)
+                self.assertEqual([(pad.type, pad.pack()) for pad in pads],
+                                 [(option.PadN, b'\x01\x01\x00')])
+
+    def test_constructed_header_ends_in_one_zero_length_padn(self) -> None:
+        from pcapkit.const.ipv6.option import Option as IPv6_Option
+        from pcapkit.const.mh.option import Option as MH_Option
+        from pcapkit.const.mh.packet import Packet
+        from pcapkit.const.reg.transtype import TransType
+
+        protocols = {cls.__name__: cls for cls, _ in self._protocols()}
+        built = {
+            # 2 + a 4-octet Router Alert = 6, two short of 8
+            'HOPOPT': (protocols['HOPOPT'](next=TransType.IPv6_NoNxt,
+                                           options=[(IPv6_Option.Router_Alert, {'alert': 0})]),
+                       '3b00' '05020000' '0100',
+                       [(IPv6_Option.Router_Alert, 4), (IPv6_Option.PadN, 2)]),
+            'IPv6_Opts': (protocols['IPv6_Opts'](next=TransType.IPv6_NoNxt,
+                                                 options=[(IPv6_Option.Router_Alert, {'alert': 0})]),
+                          '3b00' '05020000' '0100',
+                          [(IPv6_Option.Router_Alert, 4), (IPv6_Option.PadN, 2)]),
+            # 6 + 2 octets of reserved + a 6-octet PadN = 14, two short of 16
+            'MH': (protocols['MH'](next=TransType.IPv6_NoNxt, type=Packet.Binding_Refresh_Request,
+                                   chksum=b'\x00\x00', data={'options': [(MH_Option.PadN, {'length': 4})]}),
+                   '3b01000000000000' '010400000000' '0100',
+                   [(MH_Option.PadN, 6), (MH_Option.PadN, 2)]),
+        }
+        for name, (proto, hexstr, expected) in built.items():
+            with self.subTest(protocol=name):
+                raw = bytes(proto)
+                self.assertEqual(raw.hex(), hexstr)
+                parsed = type(proto)(io.BytesIO(raw), len(raw), extension=True)
+                self.assertEqual([(code, opt.length)
+                                  for code, opt in parsed.info.options.items(multi=True)],
+                                 expected)
+
+    def test_parsed_two_pad1_rebuild_byte_for_byte(self) -> None:
+        for cls, option in self._protocols():
+            octets = bytes.fromhex(TWO_PAD1[cls.__name__])
+            with self.subTest(protocol=cls.__name__):
+                parsed = cls(io.BytesIO(octets), len(octets), extension=True)
+                pads = list(parsed.info.options.items(multi=True))[-2:]
+                self.assertEqual([code for code, _ in pads], [option.Pad1, option.Pad1])
+                self.assertEqual(bytes(cls.from_data(parsed.info)).hex(), octets.hex())
+
+
+if __name__ == '__main__':
+    unittest.main()

tests/protocols/test_protocol_make_unit.py

@@ -51,12 +51,13 @@ def _attr(path: 'str') -> 'Any':
     'TCP options': ('pcapkit.protocols.transport.tcp.TCP',
                     {'options': [b'\x02\x04\x05\xb4']},
                     '0000000000000000000000006000ffff00000000020405b4'),
+    # two octets of padding follow the option, as one zero-length PadN (#1793)
     'HOPOPT options': ('pcapkit.protocols.internet.hopopt.HOPOPT',
                        {'extension': True, 'options': [b'\x05\x02\x00\x00']},
-                       '1100050200000000'),
+                       '1100050200000100'),
     'IPv6_Opts options': ('pcapkit.protocols.internet.ipv6_opts.IPv6_Opts',
                           {'extension': True, 'options': [b'\x05\x02\x00\x00']},
-                          '1100050200000000'),
+                          '1100050200000100'),
     'MH options': ('pcapkit.protocols.internet.mh.MH',
                    {'data': {'options': [b'\x01\x02\x00\x00']}},
                    '11010000000000000102000001020000'),