diff --git a/pcapkit/protocols/internet/ipv4.py b/pcapkit/protocols/internet/ipv4.py index fc45c9ef76..25a1bbb252 100644 --- a/pcapkit/protocols/internet/ipv4.py +++ b/pcapkit/protocols/internet/ipv4.py @@ -517,6 +517,10 @@ def _make_data(cls, data: 'Data_IPv4') -> 'dict[str, Any]': # type: ignore[over Key-value pairs for protocol construction. """ + # NOTE: ``make`` takes ``offset`` in on-wire 8-octet units, while + # ``data.offset`` is in octets (see :meth:`read`), so scale back down to + # keep the data-to-schema round trip exact, mirroring + # :meth:`pcapkit.protocols.internet.ipv6_frag.IPv6_Frag._make_data`. return { 'tos_pre': data.tos.pre, 'tos_del': data.tos['del'], @@ -526,13 +530,16 @@ def _make_data(cls, data: 'Data_IPv4') -> 'dict[str, Any]': # type: ignore[over 'id': data.id, 'df': data.flags.df, 'mf': data.flags.mf, - 'offset': data.offset, + 'offset': data.offset // 8, 'ttl': data.ttl, 'protocol': data.protocol, 'checksum': data.checksum, 'src': data.src, 'dst': data.dst, - 'options': data.options, + # NOTE: ``options`` is only present on ``data`` when the packet's + # ``hdr_len`` exceeds the fixed 20-octet header (see :meth:`read`), + # so read it defensively rather than assuming it always exists. + 'options': getattr(data, 'options', None), 'payload': cls._make_payload(data), } @@ -1359,7 +1366,11 @@ def _make_opt_sec(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_SECOpt Args: kind: option type code option: option data - sec: security option + level: classification level + level_default: default value for classification level + level_namespace: namespace for classification level + level_reversed: whether classification level is reversed + authorities: list of protection authority flags **kwargs: arbitrary keyword arguments Returns: @@ -1773,8 +1784,8 @@ def _make_opt_qs(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_QuickSt """Make IPv4 Quick-Start (``QS``) option. Args: - code: option type value - opt: option data + kind: option type code + option: option data func: QS function type func_default: default value for QS function type func_namespace: namespace for QS function type diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index 808a079b2a..42cd85a5b9 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -2,10 +2,12 @@ import datetime import importlib.util +import io from ipaddress import ip_address import types import unittest from unittest import mock +import warnings from tests._support import purge_modules @@ -56,6 +58,54 @@ class DummyDict(dict): self.assertEqual(values['dst'], '198.51.100.20') self.assertIn('payload', values) + def test_ipv4_make_data_scales_offset_and_defaults_missing_options(self) -> None: + """Regression test for #494. + + Two defects live in :meth:`IPv4._make_data + `: the fragment + ``offset`` was handed back in octets to a parameter that + :meth:`IPv4.make ` takes in + on-wire 8-octet units (:rfc:`791`), and ``data.options`` was read + unconditionally even though :meth:`IPv4.read + ` only sets it when + ``hdr_len`` exceeds the fixed 20-octet header. + + The two interact: the missing-``options`` ``AttributeError`` fires + before the unscaled ``offset`` can even be returned, so a fixture + that always supplies ``options`` -- like the ``DummyDict`` one above, + which also uses ``offset=0``, the one value for which the missing + ``// 8`` is invisible -- cannot catch either. This test uses a + non-zero offset *and* a real packet with no options, so both + defects are exercised together, exactly as the issue's own repro + does. + + """ + from pcapkit.protocols.internet.ipv4 import IPv4 + + proto = object.__new__(IPv4) + + # Wire Fragment Offset of 5 (8-octet units), no options -> hdr_len + # stays at the fixed 20 octets and read() never sets ``.options``. + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + raw = proto.make(offset=5, protocol=6, payload=b'\xaa' * 8).pack() + data = IPv4(io.BytesIO(raw), len(raw)).info + + self.assertFalse(hasattr(data, 'options')) + self.assertEqual(data.offset, 40) # read() scales wire units to octets + + values = IPv4._make_data(data) + self.assertEqual(values['offset'], 5) # scaled back down to wire units + self.assertIsNone(values['options']) + + # Full round trip: re-packing with the recovered offset must + # reproduce the exact original wire bytes. + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + raw2 = proto.make(offset=values['offset'], protocol=6, + payload=b'\xaa' * 8).pack() + self.assertEqual(raw, raw2) + def test_ipv4_properties_read_and_make_cover_packet_paths(self) -> None: from pcapkit.const.ipv4.option_number import OptionNumber from pcapkit.const.reg.transtype import TransType