Handle failed slave selection and stale client buffers safely

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);

tests/host/Arduino.h

@@ -0,0 +1,4 @@
+#pragma once
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>

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.

tests/host/invalid_slave.cpp

@@ -0,0 +1,116 @@
+// Host regression test: compile the real ModbusClient against a recording backend.
+#include "ModbusClient.h"
+#include <assert.h>
+#include <errno.h>
+#include <stdio.h>
+#include <initializer_list>
+
+
+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");
+}