Skip to content

ensure unpacking of proper length during decoding (#2664) #2665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
janiversen merged 2 commits into from
May 9, 2025
Merged

ensure unpacking of proper length during decoding (#2664) #2665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4a474c
ensure unpacking of proper length during decoding (#2664)
loi-opfel May 9, 2025
4699c35
Test case.
janiversen May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pymodbus/pdu/bit_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode a request pdu."""
self.address, self.count = struct.unpack(">HH", data)
self.address, self.count = struct.unpack(">HH", data[:4])
Copy link
Copy Markdown
Collaborator

@janiversen janiversen May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really confused, how can this help with the exception ???

The buffer needs to be >= 4, so the exception arises when the buffer is less than 4 bytes.

Copy link
Copy Markdown
Contributor Author

@brherger brherger May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no way of knowing what is coming in, not logged in the exception. It will throw an exception for != 4.
This is also done elsewhere in these files.

>>> data =  b"\x11\x22\x33\x44\x55\x66"
>>> struct.unpack(">HH", data)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
struct.error: unpack requires a buffer of 4 bytes
>>> struct.unpack(">HH", data[:3])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
struct.error: unpack requires a buffer of 4 bytes
>>> struct.unpack(">HH", data[:4])
(4386, 13124)


def get_response_pdu_size(self) -> int:
"""Get response pdu size.
Expand Down Expand Up @@ -84,7 +84,7 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode a write coil request."""
self.address, value = struct.unpack(">HH", data)
self.address, value = struct.unpack(">HH", data[:4])
self.bits = [bool(value)]


Expand Down Expand Up @@ -157,4 +157,4 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode a write coils response."""
self.address, self.count = struct.unpack(">HH", data)
self.address, self.count = struct.unpack(">HH", data[:4])
4 changes: 2 additions & 2 deletions pymodbus/pdu/diag_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ def decode(self, data: bytes) -> None:
data_len += 1
data += b"0"
if (word_len := data_len // 2) == 1:
(self.message,) = struct.unpack(">H", data)
(self.message,) = struct.unpack(">H", data[:2])
else:
self.message = struct.unpack(">" + "H" * word_len, data)
self.message = struct.unpack(">" + "H" * word_len, data[:2 * word_len])

def get_response_pdu_size(self) -> int:
"""Get response pdu size.
Expand Down
2 changes: 1 addition & 1 deletion pymodbus/pdu/file_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode the incoming request."""
self.address = struct.unpack(">H", data)[0]
self.address = struct.unpack(">H", data[:2])[0]

async def update_datastore(self, _context: ModbusDeviceContext) -> ModbusPDU:
"""Run a read exception status request against the store."""
Expand Down
2 changes: 1 addition & 1 deletion pymodbus/pdu/mei_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode data part of the message."""
self.sub_function_code, self.read_code, self.object_id = struct.unpack(">BBB", data)
self.sub_function_code, self.read_code, self.object_id = struct.unpack(">BBB", data[:3])

async def update_datastore(self, _context: ModbusDeviceContext) -> ModbusPDU:
"""Run a read exception status request against the store."""
Expand Down
2 changes: 1 addition & 1 deletion pymodbus/pdu/other_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode a the response."""
ready, self.count = struct.unpack(">HH", data)
ready, self.count = struct.unpack(">HH", data[:4])
self.status = ready == ModbusStatus.READY


Expand Down
116 changes: 87 additions & 29 deletions pymodbus/pdu/register_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Register Reading Request/Response."""

from __future__ import annotations

import struct
Expand All @@ -24,7 +25,7 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode a register request packet."""
self.address, self.count = struct.unpack(">HH", data)
self.address, self.count = struct.unpack(">HH", data[:4])

def get_response_pdu_size(self) -> int:
"""Get response pdu size.
Expand All @@ -38,8 +39,16 @@ async def update_datastore(self, context: ModbusDeviceContext) -> ModbusPDU:
values = await context.async_getValues(
self.function_code, self.address, self.count
)
response_class = (ReadHoldingRegistersResponse if self.function_code == 3 else ReadInputRegistersResponse)
return response_class(registers=cast(list[int], values), dev_id=self.dev_id, transaction_id=self.transaction_id)
response_class = (
ReadHoldingRegistersResponse
if self.function_code == 3
else ReadInputRegistersResponse
)
return response_class(
registers=cast(list[int], values),
dev_id=self.dev_id,
transaction_id=self.transaction_id,
)


class ReadHoldingRegistersResponse(ModbusPDU):
Expand All @@ -59,7 +68,9 @@ def decode(self, data: bytes) -> None:
"""Decode a register response packet."""
self.registers = []
if (data_len := int(data[0])) >= len(data):
raise ModbusIOException(f"byte_count {data_len} > length of packet {len(data)}")
raise ModbusIOException(
f"byte_count {data_len} > length of packet {len(data)}"
)
for i in range(1, data_len, 2):
self.registers.append(struct.unpack(">H", data[i : i + 2])[0])

Expand All @@ -82,13 +93,15 @@ class ReadWriteMultipleRegistersRequest(ModbusPDU):
function_code = 23
rtu_byte_count_pos = 10

def __init__(self,
read_address: int = 0x00,
read_count: int = 0,
write_address: int = 0x00,
write_registers: list[int] | None = None,
dev_id: int = 1,
transaction_id: int = 0) -> None:
def __init__(
self,
read_address: int = 0x00,
read_count: int = 0,
write_address: int = 0x00,
write_registers: list[int] | None = None,
dev_id: int = 1,
transaction_id: int = 0,
) -> None:
"""Initialize a new request message."""
if not write_registers:
write_registers = []
Expand Down Expand Up @@ -135,9 +148,13 @@ def decode(self, data: bytes) -> None:
async def update_datastore(self, context: ModbusDeviceContext) -> ModbusPDU:
"""Run a write single register request against a datastore."""
if not (1 <= self.read_count <= 0x07D):
return ExceptionResponse(self.function_code, ExceptionResponse.ILLEGAL_VALUE)
return ExceptionResponse(
self.function_code, ExceptionResponse.ILLEGAL_VALUE
)
if not 1 <= self.write_count <= 0x079:
return ExceptionResponse(self.function_code, ExceptionResponse.ILLEGAL_VALUE)
return ExceptionResponse(
self.function_code, ExceptionResponse.ILLEGAL_VALUE
)
rc = await context.async_setValues(
self.function_code, self.write_address, self.write_registers
)
Expand All @@ -146,7 +163,11 @@ async def update_datastore(self, context: ModbusDeviceContext) -> ModbusPDU:
registers = await context.async_getValues(
self.function_code, self.read_address, self.read_count
)
return ReadWriteMultipleRegistersResponse(registers=cast(list[int], registers), dev_id=self.dev_id, transaction_id=self.transaction_id)
return ReadWriteMultipleRegistersResponse(
registers=cast(list[int], registers),
dev_id=self.dev_id,
transaction_id=self.transaction_id,
)

def get_response_pdu_size(self) -> int:
"""Get response pdu size.
Expand Down Expand Up @@ -174,7 +195,7 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode a write single register packet packet request."""
self.address, register = struct.unpack(">HH", data)
self.address, register = struct.unpack(">HH", data[:4])
self.registers = [register]


Expand All @@ -184,14 +205,18 @@ class WriteSingleRegisterRequest(WriteSingleRegisterResponse):
async def update_datastore(self, context: ModbusDeviceContext) -> ModbusPDU:
"""Run a write single register request against a datastore."""
if not 0 <= self.registers[0] <= 0xFFFF:
return ExceptionResponse(self.function_code, ExceptionResponse.ILLEGAL_VALUE)
return ExceptionResponse(
self.function_code, ExceptionResponse.ILLEGAL_VALUE
)
rc = await context.async_setValues(
self.function_code, self.address, self.registers
)
if rc:
return ExceptionResponse(self.function_code, rc)
values = await context.async_getValues(self.function_code, self.address, 1)
return WriteSingleRegisterResponse(address=self.address, registers=cast(list[int], values))
return WriteSingleRegisterResponse(
address=self.address, registers=cast(list[int], values)
)

def get_response_pdu_size(self) -> int:
"""Get response pdu size.
Expand Down Expand Up @@ -225,13 +250,20 @@ def decode(self, data: bytes) -> None:
async def update_datastore(self, context: ModbusDeviceContext) -> ModbusPDU:
"""Run a write single register request against a datastore."""
if not 1 <= self.count <= 0x07B:
return ExceptionResponse(self.function_code, ExceptionResponse.ILLEGAL_VALUE)
return ExceptionResponse(
self.function_code, ExceptionResponse.ILLEGAL_VALUE
)
rc = await context.async_setValues(
self.function_code, self.address, self.registers
)
)
if rc:
return ExceptionResponse(self.function_code, rc)
return WriteMultipleRegistersResponse(address=self.address, count=self.count, dev_id=self.dev_id, transaction_id=self.transaction_id)
return WriteMultipleRegistersResponse(
address=self.address,
count=self.count,
dev_id=self.dev_id,
transaction_id=self.transaction_id,
)

def get_response_pdu_size(self) -> int:
"""Get response pdu size.
Expand All @@ -253,7 +285,7 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode a write single register packet packet request."""
self.address, self.count = struct.unpack(">HH", data)
self.address, self.count = struct.unpack(">HH", data[:4])


class MaskWriteRegisterRequest(ModbusPDU):
Expand All @@ -262,7 +294,14 @@ class MaskWriteRegisterRequest(ModbusPDU):
function_code = 0x16
rtu_frame_size = 10

def __init__(self, address=0x0000, and_mask=0xFFFF, or_mask=0x0000, dev_id=1, transaction_id=0) -> None:
def __init__(
self,
address=0x0000,
and_mask=0xFFFF,
or_mask=0x0000,
dev_id=1,
transaction_id=0,
) -> None:
"""Initialize a new instance."""
super().__init__(transaction_id=transaction_id, dev_id=dev_id, address=address)
self.and_mask = and_mask
Expand All @@ -274,22 +313,34 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode the incoming request."""
self.address, self.and_mask, self.or_mask = struct.unpack(">HHH", data)
self.address, self.and_mask, self.or_mask = struct.unpack(">HHH", data[:6])

async def update_datastore(self, context: ModbusDeviceContext) -> ModbusPDU:
"""Run a mask write register request against the store."""
if not 0x0000 <= self.and_mask <= 0xFFFF:
return ExceptionResponse(self.function_code, ExceptionResponse.ILLEGAL_VALUE)
return ExceptionResponse(
self.function_code, ExceptionResponse.ILLEGAL_VALUE
)
if not 0x0000 <= self.or_mask <= 0xFFFF:
return ExceptionResponse(self.function_code, ExceptionResponse.ILLEGAL_VALUE)
return ExceptionResponse(
self.function_code, ExceptionResponse.ILLEGAL_VALUE
)
values = await context.async_getValues(self.function_code, self.address, 1)
values = (cast(Sequence[int | bool], values)[0] & self.and_mask) | (self.or_mask & ~self.and_mask)
values = (cast(Sequence[int | bool], values)[0] & self.and_mask) | (
self.or_mask & ~self.and_mask
)
rc = await context.async_setValues(
self.function_code, self.address, cast(list[int], [values])
)
if rc:
return ExceptionResponse(self.function_code, rc)
return MaskWriteRegisterResponse(address=self.address, and_mask=self.and_mask, or_mask=self.or_mask, dev_id=self.dev_id, transaction_id=self.transaction_id)
return MaskWriteRegisterResponse(
address=self.address,
and_mask=self.and_mask,
or_mask=self.or_mask,
dev_id=self.dev_id,
transaction_id=self.transaction_id,
)


class MaskWriteRegisterResponse(ModbusPDU):
Expand All @@ -298,7 +349,14 @@ class MaskWriteRegisterResponse(ModbusPDU):
function_code = 0x16
rtu_frame_size = 10

def __init__(self, address=0x0000, and_mask=0xFFFF, or_mask=0x0000, dev_id=1, transaction_id=0) -> None:
def __init__(
self,
address=0x0000,
and_mask=0xFFFF,
or_mask=0x0000,
dev_id=1,
transaction_id=0,
) -> None:
"""Initialize new instance."""
super().__init__(transaction_id=transaction_id, dev_id=dev_id, address=address)
self.and_mask = and_mask
Expand All @@ -311,4 +369,4 @@ def encode(self) -> bytes:

def decode(self, data: bytes) -> None:
"""Decode a the response."""
self.address, self.and_mask, self.or_mask = struct.unpack(">HHH", data)
self.address, self.and_mask, self.or_mask = struct.unpack(">HHH", data[:6])
8 changes: 8 additions & 0 deletions test/framer/test_framer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ def test_roundtrip_CRC(self):
assert FramerRTU.compute_CRC(data) == 0xE2DB
assert FramerRTU.check_CRC(data, 0xE2DB)

async def test_handleFrame2(self):
"""Test handleFrame."""
test_framer = FramerRTU(DecodePDU(True))
msg = b"\xfe\x04\x00\x03\x00\x01\xd5\xc5\x00"
used_len, pdu = test_framer.handleFrame(msg, 0, 0)
assert used_len == len(msg)
assert pdu


class TestFramerType:
"""Test classes."""
Expand Down