🐛 avoid potential invalid memory access via lib60870 callback parameter

Author: m-unkel

.gitignore

@@ -7,6 +7,7 @@ dist
 *.so.*
 ~*
 callgrind.*
+asan.log
 valgrind.log
 *.prof
 bin/upload*

bin/profiler.sh

@@ -2,7 +2,7 @@
 
 echo ""
 echo "+----------------------------------------------------------+"
-echo "| PROFILER                                                 |"
+echo "| PREPARE: VALGRIND                                        |"
 echo "+----------------------------------------------------------+"
 
 # change working directory to repository directory
@@ -14,17 +14,22 @@ if [ $# -eq 1 ] ; then
     ARGS=$1
 fi
 
+if [ -d cmake-build-valgrind ]; then
+    rm -rf cmake-build-valgrind
+fi
+
 # generate make scripts, build application and install to bin folder
-cmake . -Bcmake-build-debug -DCMAKE_BUILD_TYPE=Debug "$ARGS"
-cmake . -Bcmake-build-debug -DCMAKE_BUILD_TYPE=Debug -DC104_VERSION_INFO=2.2.2 "$ARGS"
-cd cmake-build-debug || exit 1
+export CFLAGS="-O0"
+export CXXFLAGS="-O0"
+cmake . -Bcmake-build-valgrind -DCMAKE_BUILD_TYPE=Debug -DC104_VERSION_INFO=2.2.2 "$ARGS"
+cd cmake-build-valgrind || exit 1
 make -j"$(nproc+1)" _c104
 cd .. || exit 1
 
 rm ./c104/_c104*.so
 
 # copy libraries for python package
-cp ./cmake-build-debug/_c104*.so ./c104/
+cp ./cmake-build-valgrind/_c104*.so ./c104/
 
 echo ""
 echo "BUILD DONE!"
@@ -40,17 +45,43 @@ echo "+----------------------------------------------------------+"
 #valgrind --leak-check=full --leak-resolution=med --track-origins=yes --read-inline-info=yes --suppressions=valgrind-python.supp python3 -E -tt ./test.py
 valgrind --tool=memcheck --dsymutil=yes --leak-check=full --track-origins=yes --show-leak-kinds=all --leak-resolution=high --suppressions=./tests/valgrind-python.supp python3 -X showrefcount ./tests/test.py > valgrind.log 2>&1
 
+# clean-up
+rm ./c104/_c104*.so
+
 echo ""
 echo "+----------------------------------------------------------+"
-echo "| PROFILER: GOOGLE-PERFTOOLS                               |"
+echo "| PREPARE: GCC AddressSanitizer (ASan)                     |"
 echo "+----------------------------------------------------------+"
-python3 -m yep -- test.py
-pprof --callgrind c104.so test.py.prof > callgrind.c104
 
-cd ..
+if [ -d cmake-build-asan ]; then
+    rm -rf cmake-build-asan
+fi
+
+# generate make scripts, build application and install to bin folder
+export CFLAGS="-fsanitize=address,leak -O0"
+export CXXFLAGS="-fsanitize=address,leak -O0"
+cmake . -Bcmake-build-asan -DCMAKE_BUILD_TYPE=Debug -DC104_VERSION_INFO=2.2.2 "$ARGS"
+cd cmake-build-asan || exit 1
+make -j"$(nproc+1)" _c104
+cd .. || exit 1
+
+# copy libraries for python package
+cp ./cmake-build-asan/_c104*.so ./c104/
+
+echo ""
+echo "BUILD DONE!"
+echo ""
 
 echo ""
-echo "Open file tests/callgrind.c104 in KCachegrind or QCachegrind to visualize metrics."
+echo "+----------------------------------------------------------+"
+echo "| PROFILER: GCC AddressSanitizer (ASan)                    |"
+echo "+----------------------------------------------------------+"
+
+export ASAN_OPTIONS=verify_asan_link_order=0
+LD_PRELOAD=$(gcc -print-file-name=libasan.so) python3 tests/test.py > asan.log 2>&1
+
+# clean-up
+rm ./c104/_c104*.so
 
 echo ""
 echo "+----------------------------------------------------------+"

c104/__init__.pyi

@@ -1345,6 +1345,11 @@ class Information:
     This class represents all specialized kind of information a specific point may have
     """
     @property
+    def is_readonly(self) -> bool:
+        """
+        test if the information is read-only
+        """
+    @property
     def processed_at(self) -> datetime.datetime:
         """
         timestamp with milliseconds of last local information processing

docs/source/changelog.rst

@@ -4,10 +4,19 @@ Change log
 v2.2.1 (draft)
 -------
 
+Features
+^^^^^^^^
+
+- Add **Information.is_readonly**- property to test, if a new value can be assigned to this information.
+
 Fixes
 ^^^^^^
 
 - fix docstring for c104.ProtocolParameters timings: unit is seconds not milliseconds
+- support try-except blocks in callbacks instead of catching all errors and removing the callback
+- prevent invalid pointer access from lib60870 callbacks
+- setting **Point.value** will raise ValueError if info is readonly
+- setting **Point.quality** will raise ValueError if info is readonly
 
 v2.2.0
 -------

src/Server.cpp

@@ -63,6 +63,10 @@ bool areKeysSequential(const std::map<uint_fast16_t, T> &inputMap) {
   return true; // All keys are sequential
 }
 
+// Define static members
+std::unordered_map<void *, std::weak_ptr<Server>> Server::instanceMap;
+std::mutex Server::instanceMapMutex;
+
 Server::Server(const std::string &bind_ip, const std::uint_fast16_t tcp_port,
                const std::uint_fast16_t tick_rate_ms,
                const std::uint_fast16_t select_timeout_ms,
@@ -103,35 +107,51 @@ Server::Server(const std::string &bind_ip, const std::uint_fast16_t tcp_port,
   param->t2 = 1; // acknowledgement interval
 
   // Function pointers for custom handler functions
+  void *key = static_cast<void *>(this);
   CS104_Slave_setConnectionRequestHandler(
-      slave, &Server::connectionRequestHandler, this);
+      slave, &Server::connectionRequestHandler, key);
   CS104_Slave_setConnectionEventHandler(slave, &Server::connectionEventHandler,
-                                        this);
-  CS104_Slave_setRawMessageHandler(slave, &Server::rawMessageHandler, this);
+                                        key);
+  CS104_Slave_setRawMessageHandler(slave, &Server::rawMessageHandler, key);
   CS104_Slave_setInterrogationHandler(slave, &Server::interrogationHandler,
-                                      this);
+                                      key);
   CS104_Slave_setCounterInterrogationHandler(
-      slave, &Server::counterInterrogationHandler, this);
-  // CS104_Slave_setClockSyncHandler(slave, &Server::clockSyncHandler, this);
-  CS104_Slave_setReadHandler(slave, &Server::readHandler, this);
-  CS104_Slave_setASDUHandler(slave, &Server::asduHandler, this);
+      slave, &Server::counterInterrogationHandler, key);
+  CS104_Slave_setReadHandler(slave, &Server::readHandler, key);
+  CS104_Slave_setASDUHandler(slave, &Server::asduHandler, key);
 
   DEBUG_PRINT(Debug::Server, "Created");
 }
 
 Server::~Server() {
   // stops and destroys the slave
   stop();
-  CS104_Slave_destroy(slave);
 
   {
     std::lock_guard<Module::GilAwareMutex> const st_lock(station_mutex);
     stations.clear();
   }
 
+  {
+    std::lock_guard<std::mutex> lock(instanceMapMutex);
+    instanceMap.erase(
+        static_cast<void *>(this)); // Remove from map on destruction
+  }
+
+  CS104_Slave_destroy(slave);
+
   DEBUG_PRINT(Debug::Server, "Removed");
 }
 
