diff --git a/src/ModbusClient.cpp b/src/ModbusClient.cpp index bf4b329..87afe22 100644 --- a/src/ModbusClient.cpp +++ b/src/ModbusClient.cpp @@ -77,6 +77,12 @@ int ModbusClient::begin(modbus_t* mb, int defaultId) void ModbusClient::end() { + _transmissionBegun = false; + _available = 0; + _read = 0; + _availableForWrite = 0; + _written = 0; + if (_values != NULL) { free(_values); @@ -100,7 +106,9 @@ int ModbusClient::coilRead(int id, int address) { uint8_t value; - modbus_set_slave(_mb, id); + if (modbus_set_slave(_mb, id) < 0) { + return -1; + } if (modbus_read_bits(_mb, address, 1, &value) < 0) { return -1; @@ -118,7 +126,9 @@ int ModbusClient::discreteInputRead(int id, int address) { uint8_t value; - modbus_set_slave(_mb, id); + if (modbus_set_slave(_mb, id) < 0) { + return -1; + } if (modbus_read_input_bits(_mb, address, 1, &value) < 0) { return -1; @@ -136,7 +146,9 @@ long ModbusClient::holdingRegisterRead(int id, int address) { uint16_t value; - modbus_set_slave(_mb, id); + if (modbus_set_slave(_mb, id) < 0) { + return -1; + } if (modbus_read_registers(_mb, address, 1, &value) < 0) { return -1; @@ -154,7 +166,9 @@ long ModbusClient::inputRegisterRead(int id, int address) { uint16_t value; - modbus_set_slave(_mb, id); + if (modbus_set_slave(_mb, id) < 0) { + return -1; + } if (modbus_read_input_registers(_mb, address, 1, &value) < 0) { return -1; @@ -170,7 +184,9 @@ int ModbusClient::coilWrite(int address, uint8_t value) int ModbusClient::coilWrite(int id, int address, uint8_t value) { - modbus_set_slave(_mb, id); + if (modbus_set_slave(_mb, id) < 0) { + return 0; + } if (modbus_write_bit(_mb, address, value) < 0) { return 0; @@ -186,7 +202,9 @@ int ModbusClient::holdingRegisterWrite(int address, uint16_t value) int ModbusClient::holdingRegisterWrite(int id, int address, uint16_t value) { - modbus_set_slave(_mb, id); + if (modbus_set_slave(_mb, id) < 0) { + return 0; + } if (modbus_write_register(_mb, address, value) < 0) { return 0; @@ -202,7 +220,9 @@ int ModbusClient::registerMaskWrite(int address, uint16_t andMask, uint16_t orMa int ModbusClient::registerMaskWrite(int id, int address, uint16_t andMask, uint16_t orMask) { - modbus_set_slave(_mb, id); + if (modbus_set_slave(_mb, id) < 0) { + return 0; + } if (modbus_mask_write_register(_mb, address, andMask, orMask) < 0) { return 0; @@ -224,15 +244,27 @@ int ModbusClient::beginTransmission(int id, int type, int address, int nb) return 0; } + if (modbus_set_slave(_mb, id) < 0) { + return 0; + } + int valueSize = (type == COILS) ? sizeof(uint8_t) : sizeof(uint16_t); - _values = realloc(_values, nb * valueSize); + // A new transfer invalidates the old read/write view before resizing it. + _transmissionBegun = false; + _available = 0; + _read = 0; + _availableForWrite = 0; + _written = 0; + + void* values = realloc(_values, nb * valueSize); - if (_values == NULL) { + if (values == NULL) { errno = ENOMEM; return 0; } + _values = values; memset(_values, 0x00, nb * valueSize); @@ -282,19 +314,19 @@ int ModbusClient::endTransmission() int result = -1; - modbus_set_slave(_mb, _id); - - switch (_type) { - case COILS: - result = modbus_write_bits(_mb, _address, _nb, (const uint8_t*)_values); - break; + if (modbus_set_slave(_mb, _id) >= 0) { + switch (_type) { + case COILS: + result = modbus_write_bits(_mb, _address, _nb, (const uint8_t*)_values); + break; - case HOLDING_REGISTERS: - result = modbus_write_registers(_mb, _address, _nb, (const uint16_t*)_values); - break; + case HOLDING_REGISTERS: + result = modbus_write_registers(_mb, _address, _nb, (const uint16_t*)_values); + break; - default: - return 0; + default: + break; + } } _transmissionBegun = false; @@ -320,20 +352,30 @@ int ModbusClient::requestFrom(int id, int type, int address, int nb) return 0; } + if (modbus_set_slave(_mb, id) < 0) { + return 0; + } + int valueSize = (type == COILS || type == DISCRETE_INPUTS) ? sizeof(uint8_t) : sizeof(uint16_t); - _values = realloc(_values, nb * valueSize); + // A new transfer invalidates the old read/write view before resizing it. + _transmissionBegun = false; + _available = 0; + _read = 0; + _availableForWrite = 0; + _written = 0; - if (_values == NULL) { + void* values = realloc(_values, nb * valueSize); + + if (values == NULL) { errno = ENOMEM; return 0; } + _values = values; int result = -1; - modbus_set_slave(_mb, id); - switch (type) { case COILS: result = modbus_read_bits(_mb, address, nb, (uint8_t*)_values); diff --git a/tests/host/Arduino.h b/tests/host/Arduino.h new file mode 100644 index 0000000..87cc4a0 --- /dev/null +++ b/tests/host/Arduino.h @@ -0,0 +1,4 @@ +#pragma once +#include +#include +#include diff --git a/tests/host/README.md b/tests/host/README.md new file mode 100644 index 0000000..d95c983 --- /dev/null +++ b/tests/host/README.md @@ -0,0 +1,22 @@ +# Host regression checks + +From the repository root, using Clang or GCC: + +```sh +c++ -std=c++11 -Wall -Wextra -Werror -fsanitize=address,undefined \ + -Drealloc=controlled_realloc -Dfree=controlled_free \ + -Itests/host -Isrc -c src/ModbusClient.cpp -o /tmp/modbus-client.o +c++ -std=c++11 -Wall -Wextra -Werror -fsanitize=address,undefined \ + -Itests/host -Isrc /tmp/modbus-client.o tests/host/invalid_slave.cpp \ + -o /tmp/arduino-modbus-invalid-slave +/tmp/arduino-modbus-invalid-slave +``` + +This compiles the production client against a recording libmodbus backend and +minimal Arduino header. It verifies propagation of failed slave selection, +absence of subsequent reads/writes, valid requests, and buffered destination +reselection. It does not exercise serial hardware or replace board compilation +and device testing. + +Allocator interception also checks failed buffer resizes and final buffer release. +Failed reads and end() must invalidate buffered read/write state. diff --git a/tests/host/invalid_slave.cpp b/tests/host/invalid_slave.cpp new file mode 100644 index 0000000..8afcdde --- /dev/null +++ b/tests/host/invalid_slave.cpp @@ -0,0 +1,116 @@ +// Host regression test: compile the real ModbusClient against a recording backend. +#include "ModbusClient.h" +#include +#include +#include +#include + + +struct _modbus { int slave; }; +static int requests = 0; +static int selected = 0; +static bool failSelection = false; +static bool failRead = false; +static bool failAllocation = false; +static void* buffer = NULL; +extern "C" void* controlled_realloc(void* old, size_t size) { + if (failAllocation) return NULL; + void* result = realloc(old, size); + if (result) buffer = result; + return result; +} +extern "C" void controlled_free(void* ptr) { + assert(ptr == buffer); + free(ptr); + buffer = NULL; +} +extern "C" { +int modbus_connect(modbus_t*) { return 0; } +void modbus_close(modbus_t*) {} +void modbus_free(modbus_t* ctx) { delete ctx; } +int modbus_set_error_recovery(modbus_t*, modbus_error_recovery_mode) { return 0; } +int modbus_set_response_timeout(modbus_t*, uint32_t, uint32_t) { return 0; } +const char* modbus_strerror(int) { return "test error"; } +int modbus_set_slave(modbus_t* ctx, int id) { + if (failSelection || id < 0 || id > 247) { errno = EINVAL; return -1; } + ctx->slave = id; + return 0; +} +#define READ_FN(name, type) \ +int name(modbus_t* ctx, int, int nb, type* dest) { \ + ++requests; selected = ctx->slave; \ + if (failRead) { errno = EIO; return -1; } \ + for (int i = 0; i < nb; ++i) dest[i] = 1; \ + return nb; \ +} +READ_FN(modbus_read_bits, uint8_t) +READ_FN(modbus_read_input_bits, uint8_t) +READ_FN(modbus_read_registers, uint16_t) +READ_FN(modbus_read_input_registers, uint16_t) +int modbus_write_bit(modbus_t* ctx, int, int) { ++requests; selected = ctx->slave; return 1; } +int modbus_write_register(modbus_t* ctx, int, int) { ++requests; selected = ctx->slave; return 1; } +int modbus_mask_write_register(modbus_t* ctx, int, uint16_t, uint16_t) { ++requests; selected = ctx->slave; return 1; } +int modbus_write_bits(modbus_t* ctx, int, int nb, const uint8_t*) { ++requests; selected = ctx->slave; return nb; } +int modbus_write_registers(modbus_t* ctx, int, int nb, const uint16_t*) { ++requests; selected = ctx->slave; return nb; } +} +class TestClient : public ModbusClient { +public: + TestClient() : ModbusClient(1000) { assert(begin(new modbus_t{1}, 1)); } +}; +int main() { + TestClient client; + for (int id : {-1, 248, 256}) { + requests = 0; + assert(client.coilRead(id, 0) == -1); + assert(client.discreteInputRead(id, 0) == -1); + assert(client.holdingRegisterRead(id, 0) == -1); + assert(client.inputRegisterRead(id, 0) == -1); + assert(client.coilWrite(id, 0, 1) == 0); + assert(client.holdingRegisterWrite(id, 0, 1) == 0); + assert(client.registerMaskWrite(id, 0, 0, 1) == 0); + for (int type = COILS; type <= INPUT_REGISTERS; ++type) { + assert(client.requestFrom(id, type, 0, 2) == 0); + } + assert(client.beginTransmission(id, COILS, 0, 2) == 0); + assert(client.beginTransmission(id, HOLDING_REGISTERS, 0, 2) == 0); + assert(errno == EINVAL); + assert(requests == 0); + } + assert(client.coilRead(247, 0) == 1); + assert(selected == 247); + assert(client.requestFrom(2, HOLDING_REGISTERS, 0, 2) == 2); + assert(client.read() == 1 && client.read() == 1 && client.read() == -1); + assert(client.beginTransmission(3, COILS, 0, 2) == 1); + assert(client.write(1) && client.write(0)); + assert(client.coilRead(4, 0) == 1); + assert(client.endTransmission() == 1); + assert(selected == 3); // reselect the buffered transmission's destination + assert(client.endTransmission() == 0); + assert(client.beginTransmission(3, HOLDING_REGISTERS, 0, 1) == 1); + assert(client.write(42) == 1); + failSelection = true; + requests = 0; + assert(client.endTransmission() == 0); + assert(requests == 0 && errno == EINVAL); + failSelection = false; + assert(client.endTransmission() == 0); + assert(client.requestFrom(1, HOLDING_REGISTERS, 0, 8) == 8); + failRead = true; + assert(client.requestFrom(1, COILS, 0, 1) == 0); + assert(client.available() == 0); + assert(client.read() == -1); + failRead = false; + assert(client.requestFrom(1, COILS, 0, 2) == 2); + failAllocation = true; + assert(client.requestFrom(1, COILS, 0, 16) == 0); + assert(errno == ENOMEM && client.available() == 0); + assert(client.beginTransmission(1, COILS, 0, 16) == 0); + assert(errno == ENOMEM && client.write(1) == 0); + failAllocation = false; + assert(client.requestFrom(1, COILS, 0, 2) == 2); + client.end(); + assert(buffer == NULL); + assert(client.available() == 0); + assert(client.read() == -1); + puts("invalid address propagation and valid request controls passed"); +}