Skip to content

Commit 5951cb5

Browse files
akbarsiddicakbarsiddic
authored
return ILLEGAL_VALUE for out-of-range counts (#3066)
1 parent 77ad0c0 commit 5951cb5

6 files changed

Lines changed: 174 additions & 10 deletions

File tree

‎pymodbus/pdu/bit_message.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,13 @@ def x_encode(self) -> bytes:
3232
def x_decode(self, data: bytes) -> None:
3333
"""Decode a request pdu."""
3434
self.address, self.count = struct.unpack(">HH", data[:4])
35-
self.verifyCount(self.MAX_COUNT)
3635

3736
async def datastore_update(
3837
self, context: ModbusServerContext, device_id: int
3938
) -> ModbusPDU:
4039
"""Run request against a datastore."""
40+
if not 1 <= self.count <= self.MAX_COUNT:
41+
return ExceptionResponse(self.function_code, ExcCodes.ILLEGAL_VALUE)
4142
values = await context.async_getValues(
4243
device_id, self.function_code, self.address, self.count
4344
)
@@ -70,12 +71,13 @@ def x_encode(self) -> bytes:
7071
def x_decode(self, data: bytes) -> None:
7172
"""Decode a request pdu."""
7273
self.address, self.count = struct.unpack(">HH", data[:4])
73-
self.verifyCount(self.MAX_COUNT)
7474

7575
async def datastore_update(
7676
self, context: ModbusServerContext, device_id: int
7777
) -> ModbusPDU:
7878
"""Run request against a datastore."""
79+
if not 1 <= self.count <= self.MAX_COUNT:
80+
return ExceptionResponse(self.function_code, ExcCodes.ILLEGAL_VALUE)
7981
values = await context.async_getValues(
8082
device_id, self.function_code, self.address, self.count
8183
)

‎pymodbus/pdu/register_message.py‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,13 @@ def x_encode(self) -> bytes:
3434
def x_decode(self, data: bytes) -> None:
3535
"""Decode a register request packet."""
3636
self.address, self.count = struct.unpack(">HH", data[:4])
37-
self.verifyCount(self.MAX_COUNT)
3837

3938
async def datastore_update(
4039
self, context: ModbusServerContext, device_id: int
4140
) -> ModbusPDU:
4241
"""Update diagnostic request on the given device."""
42+
if not 1 <= self.count <= self.MAX_COUNT:
43+
return ExceptionResponse(self.function_code, ExcCodes.ILLEGAL_VALUE)
4344
values = await context.async_getValues(
4445
device_id, self.function_code, self.address, self.count
4546
)
@@ -101,12 +102,13 @@ def x_encode(self) -> bytes:
101102
def x_decode(self, data: bytes) -> None:
102103
"""Decode a register request packet."""
103104
self.address, self.count = struct.unpack(">HH", data[:4])
104-
self.verifyCount(self.MAX_COUNT)
105105

106106
async def datastore_update(
107107
self, context: ModbusServerContext, device_id: int
108108
) -> ModbusPDU:
109109
"""Update diagnostic request on the given device."""
110+
if not 1 <= self.count <= self.MAX_COUNT:
111+
return ExceptionResponse(self.function_code, ExcCodes.ILLEGAL_VALUE)
110112
values = await context.async_getValues(
111113
device_id, self.function_code, self.address, self.count
112114
)
@@ -208,7 +210,6 @@ def x_decode(self, data: bytes) -> None:
208210
self.write_count,
209211
self.write_byte_count,
210212
) = struct.unpack(">HHHHB", data[:9])
211-
self.verifyCount(self.MAX_READ_COUNT, count=self.read_count)
212213
self.verifyCount(self.MAX_READ_COUNT, count=self.write_count)
213214
self._payload_byte_count = len(data) - 9
214215
self.write_registers = [

‎test/conftest.py‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import pytest
1111
import pytest_asyncio
1212

13-
from pymodbus.constants import ExcCodes
1413
from pymodbus.datastore import ModbusDeviceContext, ModbusServerContext
1514
from pymodbus.pdu import ModbusPDU
1615
from pymodbus.server import ServerAsyncStop
@@ -231,8 +230,6 @@ def __init__(self, valid=False, default=True):
231230
async def async_getValues(self, device_id, func_code, address, count=0):
232231
"""Get values."""
233232
_ = device_id, func_code, address
234-
if count > 0x100:
235-
return ExcCodes.ILLEGAL_VALUE
236233
return [self.default] * count
237234

238235
async def async_setValues(self, device_id, func_code, address, values):

‎test/pdu/test_bit.py‎

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import pymodbus.pdu.bit_message as bit_msg
88
from pymodbus.constants import ExcCodes
9+
from pymodbus.pdu.decoders import DecodePDU
910

1011

1112
class TestModbusBitMessage:
@@ -40,13 +41,70 @@ async def test_bit_read_datastore_update_value_errors(self, mock_server_context)
4041
):
4142
await pdu.datastore_update(context, 1)
4243

