fixed possible memory corruption in write only mode

BlackZork · May 13, 2024 · 80a61c9 · 80a61c9
1 parent fae936b
commit 80a61c9
Show file tree

Hide file tree

Showing 3 changed files with 90 additions and 33 deletions.
diff --git a/libmodmqttsrv/modbus_executor.cpp b/libmodmqttsrv/modbus_executor.cpp
@@ -23,7 +23,6 @@ ModbusExecutor::ModbusExecutor(
 {
     //some random past value, not using steady_clock:min() due to overflow
     mLastCommandTime = std::chrono::steady_clock::now() - std::chrono::hours(100000);
-    mLastQueue = mSlaveQueues.end();
     mCurrentSlaveQueue = mSlaveQueues.end();
     mInitialPoll = false;
     mReadRetryCount = mMaxReadRetryCount;
@@ -119,13 +118,13 @@ void
 ModbusExecutor::addWriteCommand(const std::shared_ptr<RegisterWrite>& pCommand) {
     if (mWriteCommandsQueued == 0) {
         // skip queuing for if there is no queued write commands.
-        // This improves write latency in use case, where there is a lot of polling 
-        // and sporadic write. I belive this is main use case for this gateway. 
+        // This improves write latency in use case, where there is a lot of polling
+        // and sporadic write. I belive this is main use case for this gateway.
 
-        // TODO this could leat to poll queue starvation when write commands
-        // arive in sync with slave execution time. Maybe there should be a 
-        // configuration switch to turn it off? 
-        if (mWaitingCommand != nullptr) 
+        // TODO this could lead to poll queue starvation when write commands
+        // arive in sync with slave execution time. Maybe there should be a
+        // configuration switch to turn it off?
+        if (mWaitingCommand != nullptr)
             mSlaveQueues[pCommand->mSlaveId].readdCommand(mWaitingCommand);
 
         mWaitingCommand = pCommand;
@@ -235,8 +234,8 @@ ModbusExecutor::executeNext() {
             ||
             (
                 mWaitingCommand->getDelay().delay_type == ModbusCommandDelay::DelayType::ON_SLAVE_CHANGE
-                && mLastQueue != mSlaveQueues.end()
-                && mCurrentSlaveQueue->first != mLastQueue->first
+                && mLastCommand != nullptr
+                && mWaitingCommand->mSlaveId != mLastCommand->mSlaveId
             )
         ) {
             auto delay_passed = std::chrono::steady_clock::now() - mLastCommandTime;
@@ -346,14 +345,14 @@ ModbusExecutor::sendCommand() {
             assert(mWriteCommandsQueued >= 0);
         }
     }
-    mLastQueue = mCurrentSlaveQueue;
     mLastCommand = mWaitingCommand;
 
     // to retry just leave mCurrentCommand
     // for next executeNext() call
     if (!retry) {
         mWaitingCommand.reset();
-        mCommandsLeft--;
+        if (mCommandsLeft > 0)
+            mCommandsLeft--;
     }
 }
 
@@ -386,7 +385,7 @@ void
 ModbusExecutor::resetCommandsCounter() {
     if (mCurrentSlaveQueue->second.mPollQueue.empty())
         mCommandsLeft = WRITE_BATCH_SIZE;
-    else
+    else if (mCurrentSlaveQueue != mSlaveQueues.end())
         mCommandsLeft = mCurrentSlaveQueue->second.mPollQueue.size() * 2;
 }
 

diff --git a/libmodmqttsrv/modbus_executor.hpp b/libmodmqttsrv/modbus_executor.hpp
@@ -74,7 +74,6 @@ class ModbusExecutor {
         std::chrono::steady_clock::time_point mLastCommandTime;
 
         //used to determine if we have to respect delay of RegisterPoll::ReadDelayType::ON_SLAVE_CHANGE
-        std::map<int, ModbusRequestsQueues>::iterator mLastQueue;
         std::shared_ptr<RegisterCommand> mWaitingCommand;
         std::shared_ptr<RegisterCommand> mLastCommand;
 

diff --git a/unittests/mqtt_command_only_tests.cpp b/unittests/mqtt_command_only_tests.cpp
@@ -3,6 +3,8 @@
 #include "defaults.hpp"
 #include "yaml_utils.hpp"
 
+TEST_CASE ("Write value in write-only configuration should succeed") {
+
 TestConfig config(R"(
 modmqttd:
 modbus:
@@ -22,35 +24,66 @@ TestConfig config(R"(
   objects:
     - topic: test_switch
       commands:
-        - name: set
+        - name: set1
           register: tcptest.1.2
           register_type: holding
 )");
-TEST_CASE ("Write value in write-only configuration should succeed") {
+
     MockedModMqttServerThread server(config.toString());
     server.setModbusRegisterValue("tcptest", 1, 2, modmqttd::RegisterType::HOLDING, 0);
     server.start();
 
-    server.waitForSubscription("test_switch/set");
+    server.waitForSubscription("test_switch/set1");
 
-    server.publish("test_switch/set", "7");
+    server.publish("test_switch/set1", "7");
     server.waitForModbusValue("tcptest",1,2, modmqttd::RegisterType::HOLDING, 0x7);
 
     server.stop();
 }
 
 
 TEST_CASE ("Write should respect slave delay_before_command") {
+
+TestConfig config(R"(
+modmqttd:
+modbus:
+  networks:
+    - name: tcptest
+      address: localhost
+      port: 501
+      delay_before_first_command: 5ms
+      slaves:
+        - address: 2
+          delay_before_first_command: 100ms
+
+mqtt:
+  client_id: mqtt_test
+  broker:
+    host: localhost
+  objects:
+    - topic: test_switch
+      commands:
+        - name: set1
+          register: tcptest.1.1
+          register_type: holding
+        - name: set2
+          register: tcptest.2.2
+          register_type: holding
+)");
+
     MockedModMqttServerThread server(config.toString());
-    server.setModbusRegisterValue("tcptest", 1, 2, modmqttd::RegisterType::HOLDING, 0);
+    server.setModbusRegisterValue("tcptest", 1, 1, modmqttd::RegisterType::HOLDING, 0);
+    server.setModbusRegisterValue("tcptest", 2, 2, modmqttd::RegisterType::HOLDING, 0);
     server.start();
 
-    auto now = std::chrono::steady_clock::now();
-    server.waitForSubscription("test_switch/set");
+    server.waitForSubscription("test_switch/set1");
+    server.waitForSubscription("test_switch/set2");
 
-    server.publish("test_switch/set", "7");
-    server.publish("test_switch/set", "8");
-    server.waitForModbusValue("tcptest",1,2, modmqttd::RegisterType::HOLDING, 0x8, std::chrono::milliseconds(700));
+    auto now = std::chrono::steady_clock::now();
+    server.publish("test_switch/set1", "7");
+    std::this_thread::sleep_for(std::chrono::milliseconds(50));
+    server.publish("test_switch/set2", "8");
+    server.waitForModbusValue("tcptest",2,2, modmqttd::RegisterType::HOLDING, 0x8, std::chrono::milliseconds(70000));
     auto after = std::chrono::steady_clock::now();
     //global is ignored
     REQUIRE(after - now >= std::chrono::milliseconds(100));
@@ -60,6 +93,7 @@ TEST_CASE ("Write should respect slave delay_before_command") {
 
 TEST_CASE ("When network delay") {
 
+SECTION("is set to delay_before_command write should wait") {
 
 TestConfig global_delay_config(R"(
 modmqttd:
@@ -81,7 +115,7 @@ TestConfig global_delay_config(R"(
           register_type: holding
 )");
 
-SECTION("is set to delay_before_command write should wait") {
+
     MockedModMqttServerThread server(global_delay_config.toString());
     server.setModbusRegisterValue("tcptest", 1, 2, modmqttd::RegisterType::HOLDING, 0);
     server.start();
@@ -99,20 +133,45 @@ SECTION("is set to delay_before_command write should wait") {
     server.stop();
 }
 
+
 SECTION("is set to delay_before_first_command write should wait") {
-    global_delay_config.mYAML["modbus"]["networks"][0].remove("delay_before_command");
-    global_delay_config.mYAML["modbus"]["networks"][0]["delay_before_first_command"] = "500ms";
 
-    MockedModMqttServerThread server(global_delay_config.toString());
-    server.setModbusRegisterValue("tcptest", 1, 2, modmqttd::RegisterType::HOLDING, 0);
+TestConfig global_first_delay_config(R"(
+modmqttd:
+modbus:
+    networks:
+    - name: tcptest
+      delay_before_first_command: 500ms
+      address: localhost
+      port: 501
+mqtt:
+  client_id: mqtt_test
+  broker:
+    host: localhost
+  objects:
+    - topic: test_switch
+      commands:
+        - name: set1
+          register: tcptest.1.1
+          register_type: holding
+        - name: set2
+          register: tcptest.2.2
+          register_type: holding
+)");
+
+
+    MockedModMqttServerThread server(global_first_delay_config.toString());
+    server.setModbusRegisterValue("tcptest", 1, 1, modmqttd::RegisterType::HOLDING, 0);
+    server.setModbusRegisterValue("tcptest", 2, 2, modmqttd::RegisterType::HOLDING, 0);
     server.start();
 
-    auto now = std::chrono::steady_clock::now();
-    server.waitForSubscription("test_switch/set");
+    server.waitForSubscription("test_switch/set1");
+    server.waitForSubscription("test_switch/set2");
 
-    server.publish("test_switch/set", "7");
-    server.publish("test_switch/set", "8");
-    server.waitForModbusValue("tcptest",1,2, modmqttd::RegisterType::HOLDING, 0x8, std::chrono::milliseconds(700));
+    auto now = std::chrono::steady_clock::now();
+    server.publish("test_switch/set1", "7");
+    server.publish("test_switch/set2", "8");
+    server.waitForModbusValue("tcptest",2,2, modmqttd::RegisterType::HOLDING, 0x8, std::chrono::milliseconds(700));
     auto after = std::chrono::steady_clock::now();
 
     REQUIRE(after - now >= std::chrono::milliseconds(500));