From cbbbd74ba2059cf0cda01d65ad44dac97491483c Mon Sep 17 00:00:00 2001 From: StalderT Date: Mon, 12 Oct 2026 12:19:29 +0200 Subject: [PATCH 1/3] Frame Modbus TCP requests by their MBAP length On the server side, _modbus_receive_msg() computed the length of a TCP request from its function code and never looked at the MBAP header. An unknown function code was read as 8 octets and the rest of its ADU stayed in the socket, a request whose MBAP length did not match its function code was read too short or into the next request, and a write followed by octets its byte count doesn't cover was executed, the extra octets being parsed as the next request (#867). The protocol identifier wasn't checked either. response_exception() slept for the response timeout, 500 ms by default, then flushed the socket to discard such leftovers: any malformed request held the server for that long, which an unauthenticated peer can repeat at will (#874), and the flush also discarded any pipelined request behind it. Over TCP, a request is now read exactly up to its MBAP length, whatever its function code, and a protocol identifier other than 0 or a length below 2 or beyond the maximum ADU size is rejected with EMBBADDATA. Where the next request starts can't be known after such a header, so the modbus_receive() documentation advises closing the connection on that error. modbus_reply() answers a request whose length differs from what its function code requires with an illegal data value exception, a longer one included, so nothing of a malformed request is executed. Nothing is left to discard over TCP, so response_exception() neither sleeps nor flushes there; over RTU it still does, the delay letting the rest of a corrupted frame arrive. The unknown function code test uses a response timeout shorter than the server's over TCP. An unknown function code followed by a valid read, in one segment and cut after the function code, must get both answers in order. A write with two octets more than its byte count must be rejected and leave the register unchanged. A read with protocol identifier 1 must not be answered, and the test server closes the connection. Refs #874, #867 --- docs/modbus_receive.md | 7 ++ src/modbus.c | 80 ++++++++++++++++--- tests/unit-test-client.c | 164 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 237 insertions(+), 14 deletions(-) diff --git a/docs/modbus_receive.md b/docs/modbus_receive.md index 8339e3bcf..c35a6bb69 100644 --- a/docs/modbus_receive.md +++ b/docs/modbus_receive.md @@ -26,6 +26,13 @@ length if successful. The returned request length can be zero if the indication request is ignored (eg. a query for another slave in RTU mode). Otherwise it shall return -1 and set errno. +## Errors + +- *EMBBADDATA*, over TCP: the MBAP header of the request is invalid (protocol + identifier other than 0, length below 2 or beyond the maximum ADU size). Where + the next request starts can't be known, so the connection should be closed + rather than read further. + ## Buffer size The maximum size of the request depends on the used backend, in RTU the `req` diff --git a/src/modbus.c b/src/modbus.c index abdd718c4..dbc86432d 100644 --- a/src/modbus.c +++ b/src/modbus.c @@ -506,6 +506,33 @@ int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type) if (length_to_read == 0) { switch (step) { case _STEP_FUNCTION: + if (msg_type == MSG_INDICATION && + ctx->backend->backend_type == _MODBUS_BACKEND_TYPE_TCP) { + /* Over TCP, a request ends where the length field of the MBAP + header (bytes 4 and 5) says, whatever its function code. It + counts the unit identifier and the function code, already + read. Reading exactly that much leaves the next request + intact in the socket, so it is neither lost nor parsed from + its middle. */ + const int remaining = ((msg[4] << 8) + msg[5]) - 2; + + /* A protocol identifier other than 0 is not Modbus, or a + header read where no request starts */ + if (msg_type == MSG_INDICATION && (msg[2] != 0 || msg[3] != 0)) { + errno = EMBBADDATA; + _error_print(ctx, "invalid protocol identifier"); + return -1; + } + if (remaining < 0 || + msg_length + remaining > (int) ctx->backend->max_adu_length) { + errno = EMBBADDATA; + _error_print(ctx, "invalid MBAP length"); + return -1; + } + length_to_read = remaining; + step = _STEP_DATA; + break; + } /* Function code position */ length_to_read = compute_meta_length_after_function( msg[ctx->backend->header_length], msg_type); @@ -779,8 +806,10 @@ static int response_exception(modbus_t *ctx, va_end(ap); } - /* Flush if required */ - if (to_flush) { + /* Flush if required. Over TCP there is nothing to discard: the request has + been read up to the length given by its MBAP header, and what is left in + the socket belongs to the next request. */ + if (to_flush && ctx->backend->backend_type == _MODBUS_BACKEND_TYPE_RTU) { _sleep_response_timeout(ctx); modbus_flush(ctx); } @@ -793,6 +822,28 @@ static int response_exception(modbus_t *ctx, return rsp_length; } +/* Function codes modbus_reply() answers other than by an illegal function + exception */ +static int is_handled_function(int function) +{ + switch (function) { + case MODBUS_FC_READ_COILS: + case MODBUS_FC_READ_DISCRETE_INPUTS: + case MODBUS_FC_READ_HOLDING_REGISTERS: + case MODBUS_FC_READ_INPUT_REGISTERS: + case MODBUS_FC_WRITE_SINGLE_COIL: + case MODBUS_FC_WRITE_SINGLE_REGISTER: + case MODBUS_FC_WRITE_MULTIPLE_COILS: + case MODBUS_FC_WRITE_MULTIPLE_REGISTERS: + case MODBUS_FC_REPORT_SLAVE_ID: + case MODBUS_FC_MASK_WRITE_REGISTER: + case MODBUS_FC_WRITE_AND_READ_REGISTERS: + return TRUE; + default: + return FALSE; + } +} + /* Send a response to the received request. Analyses the request and constructs a response. @@ -813,6 +864,7 @@ int modbus_reply(modbus_t *ctx, sft_t sft; uint8_t meta_length; int data_length; + int expected_length; if (ctx == NULL || req == NULL || mb_mapping == NULL) { errno = EINVAL; @@ -845,11 +897,14 @@ int modbus_reply(modbus_t *ctx, sft.function = function; sft.t_id = ctx->backend->get_response_tid(req); - /* Ensure the request is long enough to contain the meta fields (address, - quantity, byte count...) and the declared data read by the handlers - below. On the canonical modbus_receive() path the framing layer already - guarantees a complete PDU; this check defends direct callers that pass a - truncated or untrusted request to modbus_reply(). */ + /* Ensure the request has the length its function code requires: the meta + fields (address, quantity, byte count...) and the declared data read by + the handlers below. Over TCP, modbus_receive() frames the request by its + MBAP length, which can differ from it; this check also defends direct + callers that pass a truncated or untrusted request to modbus_reply(). + A longer request is rejected too, rather than executed without its extra + octets; only a truncated one may have more octets to flush. Function + codes not handled below get an illegal function exception instead. */ meta_length = compute_meta_length_after_function(function, MSG_INDICATION); data_length = 0; if (req_length >= (int) (offset + 1 + meta_length)) { @@ -857,15 +912,18 @@ int modbus_reply(modbus_t *ctx, declared data length can be read safely. */ data_length = compute_data_length_after_meta(ctx, (uint8_t *) req, MSG_INDICATION); } - if (req_length < (int) (offset + 1 + meta_length + data_length)) { + expected_length = offset + 1 + meta_length + data_length; + if (req_length < expected_length || + (req_length > expected_length && is_handled_function(function))) { rsp_length = response_exception(ctx, &sft, MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, rsp, - TRUE, - "Truncated request (length %d) for function " - "0x%0X in modbus_reply\n", + req_length < expected_length, + "Invalid request length %d (%d expected) for " + "function 0x%0X in modbus_reply\n", req_length, + expected_length, function); /* Suppress responses to broadcasts in RTU, see below. */ if (ctx->backend->backend_type == _MODBUS_BACKEND_TYPE_RTU && diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index c1542e971..8d62f8423 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -11,6 +11,19 @@ #include #include +// clang-format off +#ifdef _WIN32 +# include +#else +# include +#endif + +/* For MinGW */ +#ifndef MSG_NOSIGNAL +# define MSG_NOSIGNAL 0 +#endif +// clang-format on + #include "unit-test.h" const int EXCEPTION_RC = 2; @@ -896,8 +909,8 @@ int test_server(modbus_t *ctx, int use_backend) uint32_t old_response_to_usec; /* This requests can generate flushes server side so we need a higher - * response timeout than the server. The server uses the defined response - * timeout to sleep before flushing. + * response timeout than the server. Over RTU, the server uses the defined + * response timeout to sleep before flushing. * The old timeouts are restored at the end. */ modbus_get_response_timeout(ctx, &old_response_to_sec, &old_response_to_usec); @@ -990,7 +1003,11 @@ int test_server(modbus_t *ctx, int use_backend) if (rc == -1) goto close; - /* Test invalid function code */ + /* Test invalid function code. Over TCP the server must answer without + waiting for its response timeout, so a shorter one is enough here. */ + if (use_backend != RTU) { + modbus_set_response_timeout(ctx, 0, 100000); + } modbus_send_raw_request( ctx, invalid_fc_raw_req, INVALID_FC_REQ_LEN * sizeof(uint8_t)); rc = modbus_receive_confirmation(ctx, rsp); @@ -999,6 +1016,147 @@ int test_server(modbus_t *ctx, int use_backend) rsp[backend_offset] == (0x80 + INVALID_FC), "") + if (use_backend != RTU) { + /* An unknown function code whose MBAP header announces 4 more octets, + immediately followed by a valid read. The server must answer both, in + order, whether the two requests arrive together or the first one is + cut after its function code. */ + uint8_t pipelined_req[] = {/* TID 9, unknown function code */ + 0x00, + 0x09, + 0x00, + 0x00, + 0x00, + 0x06, + slave, + INVALID_FC, + 0x00, + 0x00, + 0x00, + 0x02, + /* TID 10, read one register */ + 0x00, + 0x0A, + 0x00, + 0x00, + 0x00, + 0x06, + slave, + MODBUS_FC_READ_HOLDING_REGISTERS, + UT_REGISTERS_ADDRESS >> 8, + UT_REGISTERS_ADDRESS & 0xFF, + 0x00, + 0x01}; + /* MBAP header and function code of the first request */ + const int cut = 8; + int s = modbus_get_socket(ctx); + int j; + + for (j = 0; j < 2; j++) { + if (j == 0) { + printf("* pipelined requests after an unknown function code: "); + rc = send( + s, (const char *) pipelined_req, sizeof(pipelined_req), MSG_NOSIGNAL); + } else { + printf("* unknown function code cut after the function code: "); + rc = send(s, (const char *) pipelined_req, cut, MSG_NOSIGNAL); + if (rc == cut) { + usleep(50000); + rc = send(s, + (const char *) pipelined_req + cut, + sizeof(pipelined_req) - cut, + MSG_NOSIGNAL); + } + } + ASSERT_TRUE(rc > 0, "FAILED (send %d)\n", rc); + + rc = modbus_receive_confirmation(ctx, rsp); + printf(" exception to TID 9: "); + ASSERT_TRUE(rc == (backend_length + EXCEPTION_RC) && rsp[1] == 0x09 && + rsp[backend_offset] == (0x80 + INVALID_FC), + "FAILED (%d)\n", + rc); + + rc = modbus_receive_confirmation(ctx, rsp); + printf(" response to TID 10: "); + ASSERT_TRUE(rc == (backend_length + 4) && rsp[1] == 0x0A && + rsp[backend_offset] == MODBUS_FC_READ_HOLDING_REGISTERS, + "FAILED (%d)\n", + rc); + } + + /* A write whose MBAP length covers two octets more than its byte + count: rejected as a whole, the register is left unchanged */ + uint16_t before; + uint16_t after; + + rc = modbus_read_registers(ctx, UT_REGISTERS_ADDRESS, 1, &before); + ASSERT_TRUE(rc == 1, "FAILED (read %d)\n", rc); + { + uint8_t long_write_req[] = {0x00, + 0x0B, + 0x00, + 0x00, + 0x00, + 0x0B, + slave, + MODBUS_FC_WRITE_MULTIPLE_REGISTERS, + UT_REGISTERS_ADDRESS >> 8, + UT_REGISTERS_ADDRESS & 0xFF, + 0x00, + 0x01, + 0x02, + (uint8_t) ~(before >> 8), + (uint8_t) ~(before & 0xFF), + 0xAA, + 0xBB}; + + rc = send(s, + (const char *) long_write_req, + sizeof(long_write_req), + MSG_NOSIGNAL); + ASSERT_TRUE(rc > 0, "FAILED (send %d)\n", rc); + } + rc = modbus_receive_confirmation(ctx, rsp); + printf("* write longer than its byte count rejected: "); + ASSERT_TRUE(rc == (backend_length + EXCEPTION_RC) && rsp[1] == 0x0B && + rsp[backend_offset] == + (0x80 + MODBUS_FC_WRITE_MULTIPLE_REGISTERS) && + rsp[backend_offset + 1] == MODBUS_EXCEPTION_ILLEGAL_DATA_VALUE, + "FAILED (%d)\n", + rc); + + rc = modbus_read_registers(ctx, UT_REGISTERS_ADDRESS, 1, &after); + printf(" register unchanged: "); + ASSERT_TRUE(rc == 1 && after == before, "FAILED (%d, 0x%X)\n", rc, after); + + /* Last test on this connection: a read with protocol identifier 1 is + not executed, and the test server closes the connection on the + error, as advised for EMBBADDATA */ + { + uint8_t bad_pid_req[] = {0x00, + 0x0C, + 0x00, + 0x01, + 0x00, + 0x06, + slave, + MODBUS_FC_READ_HOLDING_REGISTERS, + UT_REGISTERS_ADDRESS >> 8, + UT_REGISTERS_ADDRESS & 0xFF, + 0x00, + 0x01}; + + rc = send( + s, (const char *) bad_pid_req, sizeof(bad_pid_req), MSG_NOSIGNAL); + ASSERT_TRUE(rc > 0, "FAILED (send %d)\n", rc); + } + modbus_set_error_recovery(ctx, MODBUS_ERROR_RECOVERY_NONE); + rc = modbus_receive_confirmation(ctx, rsp); + printf("* request with protocol identifier 1 rejected: "); + ASSERT_TRUE(rc == -1 && errno == ECONNRESET, "FAILED (%d)\n", rc); + } + modbus_set_response_timeout(ctx, old_response_to_sec, old_response_to_usec); return 0; close: From 51e0e266d37fe0c2885322bc44eded6348c35b7c Mon Sep 17 00:00:00 2001 From: StalderT Date: Mon, 12 Oct 2026 12:21:08 +0200 Subject: [PATCH 2/3] Don't read the octets a server adds after its TCP response as the next one A client read a TCP response by its function code and ignored the MBAP length. Octets a server announced and sent beyond its response stayed in the socket and were read as the start of the next response, so a faulty or compromised device could forge the response to the next request: the transaction ID is predictable, and behind a gateway shared by several devices, the next request can target another one. The response of a function code unknown to libmodbus, sent with modbus_send_raw_request(), was cut after one octet, and its remaining octets desynchronized the connection. A response whose layout libmodbus knows is still read by its content, since some gateways announce a longer MBAP length than they send, or even a meaningless one, and refusing them or waiting for octets that never come would break them. When the MBAP length is valid, the octets it announces beyond the response are discarded, as far as they have already been received, without waiting; an invalid one is ignored, as before. The response of another function code is read up to its MBAP length. modbus_proxy() with a TCP backend gets the same. The test server answers a read at three new special addresses with an MBAP length two octets too long, followed by them or not, or with an MBAP length of 0. The test client expects the response in each case, then a valid response to its next request. --- src/modbus.c | 104 +++++++++++++++++++++++++++++++++------ tests/unit-test-client.c | 23 +++++++++ tests/unit-test-server.c | 30 +++++++++++ tests/unit-test.h.in | 7 +++ 4 files changed, 148 insertions(+), 16 deletions(-) diff --git a/src/modbus.c b/src/modbus.c index dbc86432d..33a1be990 100644 --- a/src/modbus.c +++ b/src/modbus.c @@ -350,6 +350,59 @@ compute_data_length_after_meta(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type) return length; } +/* Function codes whose response layout libmodbus knows, exceptions included */ +static int is_known_response(int function) +{ + if (function >= 0x80) { + return TRUE; + } + + switch (function) { + case MODBUS_FC_READ_COILS: + case MODBUS_FC_READ_DISCRETE_INPUTS: + case MODBUS_FC_READ_HOLDING_REGISTERS: + case MODBUS_FC_READ_INPUT_REGISTERS: + case MODBUS_FC_WRITE_SINGLE_COIL: + case MODBUS_FC_WRITE_SINGLE_REGISTER: + case MODBUS_FC_READ_EXCEPTION_STATUS: + case MODBUS_FC_WRITE_MULTIPLE_COILS: + case MODBUS_FC_WRITE_MULTIPLE_REGISTERS: + case MODBUS_FC_REPORT_SLAVE_ID: + case MODBUS_FC_MASK_WRITE_REGISTER: + case MODBUS_FC_WRITE_AND_READ_REGISTERS: + return TRUE; + default: + return FALSE; + } +} + +/* Discards up to length octets, at most an ADU, that have already been + received, without waiting for more. errno is left unchanged. */ +static void discard_received(modbus_t *ctx, int length) +{ + const int saved_errno = errno; + uint8_t buf[MAX_MESSAGE_LENGTH]; + fd_set rset; + struct timeval tv; + int rc; + + while (length > 0) { + FD_ZERO(&rset); + FD_SET(ctx->s, &rset); + tv.tv_sec = 0; + tv.tv_usec = 0; + if (ctx->backend->select(ctx, &rset, &tv, length) <= 0) { + break; + } + rc = ctx->backend->recv(ctx, buf, length); + if (rc <= 0) { + break; + } + length -= rc; + } + errno = saved_errno; +} + /* Waits a response from a modbus server or a request from a modbus client. This function blocks if there is no replies (3 timeouts). @@ -370,6 +423,8 @@ int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type) struct timeval *p_tv; unsigned int length_to_read; int msg_length = 0; + /* TCP response read by its content: where its MBAP length ends it */ + int mbap_end = 0; _step_t step; #ifdef _WIN32 int wsa_err; @@ -506,15 +561,16 @@ int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type) if (length_to_read == 0) { switch (step) { case _STEP_FUNCTION: - if (msg_type == MSG_INDICATION && - ctx->backend->backend_type == _MODBUS_BACKEND_TYPE_TCP) { - /* Over TCP, a request ends where the length field of the MBAP - header (bytes 4 and 5) says, whatever its function code. It - counts the unit identifier and the function code, already - read. Reading exactly that much leaves the next request - intact in the socket, so it is neither lost nor parsed from - its middle. */ + if (ctx->backend->backend_type == _MODBUS_BACKEND_TYPE_TCP) { + /* Over TCP, a message ends where the length field of the MBAP + header (bytes 4 and 5) says. It counts the unit identifier + and the function code, already read. Reading exactly that + much leaves the next message intact in the socket, so it is + neither lost nor parsed from its middle. */ const int remaining = ((msg[4] << 8) + msg[5]) - 2; + const int valid_length = + remaining >= 0 && + msg_length + remaining <= (int) ctx->backend->max_adu_length; /* A protocol identifier other than 0 is not Modbus, or a header read where no request starts */ @@ -523,15 +579,25 @@ int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type) _error_print(ctx, "invalid protocol identifier"); return -1; } - if (remaining < 0 || - msg_length + remaining > (int) ctx->backend->max_adu_length) { - errno = EMBBADDATA; - _error_print(ctx, "invalid MBAP length"); - return -1; + if (msg_type == MSG_INDICATION || + !is_known_response(msg[ctx->backend->header_length])) { + if (!valid_length) { + errno = EMBBADDATA; + _error_print(ctx, "invalid MBAP length"); + return -1; + } + length_to_read = remaining; + step = _STEP_DATA; + break; + } + /* A response of known layout is read by its content, as some + gateways announce a longer MBAP length than they send, or + even a meaningless one. A valid MBAP length tells which + octets beyond it to discard once read; another is ignored, + as it always was. */ + if (valid_length) { + mbap_end = msg_length + remaining; } - length_to_read = remaining; - step = _STEP_DATA; - break; } /* Function code position */ length_to_read = compute_meta_length_after_function( @@ -570,6 +636,12 @@ int _modbus_receive_msg(modbus_t *ctx, uint8_t *msg, msg_type_t msg_type) if (ctx->debug) printf("\n"); + /* Octets a peer adds beyond its response, within its MBAP length, would + otherwise be read as the start of the next response */ + if (mbap_end > msg_length) { + discard_received(ctx, mbap_end - msg_length); + } + return ctx->backend->check_integrity(ctx, msg, msg_length); } diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index 8d62f8423..ff6c8ffd5 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -784,6 +784,29 @@ int main(int argc, char *argv[]) ASSERT_TRUE(rc == -1 && errno == EMBBADDATA, ""); free(tab_rp_registers_bad); + if (use_backend != RTU) { + /* A response whose MBAP length announces two octets more than it + holds is used, whether the server sends them or not, and they are + not read as the response to the next request. So is a response + with an MBAP length of 0, which is ignored. */ + const uint16_t wrong_mbap_address[] = {UT_REGISTERS_ADDRESS_TRAILING_OCTETS, + UT_REGISTERS_ADDRESS_OVERSTATED_LENGTH, + UT_REGISTERS_ADDRESS_INVALID_MBAP_LENGTH}; + const char *wrong_mbap_case[] = {"two octets too long, sent", + "two octets too long, not sent", + "0"}; + + for (i = 0; i < 3; i++) { + rc = modbus_read_registers(ctx, wrong_mbap_address[i], 1, tab_rp_registers); + printf("* MBAP length %s: ", wrong_mbap_case[i]); + ASSERT_TRUE(rc == 1 && tab_rp_registers[0] == 0, "FAILED (%d)\n", rc); + + rc = modbus_read_registers(ctx, UT_REGISTERS_ADDRESS, 1, tab_rp_registers); + printf(" next response: "); + ASSERT_TRUE(rc == 1, "FAILED (%d)\n", rc); + } + } + /** MANUAL EXCEPTION **/ printf("\nTEST MANUAL EXCEPTION:\n"); rc = modbus_read_registers( diff --git a/tests/unit-test-server.c b/tests/unit-test-server.c index b1e364837..73c75e74d 100644 --- a/tests/unit-test-server.c +++ b/tests/unit-test-server.c @@ -209,6 +209,36 @@ int main(int argc, char *argv[]) } } continue; + } else if ((address == UT_REGISTERS_ADDRESS_TRAILING_OCTETS || + address == UT_REGISTERS_ADDRESS_OVERSTATED_LENGTH || + address == UT_REGISTERS_ADDRESS_INVALID_MBAP_LENGTH) && + use_backend != RTU) { + /* Read of one register: the response needs an MBAP length of 5 + (unit identifier, function, byte count, one register). It + announces 7, and the 2 octets beyond are sent or not, or it + announces 0 */ + uint8_t rsp[] = "\x00\x00\x00\x00\x00\x07\xFF\x03\x02\x00\x00\xAA\xBB"; + int rsp_length = + (address == UT_REGISTERS_ADDRESS_TRAILING_OCTETS) ? 13 : 11; + int w_s = modbus_get_socket(ctx); + if (w_s == -1) { + fprintf(stderr, "Unable to get a valid socket in special test\n"); + continue; + } + + if (address == UT_REGISTERS_ADDRESS_INVALID_MBAP_LENGTH) { + rsp[5] = 0x00; + } + printf("Reply with a wrong MBAP length\n"); + /* Copy TID and unit identifier */ + rsp[0] = query[0]; + rsp[1] = query[1]; + rsp[6] = query[6]; + rc = send(w_s, (const char *) rsp, rsp_length, MSG_NOSIGNAL); + if (rc == -1) { + break; + } + continue; } } else if (function == MODBUS_FC_WRITE_SINGLE_COIL) { if (address == UT_BITS_ADDRESS_INVALID_REQUEST_LENGTH) { diff --git a/tests/unit-test.h.in b/tests/unit-test.h.in index 1e6aa8ffc..456e942c5 100644 --- a/tests/unit-test.h.in +++ b/tests/unit-test.h.in @@ -49,6 +49,13 @@ const uint16_t UT_REGISTERS_ADDRESS_INVALID_TID_OR_SLAVE = 0x171; const uint16_t UT_REGISTERS_ADDRESS_SLEEP_500_MS = 0x172; /* The server will wait for 5 ms before sending each byte */ const uint16_t UT_REGISTERS_ADDRESS_BYTE_SLEEP_5_MS = 0x173; +/* The MBAP length of the response of the server will announce two octets more + than its content requires, sent after it (TCP only) */ +const uint16_t UT_REGISTERS_ADDRESS_TRAILING_OCTETS = 0x174; +/* The same two octets announced, but not sent (TCP only) */ +const uint16_t UT_REGISTERS_ADDRESS_OVERSTATED_LENGTH = 0x175; +/* An MBAP length of 0 instead (TCP only) */ +const uint16_t UT_REGISTERS_ADDRESS_INVALID_MBAP_LENGTH = 0x176; /* If the following value is used, a bad response is sent. It's better to test with a lower value than From 21cecc7f65ea1b8c2be72249fee275faa71bbaf4 Mon Sep 17 00:00:00 2001 From: StalderT Date: Mon, 12 Oct 2026 11:53:00 +0200 Subject: [PATCH 3/3] Skip the late response to a TCP request that timed out When a response timed out over TCP, it could still arrive later, ahead of the response to the next request. The client took it for that response and failed on its transaction ID, and every following exchange read the response to the previous request: the connection stayed shifted until the application closed it, or flushed it at the right time. A faulty or compromised device can cause this at will by answering just after the response timeout. After a timeout, the first response whose transaction ID doesn't match the request is now read whole and skipped, and the client goes on waiting for the response it expects. A server answers the requests of a connection in order, so only that first response can be a late one, and only one response is skipped per timeout: a peer can't make the client wait longer than one more response timeout by sending responses with other transaction IDs. Connecting or closing forgets the timeout. modbus_receive_confirmation(), which doesn't know the request, is unchanged. The test client lets a request time out against the delayed reply of the server, then expects a valid response to its next request. --- src/modbus-private.h | 3 +++ src/modbus.c | 56 ++++++++++++++++++++++++++++++++++------ tests/unit-test-client.c | 15 +++++++++++ 3 files changed, 66 insertions(+), 8 deletions(-) diff --git a/src/modbus-private.h b/src/modbus-private.h index ea83187f7..294a9ba3a 100644 --- a/src/modbus-private.h +++ b/src/modbus-private.h @@ -104,6 +104,9 @@ struct _modbus { struct timeval response_timeout; struct timeval byte_timeout; struct timeval indication_timeout; + /* TCP only: a response timed out on this connection, so it can still + arrive, late, instead of the response to a later request */ + int late_response; const modbus_backend_t *backend; void *backend_data; }; diff --git a/src/modbus.c b/src/modbus.c index 33a1be990..ed35b9894 100644 --- a/src/modbus.c +++ b/src/modbus.c @@ -674,6 +674,41 @@ int modbus_receive_confirmation(modbus_t *ctx, uint8_t *rsp) return _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); } +/* Receives the response to req. Over TCP, the response to a request that timed + out can still arrive later, ahead of the response to req: it is then read + whole and skipped. A server answers the requests of a connection in order, so + only the first response after a timeout can be such a late one, and only one + response is skipped per timeout: a peer can't make the client wait longer than + one more response timeout by sending responses with other transaction IDs. */ +static int receive_confirmation(modbus_t *ctx, const uint8_t *req, uint8_t *rsp) +{ + int rc; + + for (;;) { + rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + if (ctx->backend->backend_type != _MODBUS_BACKEND_TYPE_TCP) { + return rc; + } + if (rc == -1) { + if (errno == ETIMEDOUT) { + ctx->late_response = TRUE; + } + return -1; + } + if (!ctx->late_response || (rsp[0] == req[0] && rsp[1] == req[1])) { + ctx->late_response = FALSE; + return rc; + } + + if (ctx->debug) { + fprintf(stderr, + "Skip a late response (transaction ID 0x%X)\n", + (rsp[0] << 8) + rsp[1]); + } + ctx->late_response = FALSE; + } +} + static int check_confirmation(modbus_t *ctx, uint8_t *req, uint8_t *rsp, int rsp_length) { int rc; @@ -1593,7 +1628,7 @@ static int read_io_status(modbus_t *ctx, int function, int addr, int nb, uint8_t unsigned int offset_end; unsigned int i; - rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + rc = receive_confirmation(ctx, req, rsp); if (rc == -1) return -1; @@ -1702,7 +1737,7 @@ static int read_registers(modbus_t *ctx, int function, int addr, int nb, uint16_ unsigned int offset; int i; - rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + rc = receive_confirmation(ctx, req, rsp); if (rc == -1) return -1; @@ -1793,7 +1828,7 @@ static int write_single(modbus_t *ctx, int function, int addr, const uint16_t va /* Used by write_bit and write_register */ uint8_t rsp[MAX_MESSAGE_LENGTH]; - rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + rc = receive_confirmation(ctx, req, rsp); if (rc == -1) return -1; @@ -1878,7 +1913,7 @@ int modbus_write_bits(modbus_t *ctx, int addr, int nb, const uint8_t *src) if (rc > 0) { uint8_t rsp[MAX_MESSAGE_LENGTH]; - rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + rc = receive_confirmation(ctx, req, rsp); if (rc == -1) return -1; @@ -1927,7 +1962,7 @@ int modbus_write_registers(modbus_t *ctx, int addr, int nb, const uint16_t *src) if (rc > 0) { uint8_t rsp[MAX_MESSAGE_LENGTH]; - rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + rc = receive_confirmation(ctx, req, rsp); if (rc == -1) return -1; @@ -1970,7 +2005,7 @@ int modbus_mask_write_register(modbus_t *ctx, /* Used by write_bit and write_register */ uint8_t rsp[MAX_MESSAGE_LENGTH]; - rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + rc = receive_confirmation(ctx, req, rsp); if (rc == -1) return -1; @@ -2043,7 +2078,7 @@ int modbus_write_and_read_registers(modbus_t *ctx, if (rc > 0) { unsigned int offset; - rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + rc = receive_confirmation(ctx, req, rsp); if (rc == -1) return -1; @@ -2086,7 +2121,7 @@ int modbus_report_slave_id(modbus_t *ctx, int max_dest, uint8_t *dest) unsigned int offset; uint8_t rsp[MAX_MESSAGE_LENGTH]; - rc = _modbus_receive_msg(ctx, rsp, MSG_CONFIRMATION); + rc = receive_confirmation(ctx, req, rsp); if (rc == -1) return -1; @@ -2124,6 +2159,8 @@ void _modbus_init_common(modbus_t *ctx) ctx->indication_timeout.tv_sec = 0; ctx->indication_timeout.tv_usec = 0; + + ctx->late_response = FALSE; } /* Define the slave number */ @@ -2300,6 +2337,8 @@ int modbus_connect(modbus_t *ctx) return -1; } + /* No response of a previous connection can arrive on a new one */ + ctx->late_response = FALSE; return ctx->backend->connect(ctx); } @@ -2308,6 +2347,7 @@ void modbus_close(modbus_t *ctx) if (ctx == NULL) return; + ctx->late_response = FALSE; ctx->backend->close(ctx); } diff --git a/tests/unit-test-client.c b/tests/unit-test-client.c index ff6c8ffd5..ea018af98 100644 --- a/tests/unit-test-client.c +++ b/tests/unit-test-client.c @@ -734,6 +734,21 @@ int main(int argc, char *argv[]) printf("7/8 Disable byte timeout: "); ASSERT_TRUE(rc == 1, ""); + if (use_backend != RTU) { + /* The response to a request that timed out arrives while the client + waits for the response to its next request: it must be skipped */ + modbus_set_response_timeout(ctx, 0, 200000); + rc = modbus_read_registers( + ctx, UT_REGISTERS_ADDRESS_SLEEP_500_MS, 1, tab_rp_registers); + printf("* request timed out: "); + ASSERT_TRUE(rc == -1 && errno == ETIMEDOUT, ""); + + modbus_set_response_timeout(ctx, 1, 0); + rc = modbus_read_registers(ctx, UT_REGISTERS_ADDRESS, 1, tab_rp_registers); + printf("* its late response skipped: "); + ASSERT_TRUE(rc == 1, "FAILED (%d)\n", rc); + } + // Invalid in TCP or RTU mode... modbus_t *invalid_ctx = modbus_new_tcp("1.2.3.4", 1502); modbus_set_response_timeout(invalid_ctx, 0, 1);