+std::shared_ptr<Server> Server::getInstance(void *key) {
+  std::lock_guard<std::mutex> lock(instanceMapMutex);
+  auto it = instanceMap.find(key);
+  if (it != instanceMap.end()) {
+    return it->second.lock();
+  }
+  return {nullptr};
+}
+
 std::string Server::getIP() const { return ip; }
 
 std::uint_fast16_t Server::getPort() const { return port; }
@@ -782,11 +802,8 @@ void Server::setOnConnectCallback(py::object &callable) {
 std::uint_fast16_t Server::getTickRate_ms() const { return tickRate_ms; }
 
 bool Server::connectionRequestHandler(void *parameter, const char *ipAddress) {
-  std::shared_ptr<Server> instance{};
-
-  try {
-    instance = static_cast<Server *>(parameter)->shared_from_this();
-  } catch (const std::bad_weak_ptr &e) {
+  const auto instance = getInstance(parameter);
+  if (!instance) {
     DEBUG_PRINT(Debug::Server, "Reject connection request in shutdown");
     return false;
   }
@@ -818,11 +835,8 @@ void Server::connectionEventHandler(void *parameter,
     begin = std::chrono::steady_clock::now();
   }
 
-  std::shared_ptr<Server> instance{};
-
-  try {
-    instance = static_cast<Server *>(parameter)->shared_from_this();
-  } catch (const std::bad_weak_ptr &e) {
+  const auto instance = getInstance(parameter);
+  if (!instance) {
     DEBUG_PRINT(Debug::Server, "Ignore connection event " +
                                    PeerConnectionEvent_toString(event) +
                                    " in shutdown");
@@ -1259,11 +1273,8 @@ Server::getValidMessage(IMasterConnection connection, CS101_ASDU asdu) {
 
 void Server::rawMessageHandler(void *parameter, IMasterConnection connection,
                                uint_fast8_t *msg, int msgSize, bool sent) {
-  std::shared_ptr<Server> instance{};
-
-  try {
-    instance = static_cast<Server *>(parameter)->shared_from_this();
-  } catch (const std::bad_weak_ptr &e) {
+  const auto instance = getInstance(parameter);
+  if (!instance) {
     DEBUG_PRINT(Debug::Server, "Ignore raw message in shutdown");
     return;
   }
@@ -1284,11 +1295,8 @@ bool Server::interrogationHandler(void *parameter, IMasterConnection connection,
     begin = std::chrono::steady_clock::now();
   }
 
-  std::shared_ptr<Server> instance{};
-
-  try {
-    instance = static_cast<Server *>(parameter)->shared_from_this();
-  } catch (const std::bad_weak_ptr &e) {
+  const auto instance = getInstance(parameter);
+  if (!instance) {
     DEBUG_PRINT(Debug::Server, "Reject interrogation command in shutdown");
     return false;
   }
@@ -1381,11 +1389,8 @@ bool Server::counterInterrogationHandler(void *parameter,
     begin = std::chrono::steady_clock::now();
   }
 
-  std::shared_ptr<Server> instance{};
-
-  try {
-    instance = static_cast<Server *>(parameter)->shared_from_this();
-  } catch (const std::bad_weak_ptr &e) {
+  const auto instance = getInstance(parameter);
+  if (!instance) {
     DEBUG_PRINT(Debug::Server,
                 "Reject counter interrogation command in shutdown");
     return false;
@@ -1482,11 +1487,8 @@ bool Server::readHandler(void *parameter, IMasterConnection connection,
     begin = std::chrono::steady_clock::now();
   }
 
-  std::shared_ptr<Server> instance{};
-
-  try {
-    instance = static_cast<Server *>(parameter)->shared_from_this();
-  } catch (const std::bad_weak_ptr &e) {
+  const auto instance = getInstance(parameter);
+  if (!instance) {
     DEBUG_PRINT(Debug::Server, "Reject read command in shutdown");
     return false;
   }
@@ -1562,11 +1564,8 @@ bool Server::asduHandler(void *parameter, IMasterConnection connection,
     begin = std::chrono::steady_clock::now();
   }
 
-  std::shared_ptr<Server> instance{};
-
-  try {
-    instance = static_cast<Server *>(parameter)->shared_from_this();
-  } catch (const std::bad_weak_ptr &e) {
+  const auto instance = getInstance(parameter);
+  if (!instance) {
     DEBUG_PRINT(Debug::Server, "Reject asdu in shutdown");
     return false;
   }

src/Server.h

@@ -62,7 +62,15 @@ struct Selection {
 };
 
 /**
- * @brief service model for IEC60870-5-104 communication as server
+ * @brief Represents a server capable of handling connections, stations,
+ * and transport security for a network-based application.
+ *
+ * The `Server` class is designed to manage server-client communication,
+ * handle multiple connections, and interact with stations defined on this
+ * server. It provides robust features for managing connection limits,
+ * executing protocol-specific parameters, and integrating externally defined
+ * callback execution. The server also supports transport security options
+ * and schedule-based periodic tasks.
  */
 class Server : public std::enable_shared_from_this<Server> {
 
@@ -103,9 +111,16 @@ class Server : public std::enable_shared_from_this<Server> {
       std::uint_fast8_t max_open_connections = 0,
       std::shared_ptr<Remote::TransportSecurity> transport_security = nullptr) {
     // Not using std::make_shared because the constructor is private.
-    return std::shared_ptr<Server>(
+    auto server = std::shared_ptr<Server>(
         new Server(bind_ip, tcp_port, tick_rate_ms, select_timeout_ms,
                    max_open_connections, std::move(transport_security)));
+
+    // track reference as weak pointer for safe static callbacks
+    void *key = static_cast<void *>(server.get());
+    std::lock_guard<std::mutex> lock(instanceMapMutex);
+    instanceMap[key] = server;
+
+    return server;
   }
 
   // DESTRUCTOR
@@ -675,6 +690,12 @@ class Server : public std::enable_shared_from_this<Server> {
   /// another loop
   mutable std::condition_variable_any runThread_wait{};
 
+  /// @brief instance weak pointer list for safe static callbacks
+  static std::unordered_map<void *, std::weak_ptr<Server>> instanceMap;
+
+  /// @brief mutex to lock instanceMap read/write access
+  static std::mutex instanceMapMutex;
+
   /// @brief python callback function pointer
   Module::Callback<void> py_onReceiveRaw{
       "Server.on_receive_raw", "(server: c104.Server, data: bytes) -> None"};
@@ -706,8 +727,41 @@ class Server : public std::enable_shared_from_this<Server> {
   // void thread_callback();
 
 public:
+  /**
+   * @brief Retrieves the selector associated with a given common address (ca)
+   * and information object address (ioa) from the selection vector.
+   *
+   * This function searches for a matching selector in the selection vector
+   * based on the provided parameters. If a selection is found
+   * and the time elapsed since its creation does not exceed
+   * the configured selection timeout, the function returns the selector;
+   * otherwise, it returns an empty optional.
+   *
+   * @param ca The common address to search for.
+   * @param ioa The information object address to search for.
+   * @return An optional containing the selector if found and valid;
+   *         otherwise, an empty optional.
+   */
   std::optional<uint8_t> getSelector(uint16_t ca, uint32_t ioa);
 
+  /**
+   * @brief Retrieves the shared instance of the Server associated with the
+   * given key.
+   *
+   * This function is a thread-safe method to access Server instances stored in
+   * an internal instance map. A weak reference is used for storage, and the
+   * function returns a shared pointer to the associated Server instance. If the
+   * key is not found in the map or the associated weak reference has expired, a
+   * null shared pointer is returned.
+   *
+   * @param key A pointer serving as the unique identifier for the desired
+   * Server instance.
+   * @return A shared pointer to the Server instance associated with the given
+   * key, or a null shared pointer if the key is not found or the instance has
+   * been deallocated.
+   */
+  static std::shared_ptr<Server> getInstance(void *key);
+
   /**
    * @brief Callback to accept or decline incoming client connections
    * @param parameter reference to custom bound connection data