44+
@pytest.mark.parametrize(
45+
("request_class", "function_code"),
46+
[
47+
(bit_msg.ReadCoilsRequest, 1),
48+
(bit_msg.ReadDiscreteInputsRequest, 2),
49+
],
50+
)
51+
async def test_bit_read_count_above_max_returns_illegal_value(
52+
self, request_class, function_code, mock_server_context
53+
):
54+
"""An out-of-range bit read quantity must answer 0x03, not silence."""
55+
context = mock_server_context()
56+
context.async_getValues = mock.AsyncMock(return_value=[True] * 10)
57+
request = request_class(address=0, count=request_class.MAX_COUNT + 1)
58+
result = await request.datastore_update(context, 1)
59+
assert result.function_code == function_code | 0x80
60+
assert result.exception_code == ExcCodes.ILLEGAL_VALUE
61+
context.async_getValues.assert_not_awaited()
62+
63+
@pytest.mark.parametrize(
64+
"request_class",
65+
[bit_msg.ReadCoilsRequest, bit_msg.ReadDiscreteInputsRequest],
66+
)
67+
async def test_bit_read_count_zero_returns_illegal_value(
68+
self, request_class, mock_server_context
69+
):
70+
"""A zero bit read quantity must answer 0x03."""
71+
context = mock_server_context()
72+
context.async_getValues = mock.AsyncMock(return_value=[True] * 10)
73+
result = await request_class(address=0, count=0).datastore_update(context, 1)
74+
assert result.function_code == request_class.function_code | 0x80
75+
assert result.exception_code == ExcCodes.ILLEGAL_VALUE
76+
context.async_getValues.assert_not_awaited()
77+
78+
@pytest.mark.parametrize(
79+
"request_class",
80+
[bit_msg.ReadCoilsRequest, bit_msg.ReadDiscreteInputsRequest],
81+
)
82+
async def test_bit_read_count_at_max_returns_normal_response(
83+
self, request_class, mock_server_context
84+
):
85+
"""A count exactly at the maximum must be accepted, not rejected."""
86+
context = mock_server_context()
87+
request = request_class(address=0, count=request_class.MAX_COUNT)
88+
result = await request.datastore_update(context, 1)
89+
assert result.function_code == request_class.function_code
90+
91+
def test_bit_read_decode_accepts_count_above_max(self):
92+
"""Decoding must not raise for an over-max bit read quantity."""
93+
decoder = DecodePDU(True)
94+
for frame, expected_count in (
95+
(b"\x01\x00\x00\x07\xd1", 2001),
96+
(b"\x02\x00\x00\x07\xd1", 2001),
97+
):
98+
pdu = decoder.decode(frame)
99+
assert pdu.count == expected_count
100+
43101
async def test_bit_datastore_exceptions(self, mock_server_context):
44102
"""Test bit exception response from datastore."""
45103
context = mock_server_context()
46104
context.async_getValues = mock.AsyncMock(return_value=ExcCodes.ILLEGAL_VALUE)
47105
for pdu in (
48-
(bit_msg.ReadCoilsRequest(address=1, count=0x800)),
49-
(bit_msg.ReadDiscreteInputsRequest(address=1, count=0x800)),
106+
(bit_msg.ReadCoilsRequest(address=1, count=5)),
107+
(bit_msg.ReadDiscreteInputsRequest(address=1, count=5)),
50108
(bit_msg.WriteSingleCoilRequest(address=1, bits=[True])),
51109
(bit_msg.WriteMultipleCoilsRequest(address=1, bits=[True] * 5)),
52110
):

