From fca45b6c8629ba5a67a72169b732dac85e7f17c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alexandru=20S=C4=83vulescu?= <alexandru.savulescu@cern.ch>
Date: Thu, 14 Nov 2024 13:36:28 +0100
Subject: [PATCH] RAMS7200 memory leaks on Tx

---
 CMakeLists.txt        |  2 +-
 Common/S7Utils.hxx    |  9 +++++++--
 RAMS7200LibFacade.cxx | 11 +++++++----
 RAMS7200MS.cxx        | 23 +++++++++++++----------
 test.cpp              |  4 +---
 5 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 617f82d..9c79392 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -18,7 +18,7 @@ project(
     LANGUAGES CXX
 )
 
-set(PROJECT_VERSION 1.0.2)
+set(PROJECT_VERSION 1.0.3)
 
 configure_file(config.h.in configured/config.h)
 include_directories(${CMAKE_CURRENT_BINARY_DIR}/configured)
diff --git a/Common/S7Utils.hxx b/Common/S7Utils.hxx
index 256d232..4a7844d 100644
--- a/Common/S7Utils.hxx
+++ b/Common/S7Utils.hxx
@@ -230,11 +230,16 @@ namespace Common{
             }
 
             static void TS7AllocateDataItemForAddress(TS7DataItem& item){
+                Common::S7Utils::TS7DeallocateDataItem(item);
+                item.pdata    =  new char[DataSizeByte(item.WordLen )*item.Amount];
+                std::memset(item.pdata, 0, DataSizeByte(item.WordLen )*item.Amount);
+            }
+
+            static void TS7DeallocateDataItem(TS7DataItem& item){
                 if(item.pdata != nullptr){
                     delete[] static_cast<char*>(item.pdata);
+                    item.pdata = nullptr;
                 }
-                item.pdata    =  new char[DataSizeByte(item.WordLen )*item.Amount];
-                std::memset(item.pdata, 0, DataSizeByte(item.WordLen )*item.Amount);
             }
 
             static int GetByteSizeFromAddress(const std::string& Address)
diff --git a/RAMS7200LibFacade.cxx b/RAMS7200LibFacade.cxx
index 69bd05a..94c7b0c 100644
--- a/RAMS7200LibFacade.cxx
+++ b/RAMS7200LibFacade.cxx
@@ -230,6 +230,10 @@ void RAMS7200LibFacade::RAMS7200ReadWriteMaxN(std::vector<DPInfo> dpItems, std::
             } else {
                 queueAll(std::move(dpItems), std::move(items));
             }
+        } else {
+            std::for_each(items.begin(), items.end(), [](TS7DataItem& item){
+               Common::S7Utils::TS7DeallocateDataItem(item);
+            });
         }
 
     }
@@ -251,8 +255,7 @@ void RAMS7200LibFacade::queueAll(std::vector<DPInfo>&& dpItems, std::vector<TS7D
             toDPItems.emplace_back(dpItems[i].dpAddress.c_str(), dpItems[i].dpSize, static_cast<char*>(s7items[i].pdata));
         } else {
             failed <<  dpItems[i].dpAddress.c_str() << " ";
-            delete[] static_cast<char*>(s7items[i].pdata);
-            s7items[i].pdata = nullptr;
+            Common::S7Utils::TS7DeallocateDataItem(s7items[i]);
         }
     }
 
@@ -292,12 +295,12 @@ void RAMS7200LibFacade::doSmoothing(std::vector<DPInfo>&& dpItems, std::vector<T
                         toDPItems.emplace_back(DPInfo.dpAddress.c_str(), dataSize, static_cast<char*>(item.pdata));
                         Common::Logger::globalInfo(Common::Logger::L4, DPInfo.dpAddress.c_str(), "--> Smoothing updated");
                     } else {
-                        delete[] static_cast<char*>(item.pdata);
+                        Common::S7Utils::TS7DeallocateDataItem(item);
                     }
                 }
             } else {
                 failed << dpItems[i].dpAddress.c_str() << " ";
-                delete[] static_cast<char*>(item.pdata);
+                Common::S7Utils::TS7DeallocateDataItem(item);
             }
             item.pdata = nullptr;
         }
diff --git a/RAMS7200MS.cxx b/RAMS7200MS.cxx
index 8ab1f38..f59533e 100644
--- a/RAMS7200MS.cxx
+++ b/RAMS7200MS.cxx
@@ -18,7 +18,10 @@
 #include <algorithm>
 
 
-RAMS7200MSVar::RAMS7200MSVar(std::string varName, int pollTime, TS7DataItem type) : varName(varName), pollTime(pollTime), _toPlc(type), _toDP(type){}
+RAMS7200MSVar::RAMS7200MSVar(std::string varName, int pollTime, TS7DataItem type) : varName(varName), pollTime(pollTime), _toPlc(type), _toDP(type){
+    _toPlc.pdata = nullptr;
+    _toDP.pdata = nullptr;
+}
 
 void RAMS7200MS::addVar(std::string varName, int pollTime)
 {
@@ -32,13 +35,9 @@ void RAMS7200MS::removeVar(std::string varName)
     std::lock_guard lock{_rwmutex};
     auto it = vars.find(varName);
     if(it != vars.end()) {
-        if (it->second._toDP.pdata != nullptr) {
-            delete[] static_cast<char*>(it->second._toDP.pdata);
-        }
-        if (it->second._toPlc.pdata != nullptr) {
-            delete[] static_cast<char*>(it->second._toPlc.pdata);
-        }
-        vars.erase(it);
+      Common::S7Utils::TS7DeallocateDataItem(it->second._toDP);
+      Common::S7Utils::TS7DeallocateDataItem(it->second._toPlc);
+      vars.erase(it);
     }
 }
 
@@ -48,8 +47,12 @@ void RAMS7200MS::queuePLCItem(const std::string& varName, void* item)
 
   if (vars.count(varName) == 0) {
     Common::Logger::globalWarning(__PRETTY_FUNCTION__, "Undefined variable:", varName.c_str());
+    delete[] static_cast<char*>(item);
     return;
   }
-
-  vars.at(varName)._toPlc.pdata = item;
+  auto old_data = std::exchange(vars.at(varName)._toPlc.pdata, item);
+  if (old_data != nullptr) {
+    delete[] static_cast<char*>(old_data);
+    Common::Logger::globalInfo(Common::Logger::L1, "Overwriting old data for:", CharString(_ip.c_str()) + varName.c_str());
+  }
 }
diff --git a/test.cpp b/test.cpp
index d319c7b..868627d 100644
--- a/test.cpp
+++ b/test.cpp
@@ -306,9 +306,7 @@ void PerformTests()
             default:
                 break;
         }
-        if(item.pdata != nullptr){
-            delete[] item.pdata;
-        }
+        Common::S7Utils::TS7DeallocateDataItem(item);
     }
 }