python accessing freed shared memory object (#1253)

* added a 'isValid' member in shared memory (also updated shm version) with default true, any access to shared memory() checks also for validity. any free will set this to false and then unmap shm. Any access to shm will then check validity in python.

* fixed tests for shm

* added tests in python as well

---------

Co-authored-by: Alice <alice.mazzoleni@psi.ch>

slsDetectorSoftware/src/CtbConfig.h

@@ -56,6 +56,7 @@ class CtbConfig {
     std::string getSlowADCName(size_t index) const;
     std::vector<std::string> getSlowADCNames() const;
     static const char *shm_tag();
+    bool isValid{true}; // false if freed to block access from python or c++ api
 };
 
 } // namespace sls

slsDetectorSoftware/src/Detector.cpp

@@ -29,6 +29,8 @@ void freeSharedMemory(const int detectorIndex, const int moduleIndex) {
     if (moduleIndex >= 0) {
         SharedMemory<sharedModule> moduleShm(detectorIndex, moduleIndex);
         if (moduleShm.exists()) {
+            moduleShm.openSharedMemory(false);
+            moduleShm()->isValid = false;
             moduleShm.removeSharedMemory();
         }
         return;
@@ -41,18 +43,26 @@ void freeSharedMemory(const int detectorIndex, const int moduleIndex) {
     if (detectorShm.exists()) {
         detectorShm.openSharedMemory(false);
         numDetectors = detectorShm()->totalNumberOfModules;
+        detectorShm()->isValid = false;
         detectorShm.removeSharedMemory();
     }
 
     for (int i = 0; i < numDetectors; ++i) {
         SharedMemory<sharedModule> moduleShm(detectorIndex, i);
+        if (moduleShm.exists()) {
+            moduleShm.openSharedMemory(false);
+            moduleShm()->isValid = false;
+        }
         moduleShm.removeSharedMemory();
     }
 
     // Ctb configuration
     SharedMemory<CtbConfig> ctbShm(detectorIndex, -1, CtbConfig::shm_tag());
-    if (ctbShm.exists())
+    if (ctbShm.exists()) {
+        ctbShm.openSharedMemory(false);
+        ctbShm()->isValid = false;
         ctbShm.removeSharedMemory();
+    }
 }
 
 using defs = slsDetectorDefs;

slsDetectorSoftware/src/DetectorImpl.h

@@ -24,7 +24,7 @@ class detectorData;
 class Module;
 
 #define DETECTOR_SHMAPIVERSION 0x190809
-#define DETECTOR_SHMVERSION    0x250616
+#define DETECTOR_SHMVERSION    0x250729
 #define SHORT_STRING_LENGTH    50
 
 /**
@@ -51,6 +51,8 @@ struct sharedDetector {
     int totalNumberOfModules;
     slsDetectorDefs::detectorType detType;
 
+    bool isValid{true}; // false if freed to block access from python or c++ api
+
     /** END OF FIXED PATTERN
      * -----------------------------------------------*/
 

slsDetectorSoftware/src/Module.h

@@ -19,7 +19,7 @@ namespace sls {
 class ServerInterface;
 
 #define MODULE_SHMAPIVERSION 0x190726
-#define MODULE_SHMVERSION    0x230913
+#define MODULE_SHMVERSION    0x250729
 
 /**
  * @short structure allocated in shared memory to store Module settings for
@@ -32,6 +32,7 @@ struct sharedModule {
     int shmversion;
     char hostname[MAX_STR_LENGTH];
     slsDetectorDefs::detectorType detType;
+    bool isValid{true}; // false if freed to block access from python or c++ api
 
     /** END OF FIXED PATTERN -----------------------------------------------*/
 

slsDetectorSoftware/src/SharedMemory.h

@@ -10,6 +10,7 @@
  *@short functions basic implemenation of shared memory
  */
 
+#include "sls/TypeTraits.h"
 #include "sls/logger.h"
 #include "sls/sls_detector_exceptions.h"
 
@@ -26,11 +27,18 @@
 
 namespace sls {
 
+struct sharedDetector;
+
 #define SHM_DETECTOR_PREFIX "/slsDetectorPackage_detector_"
 #define SHM_MODULE_PREFIX   "_module_"
 #define SHM_ENV_NAME        "SLSDETNAME"
 
 template <typename T> class SharedMemory {
+
+    static_assert(has_bool_isValid<T>::value,
+                  "SharedMemory requires the struct to have a bool member "
+                  "named 'isValid'");
+
     static constexpr int NAME_MAX_LENGTH = 255;
     std::string name;
     T *shared_struct{nullptr};
@@ -65,15 +73,21 @@ template <typename T> class SharedMemory {
     }
 
     T *operator()() {
-        if (shared_struct)
-            return shared_struct;
-        throw SharedMemoryError(getNoShmAccessMessage());
+        if (!shared_struct)
+            throw SharedMemoryError(getNoShmAccessMessage());
+        if (!shared_struct->isValid) {
+            throw SharedMemoryError(getInvalidShmMessage());
+        }
+        return shared_struct;
     }
 
     const T *operator()() const {
-        if (shared_struct)
-            return shared_struct;
-        throw SharedMemoryError(getNoShmAccessMessage());
+        if (!shared_struct)
+            throw SharedMemoryError(getNoShmAccessMessage());
+        if (!shared_struct->isValid) {
+            throw SharedMemoryError(getInvalidShmMessage());
+        }
+        return shared_struct;
     }
 
     std::string getName() const { return name; }
@@ -215,10 +229,15 @@ template <typename T> class SharedMemory {
         }
     }
 
-    const char *getNoShmAccessMessage() const {
+    inline const char *getNoShmAccessMessage() const {
         return ("No shared memory to access. Create it first with "
                 "hostname or config command.");
     };
+
+    inline const char *getInvalidShmMessage() const {
+        return ("Shared memory is invalid or freed. Close resources before "
+                "access.");
+    };
 };
 
 } // namespace sls

slsDetectorSoftware/tests/test-CtbConfig.cpp

@@ -10,7 +10,7 @@
 namespace sls {
 
 TEST_CASE("Default construction") {
-    static_assert(sizeof(CtbConfig) == ((18 + 32 + 64 + 5 + 8) * 20),
+    static_assert(sizeof(CtbConfig) == ((18 + 32 + 64 + 5 + 8) * 20 + 1),
                   "Size of CtbConfig does not match");
 
     CtbConfig c;

slsDetectorSoftware/tests/test-SharedMemory.cpp

@@ -5,27 +5,35 @@
 #include "catch.hpp"
 #include "sls/string_utils.h"
 
-#include <iostream>
-
 namespace sls {
 
 struct Data {
     int x;
     double y;
     char mess[50];
+    bool isValid{true};
 };
 
+void freeShm(const int dindex, const int mIndex) {
+    SharedMemory<Data> shm(dindex, mIndex);
+    if (shm.exists()) {
+        shm.openSharedMemory(false);
+        shm()->isValid = false;
+        shm.removeSharedMemory();
+    }
+}
+
 constexpr int shm_id = 10;
 
 TEST_CASE("Create SharedMemory read and write", "[detector]") {
-    const char *env_p = std::getenv("SLSDETNAME");
-    std::string env_name = env_p ? ("_" + std::string(env_p)) : "";
-
     SharedMemory<Data> shm(shm_id, -1);
     if (shm.exists()) {
         shm.removeSharedMemory();
     }
     shm.createSharedMemory();
+
+    const char *env_p = std::getenv("SLSDETNAME");
+    std::string env_name = env_p ? ("_" + std::string(env_p)) : "";
     CHECK(shm.getName() == std::string("/slsDetectorPackage_detector_") +
                                std::to_string(shm_id) + env_name);
 
@@ -44,16 +52,19 @@ TEST_CASE("Create SharedMemory read and write", "[detector]") {
 }
 
 TEST_CASE("Open existing SharedMemory and read", "[detector]") {
-
     {
-        SharedMemory<double> shm(shm_id, -1);
+        SharedMemory<Data> shm(shm_id, -1);
+        if (shm.exists()) {
+            shm.removeSharedMemory();
+        }
         shm.createSharedMemory();
-        *shm() = 5.3;
+        shm()->x = 3;
+        shm()->y = 5.9;
     }
 
-    SharedMemory<double> shm2(shm_id, -1);
+    SharedMemory<Data> shm2(shm_id, -1);
     shm2.openSharedMemory(true);
-    CHECK(*shm2() == 5.3);
+    CHECK(shm2()->y == 5.9);
 
     shm2.removeSharedMemory();
 }
@@ -61,8 +72,8 @@ TEST_CASE("Open existing SharedMemory and read", "[detector]") {
 TEST_CASE("Creating a second shared memory with the same name throws",
           "[detector]") {
 
-    SharedMemory<double> shm0(shm_id, -1);
-    SharedMemory<double> shm1(shm_id, -1);
+    SharedMemory<Data> shm0(shm_id, -1);
+    SharedMemory<Data> shm1(shm_id, -1);
 
     shm0.createSharedMemory();
     CHECK_THROWS(shm1.createSharedMemory());
@@ -120,19 +131,18 @@ TEST_CASE("Create several shared memories", "[detector]") {
     std::string env_name = env_p ? ("_" + std::string(env_p)) : "";
 
     constexpr int N = 5;
-    std::vector<SharedMemory<int>> v;
+    std::vector<SharedMemory<Data>> v;
     v.reserve(N);
     for (int i = 0; i != N; ++i) {
-        std::cout << "i:" << i << std::endl;
         v.emplace_back(shm_id + i, -1);
         CHECK(v[i].exists() == false);
         v[i].createSharedMemory();
-        *v[i]() = i;
-        CHECK(*v[i]() == i);
+        v[i]()->x = i;
+        CHECK(v[i]()->x == i);
     }
 
     for (int i = 0; i != N; ++i) {
-        CHECK(*v[i]() == i);
+        CHECK(v[i]()->x == i);
         CHECK(v[i].getName() == std::string("/slsDetectorPackage_detector_") +
                                     std::to_string(i + shm_id) + env_name);
     }
@@ -147,7 +157,7 @@ TEST_CASE("Create create a shared memory with a tag") {
     const char *env_p = std::getenv("SLSDETNAME");
     std::string env_name = env_p ? ("_" + std::string(env_p)) : "";
 
-    SharedMemory<int> shm(0, -1, "ctbdacs");
+    SharedMemory<Data> shm(0, -1, "ctbdacs");
     REQUIRE(shm.getName() ==
             "/slsDetectorPackage_detector_0" + env_name + "_ctbdacs");
 }
@@ -162,7 +172,7 @@ TEST_CASE("Create create a shared memory with a tag when SLSDETNAME is set") {
     unsetenv(SHM_ENV_NAME);
     setenv(SHM_ENV_NAME, "myprefix", 1);
 
-    SharedMemory<int> shm(0, -1, "ctbdacs");
+    SharedMemory<Data> shm(0, -1, "ctbdacs");
     REQUIRE(shm.getName() == "/slsDetectorPackage_detector_0_myprefix_ctbdacs");
 
     // Clean up after us
@@ -172,15 +182,14 @@ TEST_CASE("Create create a shared memory with a tag when SLSDETNAME is set") {
         setenv(SHM_ENV_NAME, old_slsdetname.c_str(), 1);
 }
 
-TEST_CASE("map int64 to int32 throws") {
-    SharedMemory<int32_t> shm(shm_id, -1);
+TEST_CASE("Access to already freed shm object", "[detector]") {
+    SharedMemory<Data> shm(shm_id, -1);
     shm.createSharedMemory();
-    *shm() = 7;
+    shm()->x = 10;
 
-    SharedMemory<int64_t> shm2(shm_id, -1);
-    REQUIRE_THROWS(shm2.openSharedMemory(true));
-
-    shm.removeSharedMemory();
+    freeShm(shm_id, -1);
+    CHECK(shm.exists() == false);
+    REQUIRE_THROWS(shm()); // trying to access should throw
 }
 
 } // namespace sls

slsSupportLib/include/sls/TypeTraits.h

@@ -112,4 +112,12 @@ struct Conjunction<B1, Bn...>
 template <typename T, typename... Ts>
 using AllSame =
     typename std::enable_if<Conjunction<std::is_same<T, Ts>...>::value>::type;
+
+// Trait to detect if T has a bool member named 'isValid'
+template <typename, typename = std::void_t<>>
+struct has_bool_isValid : std::false_type {};
+
+template <typename T>
+struct has_bool_isValid<T, std::void_t<decltype(std::declval<T>().isValid)>>
+    : std::is_same<decltype(std::declval<T>().isValid), bool> {};
 } // namespace sls

tests/CMakeLists.txt

@@ -63,3 +63,4 @@ configure_file(scripts/test_simulators.py ${CMAKE_BINARY_DIR}/bin/test_simulator
 configure_file(scripts/test_frame_synchronizer.py ${CMAKE_BINARY_DIR}/bin/test_frame_synchronizer.py COPYONLY)
 configure_file(scripts/utils_for_test.py ${CMAKE_BINARY_DIR}/bin/utils_for_test.py COPYONLY)
 configure_file(scripts/test_roi.py ${CMAKE_BINARY_DIR}/bin/test_roi.py COPYONLY)
+configure_file(scripts/test_free.py ${CMAKE_BINARY_DIR}/bin/test_free.py COPYONLY)

tests/scripts/test_free.py

@@ -0,0 +1,164 @@
+# SPDX-License-Identifier: LGPL-3.0-or-other
+# Copyright (C) 2021 Contributors to the SLS Detector Package
+'''
+This file is used to start up simulators and test for freeing shm and accessing it from python.
+Run this using: pytest -s test_free.py
+'''
+
+import pytest, sys
+
+from slsdet import Detector, Ctb, freeSharedMemory
+from utils_for_test import (
+    Log,
+    LogLevel,
+    cleanup,
+    startDetectorVirtualServer,
+    connectToVirtualServers,
+    SERVER_START_PORTNO
+)
+
+'''
+scope = module =>Once per test file/module 
+to share expensive setup like startDetectorVirtualServer
+'''
+@pytest.fixture(scope="module")
+def det_config():
+    return {
+        "name": "ctb",
+        "num_mods": 1
+    }
+
+@pytest.fixture(scope="module", autouse=True)
+def setup_simulator(det_config):
+    """Fixture to start the detector server once and clean up at the end."""
+    fp = sys.stdout
+
+    cleanup(fp)
+    startDetectorVirtualServer(det_config["name"], det_config["num_mods"], fp)
+
+    Log(LogLevel.INFOBLUE, f'Waiting for server to start up and connect')
+    connectToVirtualServers(det_config["name"], det_config["num_mods"])
+    Log(LogLevel.INFOBLUE, f'Freeing shm before tests')
+    freeSharedMemory()
+
+    yield  # tests run here
+
+    cleanup(fp)
+
+
+
+def test_exptime_after_free_should_raise(setup_simulator):
+    Log(LogLevel.INFOBLUE, f'\nRunning test_exptime_after_free_should_raise')
+
+
+    d = Ctb() # creates multi shm (assuming no shm exists)
+    d.hostname = f"localhost:{SERVER_START_PORTNO}" # hostname command creates mod shm, d maps to it
+
+    d.free() # frees the shm, d should not map to it anymore
+
+    # accessing invalid shm should throw
+    with pytest.raises(Exception) as exc_info:
+        _ = d.exptime
+
+    Log(LogLevel.INFOGREEN, f"✅ Test passed, exception was: {exc_info.value}")
+    assert str(exc_info.value) == "Shared memory is invalid or freed. Close resources before access."
+
+
+
+
+
+def free_and_create_shm():
+    k = Ctb() # opens existing shm if it exists
+    k.hostname = f"localhost:{SERVER_START_PORTNO}" # free and recreate shm, maps to local shm struct
+
+
+def test_exptime_after_not_passing_var_should_raise(setup_simulator):
+    Log(LogLevel.INFOBLUE, f'\nRunning test_exptime_after_not_passing_var_should_raise')
+
+
+    d = Ctb() # creates multi shm (assuming no shm exists)
+    d.hostname = f"localhost:{SERVER_START_PORTNO}" # hostname command creates mod shm, d maps to it
+
+    free_and_create_shm() # ctb() opens multi shm, hostname command frees and recreates mod shm but shm struct is local. d still maps to old shm struct
+
+    # accessing invalid shm should throw
+    with pytest.raises(Exception) as exc_info:
+        _ = d.exptime
+
+    Log(LogLevel.INFOGREEN, f"✅ Test passed, exception was: {exc_info.value}")
+    assert str(exc_info.value) == "Shared memory is invalid or freed. Close resources before access."
+
+
+
+
+def free_and_create_shm_passing_ctb_var(k):
+    k = Ctb() # opens existing shm if it exists (disregards k as its new Ctb only local to this function)
+    k.hostname = f"localhost:{SERVER_START_PORTNO}" # free and recreate shm, maps to local shm struct
+
+
+def test_exptime_after_passing_ctb_var_should_raise(setup_simulator):
+    Log(LogLevel.INFOBLUE, f'\nRunning test_exptime_after_passing_ctb_var_should_raise')
+
+    d = Ctb() # creates multi shm (assuming no shm exists)
+    d.hostname = f"localhost:{SERVER_START_PORTNO}" # hostname command creates mod shm, d maps to it
+
+    free_and_create_shm_passing_ctb_var(d) # ctb() opens multi shm, hostname command frees and recreates mod shm but shm struct is local. d still maps to old shm struct
+
+    # accessing invalid shm should throw
+    with pytest.raises(Exception) as exc_info:
+        _ = d.exptime
+
+    Log(LogLevel.INFOGREEN, f"✅ Test passed, exception was: {exc_info.value}")
+    assert str(exc_info.value) == "Shared memory is invalid or freed. Close resources before access."
+
+
+
+def free_and_create_shm_returning_ctb():
+    k = Ctb() # opens existing shm if it exists (disregards k as its new Ctb only local to this function)
+    k.hostname = f"localhost:{SERVER_START_PORTNO}" # free and recreate shm, maps to local shm struct
+    return k
+
+
+def test_exptime_after_returning_ctb_should_raise(setup_simulator):
+    Log(LogLevel.INFOBLUE, f'\nRunning test_exptime_after_returning_ctb_should_raise')
+
+    d = Ctb() # creates multi shm (assuming no shm exists)
+
+    d = free_and_create_shm_returning_ctb() # ctb() opens multi shm, hostname command frees and recreates mod shm but shm struct is local but returned. d now maps to the new sturct
+
+    # this should not throw 
+    exptime_val = d.exptime
+
+    Log(LogLevel.INFOGREEN, f"✅ Test passed, exptime was: {exptime_val}")
+    assert isinstance(exptime_val, float)
+
+    free_and_create_shm_returning_ctb() # this time d is not updated, it maps to the old shm struct
+
+    # accessing invalid shm should throw
+    with pytest.raises(Exception) as exc_info:
+        _ = d.exptime
+
+    Log(LogLevel.INFOGREEN, f"✅ Test passed, exception was: {exc_info.value}")
+    assert str(exc_info.value) == "Shared memory is invalid or freed. Close resources before access."
+
+
+
+
+
+
+def test_hostname_twice_acess_old_should_raise(setup_simulator):
+    Log(LogLevel.INFOBLUE, f'\nRunning test_hostname_twice_acess_old_should_raise')
+
+    d = Ctb() # creates multi shm (assuming no shm exists)
+    d.hostname = f"localhost:{SERVER_START_PORTNO}" # hostname command creates mod shm, d maps to it
+    d.hostname = f"localhost:{SERVER_START_PORTNO}"  # Freeing and recreating shm while mapping d to it (old shm is out of scope)
+    
+    # this should not throw 
+    exptime_val = d.exptime
+
+    Log(LogLevel.INFOGREEN, f"✅ Test passed, exptime was: {exptime_val}")
+    assert isinstance(exptime_val, float)
+
+
+
+

tests/scripts/utils_for_test.py

@@ -9,7 +9,7 @@ from enum import Enum
 from colorama import Fore, Style, init
 from datetime import timedelta
 
-from slsdet import Detector, detectorSettings, burstMode
+from slsdet import Detector, Ctb, detectorSettings, burstMode
 from slsdet.defines import DEFAULT_TCP_RX_PORTNO, DEFAULT_UDP_DST_PORTNO
 SERVER_START_PORTNO=1900
 
@@ -167,9 +167,12 @@ def startDetectorVirtualServer(name :str, num_mods, fp):
                 time.sleep(3)
 
 
-def connectToVirtualServers(name, num_mods):
+def connectToVirtualServers(name, num_mods, ctb_object=False):
     try:
-        d = Detector()
+        if ctb_object:
+            d = Ctb()
+        else:
+            d = Detector()
     except Exception as e:
         raise RuntimeException(f'Could not create Detector object for {name}. Error: {str(e)}') from e