Skip to content

Commit d0fd8cd

Browse files
JarryShawJarryShaw
committed
fix(protocols): makers name the rejected argument and value in error messages
Several _make_* builders rejected a bad caller-supplied argument with the reader's parse-error text ("invalid format"), which gives no indication of which argument was wrong or what value triggered it. #2296. - sctp.py: all 11 identical `len(flags) != 1` guards across the chunk makers, plus _make_chunk_data's empty-data guard, _make_param_hostname's NUL-termination guard, and _make_cause_invalid_stream's reserved-length guard. - mh.py: make()'s unsupported-data-type guard, _make_opt_bad/_make_opt_auth's length guards, _make_opt_badf's empty-data guard, and _make_opt_pad's Pad1-length warning. Added the missing Raises:/Warns: docstring sections. - tcp.py: _make_mptcp_capable now names which optional field was given without its predecessor. - hip.py: _make_locator's fallback branch (the one site #2276 missed). - pcapng.py: _make_block_dsb's fallback branch (the one site #2273 missed). _make_chunk_data's flags= example in the issue doesn't match any real parameter on that method (it takes reserved/I/U/B/E, not flags), so its exact wording was applied to the 11 makers that do take flags, and _make_chunk_data got a data-shaped message instead. Test: tests/protocols/test_maker_invalid_format_messages_2296_unit.py (new, 22 tests/82 subtests, one class per site group); confirmed 66 failed/20 passed against unpatched pcapkit, each failure a message-text mismatch. tests/protocols/ in full: 2011 passed/51370 subtests/0 failed. mypy and pylint findings on the 5 touched files are unchanged from main. Four other maker-side sites still use the generic text but aren't in #2296's checklist -- HOPOPT._make_opt_pad, IPv6_Opts._make_opt_pad, HTTP.make, PCAPNG.make -- tracked separately rather than folded in here.
1 parent 7951ee4 commit d0fd8cd

6 files changed

Lines changed: 536 additions & 23 deletions

File tree

‎pcapkit/protocols/internet/hip.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3450,7 +3450,9 @@ def _make_locator(locator: 'Optional[Data_Locator]' = None, *,
34503450
ip=value.ip.packed,
34513451
)
34523452
else:
3453-
raise ProtocolError(f'HIPv{version}: [ParamNo {code}] invalid format')
3453+
raise ProtocolError(f'HIPv{version}: [ParamNo {code}] invalid locator: '
3454+
f'must be an ipaddress.IPv6Address or a LocatorData, '
3455+
f'not {value!r}')
34543456

34553457
traffic = locator.traffic
34563458
type = locator.type

‎pcapkit/protocols/internet/mh.py‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1454,6 +1454,12 @@ def make(self,
14541454
Returns:
14551455
Constructed packet data.
14561456
1457+
Raises:
1458+
ProtocolError: If ``data`` is none of the forms this maker reads --
1459+
raw :obj:`bytes`, a :obj:`dict` of message arguments, an
1460+
:class:`~pcapkit.protocols.data.internet.mh.MH` data model or a
1461+
:class:`~pcapkit.protocols.schema.internet.mh.Packet` schema.
1462+
14571463
.. versionchanged:: 1.5.0
14581464
14591465
Takes ``reserved`` ahead of ``chksum`` (:issue:`1329`), which shifts
@@ -1483,7 +1489,8 @@ def make(self,
14831489
elif isinstance(data, Schema_Packet):
14841490
data_val = data
14851491
else:
1486-
raise ProtocolError(f'MH: [Type {type_val}] invalid format')
1492+
raise ProtocolError(f'MH: [Type {type_val}] invalid data: must be bytes, a dict, '
1493+
f'an MH data model or a Packet schema, not {data!r}')
14871494

14881495
# NOTE: The header has to be a multiple of 8 octets, so the message data
14891496
# needs padding until ``len(data) + 6`` is aligned. Rounding ``length`` up
@@ -7903,6 +7910,11 @@ def _make_opt_pad(self, type: 'Enum_Option', option: 'Optional[Data_PadOption]'
79037910
Returns:
79047911
Constructed option schema.
79057912
7913+
Warns:
7914+
ProtocolWarning: If ``length`` is non-zero for a ``Pad1``, which
7915+
has no ``Option Length`` field to carry it; the option is made
7916+
as a ``PadN`` instead.
7917+
79067918
Note:
79077919
:attr:`Data_PadOption.length
79087920
<pcapkit.protocols.data.internet.mh.Option.length>` counts the
@@ -7921,7 +7933,8 @@ def _make_opt_pad(self, type: 'Enum_Option', option: 'Optional[Data_PadOption]'
79217933
data = getattr(option, 'data', b'')
79227934

79237935
if type == Enum_Option.Pad1 and length != 0:
7924-
warn(f'{self.alias}: [OptNo {type}] invalid format', ProtocolWarning)
7936+
warn(f'{self.alias}: [OptNo {type}] invalid length: must be 0 for a Pad1 option, '
7937+
f'not {length!r}; making a PadN', ProtocolWarning)
79257938
type = Enum_Option.PadN # type: ignore[assignment]
79267939

79277940
return Schema_PadOption(
@@ -8024,12 +8037,18 @@ def _make_opt_bad(self, type: 'Enum_Option', option: 'Optional[Data_Authorizatio
80248037
Returns:
80258038
Constructed option schema.
80268039
8040+
Raises:
8041+
ProtocolError: If ``data`` is not a multiple of eight octets, the
8042+
length :meth:`_read_opt_bad` accepts for an :rfc:`6275`
8043+
authenticator.
8044+
80278045
"""
80288046
if option is not None:
80298047
data = option.data
80308048

80318049
if len(data) % 8 != 0:
8032-
raise ProtocolError(f'{self.alias}: [OptNo {type}] invalid format')
8050+
raise ProtocolError(f'{self.alias}: [OptNo {type}] invalid data: '
8051+
f'must be a multiple of eight octets, not {data!r}')
80338052

80348053
return Schema_AuthorizationDataOption(
80358054
type=type,
@@ -8349,6 +8368,11 @@ def _make_opt_auth(self, type: 'Enum_Option', option: 'Optional[Data_AuthOption]
83498368
Returns:
83508369
Constructed option schema.
83518370
8371+
Raises:
8372+
ProtocolError: If ``data`` is not two octets more than a multiple of
8373+
four, which is the only length leaving an ``Option Length`` of
8374+
``5 + len(data)`` that :meth:`_read_opt_auth` accepts.
8375+
83528376
"""
83538377
if option is not None:
83548378
subtype_val = option.subtype
@@ -8359,7 +8383,8 @@ def _make_opt_auth(self, type: 'Enum_Option', option: 'Optional[Data_AuthOption]
83598383
reversed=subtype_reversed, pack=False)
83608384

83618385
if (len(data) + 6) % 4 != 0:
8362-
raise ProtocolError(f'{self.alias}: [OptNo {type}] invalid format')
8386+
raise ProtocolError(f'{self.alias}: [OptNo {type}] invalid data: must be two octets '
8387+
f'more than a multiple of four, not {data!r}')
83638388

83648389
return Schema_AuthOption(
83658390
type=type,
@@ -8611,6 +8636,10 @@ def _make_opt_badf(self, type: 'Enum_Option', option: 'Optional[Data_BADFOption]
86118636
Returns:
86128637
Constructed option schema.
86138638
8639+
Raises:
8640+
ProtocolError: If ``data`` is empty, since the ``length`` field then
8641+
reads as ``0``, which :meth:`_read_opt_badf` rejects.
8642+
86148643
Note:
86158644
The ``length`` field counts the authenticator only, excluding the
86168645
SPI, see :meth:`_read_opt_badf`.
@@ -8621,7 +8650,8 @@ def _make_opt_badf(self, type: 'Enum_Option', option: 'Optional[Data_BADFOption]
86218650
data = option.data
86228651

86238652
if not data:
8624-
raise ProtocolError(f'{self.alias}: [OptNo {type}] invalid format')
8653+
raise ProtocolError(f'{self.alias}: [OptNo {type}] invalid data: '
8654+
f'must be at least one octet, not {data!r}')
86258655

86268656
return Schema_BADFOption(
86278657
type=type,

‎pcapkit/protocols/misc/pcapng.py‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4882,6 +4882,12 @@ def _make_block_dsb(self, block: 'Optional[Data_DecryptionSecretsBlock]' = None,
48824882
Returns:
48834883
Constructed block schema.
48844884
4885+
Raises:
4886+
ProtocolError: If ``secrets_data`` is none of the forms this maker
4887+
reads -- raw :obj:`bytes`, a :obj:`dict` of secrets arguments, a
4888+
:class:`~pcapkit.protocols.data.misc.pcapng.DSBSecrets` data
4889+
model or a :class:`~pcapkit.protocols.schema.schema.Schema`.
4890+
48854891
"""
48864892
declared = None # type: Optional[int]
48874893
if block is not None:
@@ -4921,7 +4927,8 @@ def _make_block_dsb(self, block: 'Optional[Data_DecryptionSecretsBlock]' = None,
49214927
elif isinstance(secrets_data, Schema):
49224928
secrets_data_val = secrets_data
49234929
else:
4924-
raise ProtocolError(f'PCAP-NG: [DSB] secrets {secrets_type_val} invalid format')
4930+
raise ProtocolError(f'PCAP-NG: [DSB] invalid secrets_data: must be bytes, a dict, '
4931+
f'a DSBSecrets data model or a Schema, not {secrets_data!r}')
49254932
# NOTE: Secrets Length excludes the padding to the next 32-bit boundary,
49264933
# which ``padding_data`` writes and the block length has to include.
49274934
if block is not None and isinstance(secrets_data_val, Schema):

‎pcapkit/protocols/transport/sctp.py‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1906,7 +1906,8 @@ def _make_chunk_donone(self, code: 'Enum_Chunk', chunk: 'Optional[Data_UnknownCh
19061906
value = chunk.value
19071907

19081908
if len(flags) != 1:
1909-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
1909+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
1910+
f'must be one octet, not {flags!r}')
19101911

19111912
return Schema_UnknownChunk(
19121913
type=code,
@@ -1965,7 +1966,8 @@ def _make_chunk_data(self, code: 'Enum_Chunk', chunk: 'Optional[Data_DATAChunk]'
19651966
data = chunk.data
19661967

19671968
if not data:
1968-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
1969+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid data: '
1970+
f'must be at least one octet, not {data!r}')
19691971

19701972
return Schema_DATAChunk(
19711973
type=code,
@@ -2028,7 +2030,8 @@ def _make_chunk_init(self, code: 'Enum_Chunk', chunk: 'Optional[Data_INITChunk]'
20282030
parameters = chunk.parameters
20292031

20302032
if len(flags) != 1:
2031-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2033+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2034+
f'must be one octet, not {flags!r}')
20322035

20332036
if parameters is not None:
20342037
parameters_value = self._make_sctp_parameters(parameters)
@@ -2089,7 +2092,8 @@ def _make_chunk_init_ack(self, code: 'Enum_Chunk', chunk: 'Optional[Data_INITACK
20892092
parameters = chunk.parameters
20902093

20912094
if len(flags) != 1:
2092-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2095+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2096+
f'must be one octet, not {flags!r}')
20932097

20942098
if parameters is not None:
20952099
parameters_value = self._make_sctp_parameters(parameters)
@@ -2150,7 +2154,8 @@ def _make_chunk_sack(self, code: 'Enum_Chunk', chunk: 'Optional[Data_SACKChunk]'
21502154
dup_tsn = list(chunk.dup_tsn)
21512155

21522156
if len(flags) != 1:
2153-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2157+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2158+
f'must be one octet, not {flags!r}')
21542159

21552160
blocks = [] # type: list[Schema_GapAckBlock]
21562161
for block in gap_blocks or []:
@@ -2200,7 +2205,8 @@ def _make_chunk_heartbeat(self, code: 'Enum_Chunk', chunk: 'Optional[Data_Heartb
22002205
parameters = chunk.parameters
22012206

22022207
if len(flags) != 1:
2203-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2208+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2209+
f'must be one octet, not {flags!r}')
22042210

22052211
if parameters is not None:
22062212
parameters_value = self._make_sctp_parameters(parameters)
@@ -2241,7 +2247,8 @@ def _make_chunk_heartbeat_ack(self, code: 'Enum_Chunk', chunk: 'Optional[Data_He
22412247
parameters = chunk.parameters
22422248

22432249
if len(flags) != 1:
2244-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2250+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2251+
f'must be one octet, not {flags!r}')
22452252

22462253
if parameters is not None:
22472254
parameters_value = self._make_sctp_parameters(parameters)
@@ -2323,7 +2330,8 @@ def _make_chunk_shutdown(self, code: 'Enum_Chunk', chunk: 'Optional[Data_Shutdow
23232330
cum_tsn_ack = chunk.cum_tsn_ack
23242331

23252332
if len(flags) != 1:
2326-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2333+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2334+
f'must be one octet, not {flags!r}')
23272335

23282336
return Schema_ShutdownChunk(
23292337
type=code,
@@ -2354,7 +2362,8 @@ def _make_chunk_shutdown_ack(self, code: 'Enum_Chunk', chunk: 'Optional[Data_Shu
23542362
flags = chunk.flags
23552363

23562364
if len(flags) != 1:
2357-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2365+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2366+
f'must be one octet, not {flags!r}')
23582367

23592368
return Schema_ShutdownACKChunk(
23602369
type=code,
@@ -2387,7 +2396,8 @@ def _make_chunk_error(self, code: 'Enum_Chunk', chunk: 'Optional[Data_ErrorChunk
23872396
error = chunk.error
23882397

23892398
if len(flags) != 1:
2390-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2399+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2400+
f'must be one octet, not {flags!r}')
23912401

23922402
if error is not None:
23932403
error_value = self._make_sctp_causes(error)
@@ -2429,7 +2439,8 @@ def _make_chunk_cookie_echo(self, code: 'Enum_Chunk', chunk: 'Optional[Data_Cook
24292439
cookie = chunk.cookie
24302440

24312441
if len(flags) != 1:
2432-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2442+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2443+
f'must be one octet, not {flags!r}')
24332444

24342445
return Schema_CookieEchoChunk(
24352446
type=code,
@@ -2460,7 +2471,8 @@ def _make_chunk_cookie_ack(self, code: 'Enum_Chunk', chunk: 'Optional[Data_Cooki
24602471
flags = chunk.flags
24612472

24622473
if len(flags) != 1:
2463-
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid format')
2474+
raise ProtocolError(f'{self.alias}: [Chunk {code}] invalid flags: '
2475+
f'must be one octet, not {flags!r}')
24642476

24652477
return Schema_CookieACKChunk(
24662478
type=code,
@@ -3018,7 +3030,8 @@ def _make_param_hostname(self, code: 'Enum_Parameter', param: 'Optional[Data_Hos
30183030
if param is not None:
30193031
name = param.name
30203032
elif not name.endswith(b'\x00'):
3021-
raise ProtocolError(f'{self.alias}: [Param {code}] invalid format')
3033+
raise ProtocolError(f'{self.alias}: [Param {code}] invalid name: '
3034+
f'must be NUL-terminated, not {name!r}')
30223035

30233036
return Schema_HostNameAddressParameter(
30243037
type=code,
@@ -3583,7 +3596,8 @@ def _make_cause_invalid_stream(self, code: 'Enum_CauseCode', cause: 'Optional[Da
35833596
reserved = cause.reserved
35843597

35853598
if len(reserved) != 2:
3586-
raise ProtocolError(f'{self.alias}: [Cause {code}] invalid format')
3599+
raise ProtocolError(f'{self.alias}: [Cause {code}] invalid reserved: '
3600+
f'must be two octets, not {reserved!r}')
35873601

35883602
return Schema_InvalidStreamIdentifierCause(
35893603
code=code,

‎pcapkit/protocols/transport/tcp.py‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2971,10 +2971,14 @@ def _make_mptcp_capable(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_M
29712971
dl_len = opt.dl_len
29722972
checksum = opt.checksum
29732973

2974+
names = ('skey', 'rkey', 'dl_len', 'checksum')
29742975
fields = (skey, rkey, dl_len, checksum)
29752976
present = fields.index(None) if None in fields else len(fields)
2976-
if any(field is not None for field in fields[present:]):
2977-
raise ProtocolError(f'{self.alias}: [OptNo {Enum_Option.Multipath_TCP}] invalid format')
2977+
for index in range(present, len(fields)):
2978+
if fields[index] is not None:
2979+
raise ProtocolError(
2980+
f'{self.alias}: [OptNo {Enum_Option.Multipath_TCP}] invalid {names[index]}: '
2981+
f'must be None when {names[present]} is None, not {fields[index]!r}')
29782982

29792983
return Schema_MPTCPCapable(
29802984
kind=cast('Enum_Option', Enum_Option.Multipath_TCP),

0 commit comments

Comments
 (0)