diff --git a/pymodbus/framer/base.py b/pymodbus/framer/base.py index df908d90a..822d47bde 100644 --- a/pymodbus/framer/base.py +++ b/pymodbus/framer/base.py @@ -88,10 +88,6 @@ def handleFrame( ) continue if (pdu := self.decoder.decode(frame_data)) is None: - # Preserve framing identity so the server can echo transaction/dev - # ids on the undecodable-function exception path (see #2990). - # Do not recover a function code from garbage payloads — noise on - # serial lines is the common cause of this path. raise ModbusIOException( "Unable to decode request", transaction_id=tid, diff --git a/pymodbus/pdu/pdu.py b/pymodbus/pdu/pdu.py index b53ef922e..dde76e817 100644 --- a/pymodbus/pdu/pdu.py +++ b/pymodbus/pdu/pdu.py @@ -29,7 +29,11 @@ def __init__( """Initialize the base data for a modbus request.""" self.dev_id: int = dev_id if dev_id > 255: - raise ModbusIOException(f"Invalid ID {dev_id}") + raise ModbusIOException( + f"Invalid ID {dev_id}", + transaction_id=transaction_id, + dev_id=dev_id, + ) self.transaction_id: int = transaction_id self.address: int = address self.bits: list[bool] = bits or [] diff --git a/pymodbus/pdu/register_message.py b/pymodbus/pdu/register_message.py index de18f05ac..3bd13e3f2 100644 --- a/pymodbus/pdu/register_message.py +++ b/pymodbus/pdu/register_message.py @@ -76,7 +76,8 @@ def decode(self, data: bytes) -> None: self.registers = [] if (data_len := int(data[0])) >= len(data): raise ModbusIOException( - f"byte_count {data_len} > length of packet {len(data)}" + f"byte_count {data_len} > length of packet {len(data)}", + function_code=self.function_code, ) for i in range(1, data_len, 2): self.registers.append(struct.unpack(">H", data[i : i + 2])[0]) diff --git a/pymodbus/server/requesthandler.py b/pymodbus/server/requesthandler.py index 170391a1b..ab26b1bb8 100644 --- a/pymodbus/server/requesthandler.py +++ b/pymodbus/server/requesthandler.py @@ -63,17 +63,14 @@ def callback_data(self, data: bytes, addr: tuple | None = None) -> int: try: used_len = super().callback_data(data, addr) except ModbusIOException as exc: - # Undecodable function codes (and frame garbage) land here. last_pdu - # is cleared before framing runs, so identity comes from the framer - # exception attributes when available. Use function code 0x00 rather - # than a hardcoded unrelated value (was 40 / 0x28) — see #2990. + function_code = 0x00 if exc.fcode is None else exc.fcode response = ExceptionResponse( - 0x00, + function_code, exception_code=ExcCodes.ILLEGAL_FUNCTION, device_id=exc.dev_id, transaction=exc.transaction_id, ) - self.server_send(response, 0) + self.server_send(response, addr) return len(data) if self.last_pdu: self.loop.call_soon(self.handle_later) diff --git a/pymodbus/transaction/transaction.py b/pymodbus/transaction/transaction.py index 2863065fc..7a268c029 100644 --- a/pymodbus/transaction/transaction.py +++ b/pymodbus/transaction/transaction.py @@ -120,6 +120,18 @@ def sync_get_response(self, dev_id, tid) -> ModbusPDU: if monotonic() >= deadline: raise asyncio.exceptions.TimeoutError() + @staticmethod + def _io_exception_from_request( + message: str, request: ModbusPDU + ) -> ModbusIOException: + """Build ModbusIOException from an outstanding request.""" + return ModbusIOException( + message, + function_code=request.function_code, + transaction_id=request.transaction_id, + dev_id=request.dev_id, + ) + def sync_execute(self, no_response_expected: bool, request: ModbusPDU) -> ModbusPDU: """Execute requests asynchronously. @@ -143,12 +155,14 @@ def sync_execute(self, no_response_expected: bool, request: ModbusPDU) -> Modbus ) self.count_until_disconnect = self.max_until_disconnect if response.dev_id != request.dev_id: - raise ModbusIOException( - f"ERROR: request uses device id={request.dev_id} but received {response.dev_id}." + raise self._io_exception_from_request( + f"ERROR: request uses device id={request.dev_id} but received {response.dev_id}.", + request, ) if response.transaction_id != request.transaction_id: - raise ModbusIOException( - f"ERROR: request uses transaction id={request.transaction_id} but received {response.transaction_id}." + raise self._io_exception_from_request( + f"ERROR: request uses transaction id={request.transaction_id} but received {response.transaction_id}.", + request, ) response.retries = count_retries return response @@ -156,13 +170,14 @@ def sync_execute(self, no_response_expected: bool, request: ModbusPDU) -> Modbus count_retries += 1 if self.count_until_disconnect < 0: self.connection_lost(asyncio.TimeoutError("Server not responding")) - raise ModbusIOException( - "ERROR: No response received of the last requests (default: retries+3), CLOSING CONNECTION." + raise self._io_exception_from_request( + "ERROR: No response received of the last requests (default: retries+3), CLOSING CONNECTION.", + request, ) self.count_until_disconnect -= 1 txt = f"No response received after {self.retries} retries, continue with next request" Log.error(txt) - raise ModbusIOException(txt) + raise self._io_exception_from_request(txt, request) async def execute( self, no_response_expected: bool, request: ModbusPDU @@ -193,33 +208,37 @@ async def execute( ) self.count_until_disconnect = self.max_until_disconnect if request.dev_id and response.dev_id != request.dev_id: - raise ModbusIOException( - f"ERROR: request uses device id={request.dev_id} but received {response.dev_id}." + raise self._io_exception_from_request( + f"ERROR: request uses device id={request.dev_id} but received {response.dev_id}.", + request, ) if ( response.transaction_id and response.transaction_id != request.transaction_id ): - raise ModbusIOException( - f"ERROR: request uses transaction id={request.transaction_id} but received {response.transaction_id}." + raise self._io_exception_from_request( + f"ERROR: request uses transaction id={request.transaction_id} but received {response.transaction_id}.", + request, ) response.retries = count_retries return response except asyncio.exceptions.TimeoutError: count_retries += 1 except asyncio.exceptions.CancelledError as exc: - raise ModbusIOException( - "Request cancelled outside library." + raise self._io_exception_from_request( + "Request cancelled outside library.", + request, ) from exc if self.count_until_disconnect < 0: self.connection_lost(asyncio.TimeoutError("Server not responding")) - raise ModbusIOException( - "ERROR: No response received of the last requests (default: retries+3), CLOSING CONNECTION." + raise self._io_exception_from_request( + "ERROR: No response received of the last requests (default: retries+3), CLOSING CONNECTION.", + request, ) self.count_until_disconnect -= 1 txt = f"No response received after {self.retries} retries, continue with next request" Log.error(txt) - raise ModbusIOException(txt) + raise self._io_exception_from_request(txt, request) def pdu_send(self, pdu: ModbusPDU, addr: tuple | None = None) -> None: """Build byte stream and send.""" diff --git a/test/client/test_client_faulty_response.py b/test/client/test_client_faulty_response.py index 0e7e2c090..5ee14a31e 100644 --- a/test/client/test_client_faulty_response.py +++ b/test/client/test_client_faulty_response.py @@ -34,8 +34,10 @@ def test_1917_frame(self): def test_faulty_frame1(self, framer): """Test ok frame.""" faulty_frame = b"\x00\x04\x00\x00\x00\x05\x00\x03\x0a\x00\x04" - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: framer.handleFrame(faulty_frame, 0, 0) + assert exc_info.value.transaction_id == 4 + assert exc_info.value.dev_id == 0 used_len, pdu = framer.handleFrame(self.good_frame, 0, 0) assert pdu assert used_len == len(self.good_frame) diff --git a/test/framer/test_extras.py b/test/framer/test_extras.py index 132713186..642133d88 100755 --- a/test/framer/test_extras.py +++ b/test/framer/test_extras.py @@ -84,8 +84,11 @@ def test_tcp_framer_transaction_wrong_tid(self): def test_tcp_framer_transaction_wrong_fc(self): """Test a half completed tcp frame transaction.""" msg = b"\x00\x01\x00\x00\x00\x06\xff\x70\x01\x02\x00\x08" - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: self._tcp.handleFrame(msg, 0, 0) + assert exc_info.value.transaction_id == 1 + assert exc_info.value.dev_id == 0xFF + assert exc_info.value.fcode is None def test_tls_incoming_packet(self): """Framer tls incoming packet.""" diff --git a/test/pdu/test_pdu.py b/test/pdu/test_pdu.py index 7aeb7d61f..d0f796d13 100644 --- a/test/pdu/test_pdu.py +++ b/test/pdu/test_pdu.py @@ -37,8 +37,10 @@ async def test_get_pdu_size(self): async def test_pdu_id(self): """Test set illegal pdu id.""" - with pytest.raises(ModbusIOException): - ModbusPDU(256) + with pytest.raises(ModbusIOException) as exc_info: + ModbusPDU(256, transaction_id=0x42) + assert exc_info.value.dev_id == 256 + assert exc_info.value.transaction_id == 0x42 async def test_is_error(self): """Test is_error.""" diff --git a/test/pdu/test_register_read_messages.py b/test/pdu/test_register_read_messages.py index fe4ffb154..0d04056e4 100644 --- a/test/pdu/test_register_read_messages.py +++ b/test/pdu/test_register_read_messages.py @@ -80,8 +80,9 @@ def test_register_read_response_decode(self): def test_register_read_response_decode_error(self): """Test register read response.""" reg = ReadHoldingRegistersResponse(count=5) - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: reg.decode(b"\x14\x00\x03\x00\x11") + assert exc_info.value.fcode == reg.function_code async def test_register_read_requests_count_errors(self, mock_server_context): """This tests that the register request messages. diff --git a/test/server/test_requesthandler.py b/test/server/test_requesthandler.py index 668f60c89..4ab50b62c 100755 --- a/test/server/test_requesthandler.py +++ b/test/server/test_requesthandler.py @@ -55,6 +55,7 @@ async def test_rh_callback_data(self, requesthandler): async def test_rh_callback_data_undecodable_echoes_identity(self, requesthandler): """Undecodable FC exception must echo framing tid/dev_id (#2990).""" + peer = ("192.0.2.1", 5020) with mock.patch( "pymodbus.transaction.TransactionManager.callback_data" ) as cb_data: @@ -65,16 +66,16 @@ async def test_rh_callback_data_undecodable_echoes_identity(self, requesthandler ) cb_data.side_effect = exc data = b"\x00\x0a\x00\x00\x00\x06\x07\x0a\x00\x00\x00\x01" - assert len(data) == requesthandler.callback_data(data, None) + assert len(data) == requesthandler.callback_data(data, peer) requesthandler.pdu_send.assert_called_once() response = requesthandler.pdu_send.call_args.args[0] assert isinstance(response, ExceptionResponse) assert response.transaction_id == 0x000A assert response.dev_id == 7 - # 0x00 | 0x80 — not the previous hardcoded 0x28 | 0x80 assert response.function_code == 0x80 - assert response.exception_code == 0x01 # ILLEGAL_FUNCTION + assert response.exception_code == 0x01 + assert requesthandler.pdu_send.call_args.kwargs.get("addr") == peer async def test_rh_callback_data_undecodable_without_framing_attrs( self, requesthandler diff --git a/test/transaction/test_transaction.py b/test/transaction/test_transaction.py index 488376414..03f05497c 100755 --- a/test/transaction/test_transaction.py +++ b/test/transaction/test_transaction.py @@ -223,14 +223,19 @@ async def test_transaction_execute(self, use_clc, scenario): elif scenario == 3: # wait receive,timeout, no_responses transact.comm_params.timeout_connect = 0.1 transact.connection_lost = mock.Mock() # type: ignore[method-assign] - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: await transact.execute(False, request) + assert exc_info.value.fcode == request.function_code + assert exc_info.value.dev_id == request.dev_id + assert exc_info.value.transaction_id == request.transaction_id elif scenario == 4: # wait receive,timeout, disconnect transact.comm_params.timeout_connect = 0.1 transact.count_until_disconnect = -1 transact.connection_lost = mock.Mock() # type: ignore[method-assign] - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: await transact.execute(False, request) + assert exc_info.value.fcode == request.function_code + assert exc_info.value.transaction_id == request.transaction_id elif scenario == 5: # wait receive,timeout, no_responses pass transact.comm_params.timeout_connect = 0.1 transact.connection_lost = mock.Mock() # type: ignore[method-assign] @@ -242,8 +247,10 @@ async def test_transaction_execute(self, use_clc, scenario): await asyncio.sleep(0.1) resp.cancel() await asyncio.sleep(0.1) - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: await resp + assert exc_info.value.fcode == request.function_code + assert exc_info.value.dev_id == request.dev_id elif scenario == 7: # response transact.comm_params.timeout_connect = 0.2 resp = asyncio.create_task(transact.execute(False, request)) @@ -259,8 +266,11 @@ async def test_transaction_execute(self, use_clc, scenario): new_resp.dev_id = 17 transact.response_future.set_result(new_resp) await asyncio.sleep(0.1) - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: resp.result() + assert exc_info.value.fcode == request.function_code + assert exc_info.value.dev_id == request.dev_id + assert exc_info.value.transaction_id == request.transaction_id else: # if scenario == 9: # response wrong tid transact.comm_params.timeout_connect = 0.2 resp = asyncio.create_task(transact.execute(False, request)) @@ -269,8 +279,11 @@ async def test_transaction_execute(self, use_clc, scenario): new_resp.transaction_id = 17 transact.response_future.set_result(new_resp) await asyncio.sleep(0.1) - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: resp.result() + assert exc_info.value.fcode == request.function_code + assert exc_info.value.dev_id == request.dev_id + assert exc_info.value.transaction_id == request.transaction_id async def test_transaction_receiver(self, use_clc): """Test tracers in disconnect.""" @@ -449,13 +462,18 @@ async def test_sync_transaction_execute(self, use_clc, scenario): ) elif scenario == 3: # wait receive,timeout, no_responses transact.comm_params.timeout_connect = 0.1 - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: transact.sync_execute(False, request) + assert exc_info.value.fcode == request.function_code + assert exc_info.value.dev_id == request.dev_id + assert exc_info.value.transaction_id == request.transaction_id elif scenario == 4: # wait receive,timeout, disconnect transact.comm_params.timeout_connect = 0.1 transact.count_until_disconnect = -1 - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: transact.sync_execute(False, request) + assert exc_info.value.fcode == request.function_code + assert exc_info.value.transaction_id == request.transaction_id elif scenario == 5: # wait receive,timeout, no_responses pass transact.comm_params.timeout_connect = 0.1 with pytest.raises(ModbusIOException): @@ -475,8 +493,11 @@ async def test_sync_transaction_execute(self, use_clc, scenario): transact.sync_get_response = mock.Mock(return_value=pdu) # type: ignore[method-assign] transact.pdu_send = mock.Mock() # type: ignore[method-assign] transact.comm_params.timeout_connect = 0.2 - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: transact.sync_execute(False, request) + assert exc_info.value.fcode == request.function_code + assert exc_info.value.dev_id == request.dev_id + assert exc_info.value.transaction_id == request.transaction_id elif scenario == 8: # response wrong tid transact.transport = 1 # type: ignore[assignment] pdu = copy.deepcopy(response) @@ -484,8 +505,11 @@ async def test_sync_transaction_execute(self, use_clc, scenario): transact.sync_get_response = mock.Mock(return_value=pdu) # type: ignore[method-assign] transact.pdu_send = mock.Mock() # type: ignore[method-assign] transact.comm_params.timeout_connect = 0.2 - with pytest.raises(ModbusIOException): + with pytest.raises(ModbusIOException) as exc_info: transact.sync_execute(False, request) + assert exc_info.value.fcode == request.function_code + assert exc_info.value.dev_id == request.dev_id + assert exc_info.value.transaction_id == request.transaction_id else: # if scenario == 9 # pdu_send from client transact.transport = 1 # type: ignore[assignment] transact.is_server = True