‎test/pdu/test_register_read_messages.py‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
from pymodbus.constants import ExcCodes
99
from pymodbus.exceptions import ModbusIOException
10+
from pymodbus.pdu.decoders import DecodePDU
1011
from pymodbus.pdu.register_message import (
1112
ReadHoldingRegistersRequest,
1213
ReadHoldingRegistersResponse,
@@ -127,6 +128,42 @@ def test_readwrite_decode_write_count_above_limit(self):
127128
request.x_decode(struct.pack(">HHHHB", 1, 1, 1, 122, 244) + b"\x00\x00" * 122)
128129
assert request.write_count == 122
129130

131+
def test_readwrite_decode_read_count_above_limit(self):
132+
"""Decoding must accept read_count above 125 so the server can answer 0x03."""
133+
request = ReadWriteMultipleRegistersRequest()
134+
request.x_decode(struct.pack(">HHHHB", 1, 126, 1, 1, 2) + b"\x00\x00")
135+
assert request.read_count == 126
136+
137+
async def test_readwrite_read_count_above_limit_returns_illegal_value(
138+
self, mock_server_context
139+
):
140+
"""An out-of-range FC23 read count must answer 0x03."""
141+
context = mock_server_context()
142+
context.async_getValues = mock.AsyncMock()
143+
context.async_setValues = mock.AsyncMock()
144+
request = ReadWriteMultipleRegistersRequest(
145+
read_address=1, read_count=126, write_address=1, write_registers=[5]
146+
)
147+
result = await request.datastore_update(context, 1)
148+
assert result.function_code == 0x97
149+
assert result.exception_code == ExcCodes.ILLEGAL_VALUE
150+
context.async_getValues.assert_not_awaited()
151+
context.async_setValues.assert_not_awaited()
152+
153+
async def test_readwrite_read_count_at_max_returns_normal_response(
154+
self, mock_server_context
155+
):
156+
"""A read count exactly at MAX_READ_COUNT must be accepted."""
157+
context = mock_server_context()
158+
request = ReadWriteMultipleRegistersRequest(
159+
read_address=1,
160+
read_count=ReadWriteMultipleRegistersRequest.MAX_READ_COUNT,
161+
write_address=1,
162+
write_registers=[5],
163+
)
164+
result = await request.datastore_update(context, 1)
165+
assert result.function_code == request.function_code
166+
130167
async def test_register_read_requests_count_errors(self, mock_server_context):
131168
"""This tests that the register request messages.
132169
@@ -147,6 +184,67 @@ async def test_register_read_requests_count_errors(self, mock_server_context):
147184
result = await request.datastore_update(context, 0)
148185
assert result.exception_code == ExcCodes.ILLEGAL_VALUE
149186

187+
@pytest.mark.parametrize(
188+
("request_class", "function_code"),
189+
[
190+
(ReadHoldingRegistersRequest, 3),
191+
(ReadInputRegistersRequest, 4),
192+
],
193+
)
194+
async def test_read_register_count_above_max_returns_illegal_value(
195+
self, request_class, function_code, mock_server_context
196+
):
197+
"""An out-of-range read quantity must answer 0x03, not silence."""
198+
context = mock_server_context()
199+
context.async_getValues = mock.AsyncMock(return_value=[1] * 10)
200+
request = request_class(address=0, count=request_class.MAX_COUNT + 1)
201+
result = await request.datastore_update(context, 1)
202+
assert result.function_code == function_code | 0x80
203+
assert result.exception_code == ExcCodes.ILLEGAL_VALUE
204+
context.async_getValues.assert_not_awaited()
205+
206+
@pytest.mark.parametrize(
207+
"request_class",
208+
[ReadHoldingRegistersRequest, ReadInputRegistersRequest],
209+
)
210+
async def test_read_register_count_zero_returns_illegal_value(
211+
self, request_class, mock_server_context
212+
):
213+
"""A zero read quantity must answer 0x03."""
214+
context = mock_server_context()
215+
context.async_getValues = mock.AsyncMock(return_value=[1] * 10)
216+
result = await request_class(address=0, count=0).datastore_update(context, 1)
217+
assert result.function_code == request_class.function_code | 0x80
218+
assert result.exception_code == ExcCodes.ILLEGAL_VALUE
219+
context.async_getValues.assert_not_awaited()
220+
221+
@pytest.mark.parametrize(
222+
"request_class",
223+
[ReadHoldingRegistersRequest, ReadInputRegistersRequest],
224+
)
225+
async def test_read_register_count_at_max_returns_normal_response(
226+
self, request_class, mock_server_context
227+
):
228+
"""A count exactly at the maximum must be accepted, not rejected."""
229+
context = mock_server_context()
230+
request = request_class(address=0, count=request_class.MAX_COUNT)
231+
result = await request.datastore_update(context, 1)
232+
assert result.function_code == request_class.function_code
233+
234+
def test_read_register_decode_accepts_count_above_max(self):
235+
"""Decoding must not raise for an over-max read quantity.
236+
237+
Raising here makes DecodePDU.decode() swallow the frame and the server
238+
answer nothing, replacing a correct exception response with silence.
239+
"""
240+
decoder = DecodePDU(True)
241+
for frame, expected_count in (
242+
(b"\x03\x00\x00\x00\x7e", 126),
243+
(b"\x04\x00\x00\x00\x7e", 126),
244+
):
245+
pdu = decoder.decode(frame)
246+
assert pdu.count == expected_count
247+
150248
async def test_register_read_requests_verify_errors(self, mock_server_context):
151249
"""This tests that the register request messages.
152250

‎test/server/test_server_asyncio.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,14 @@ async def test_async_tcp_server_roundtrip(self):
258258
await asyncio.wait_for(BasicClient.done, timeout=0.1)
259259
assert BasicClient.received_data, expected_response
260260

261+
async def test_async_tcp_server_read_count_above_limit(self):
262+
"""An out-of-range read quantity must answer exception 0x03 on the wire."""
263+
BasicClient.data = b"\x00\x00\x00\x00\x00\x06\x01\x03\x00\x00\x00\x7e"
264+
await self.start_server()
265+
await self.connect_server()
266+
await asyncio.wait_for(BasicClient.done, timeout=0.1)
267+
assert BasicClient.received_data == b"\x00\x00\x00\x00\x00\x03\x01\x83\x03"
268+
261269
async def test_async_server_trace_connect_disconnect(self):
262270
"""Test connect/disconnect trace handler."""
263271
trace_connect = mock.Mock()

0 commit comments

Comments
 (0)