NVMeContext: Rework sensor removal concurrent to polling

Concurrent removal of a sensor's configuration while the sensor list is being iterated for polling can lead to undefined behaviour via access through a deleted iterator: Program terminated with signal SIGSEGV, Segmentation fault. #0 std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count (__r=..., this=<optimised out>, this=<optimised out>, __r=...) at /usr/include/c++/11.2.0/bits/stl_list.h:224 224 /usr/include/c++/11.2.0/bits/stl_list.h: No such file or directory. [Current thread is 1 (LWP 6649)] #0 std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count (__r=..., this=<optimised out>, this=<optimised out>, __r=...) at /usr/include/c++/11.2.0/bits/stl_list.h:224 #1 std::__shared_ptr<NVMeSensor, (__gnu_cxx::_Lock_policy)2>::__shared_ptr (this=<optimised out>, this=<optimised out>) at /usr/include/c++/11.2.0/bits/shared_ptr_base.h:1152 #2 std::shared_ptr<NVMeSensor>::shared_ptr (this=<optimised out>, this=<optimised out>) at /usr/include/c++/11.2.0/bits/shared_ptr.h:150 #3 NVMeBasicContext::readAndProcessNVMeSensor (this=0x1ac8a90, iter=non-dereferenceable iterator for std::list) at ../git/src/NVMeBasicContext.cpp:299 #4 0x004dd8b8 in NVMeBasicContext::readAndProcessNVMeSensor (this=0x1ac8a90, iter=non-dereferenceable iterator for std::list) at ../git/src/NVMeBasicContext.cpp:312 #5 0x004dd8b8 in NVMeBasicContext::readAndProcessNVMeSensor (this=0x1ac8a90, iter=std::shared_ptr<NVMeSensor> (use count 26, weak count 28153871) = {get() = 0x1ad8db0}) at ../git/src/NVMeBasicContext.cpp:312 Rework polling and sensor removal to uphold the requirement that the iterator remains valid. Signed-off-by: Andrew Jeffery <[email protected]> Change-Id: I69b005fe3dad7ddf21d1762731f9cdfd2408cae1
openbmc · May 5, 2022 · b5d7a7f · b5d7a7f
1 parent 0b1ae9f
commit b5d7a7f
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 30 deletions.
diff --git a/include/NVMeBasicContext.hpp b/include/NVMeBasicContext.hpp
@@ -11,8 +11,7 @@ class NVMeBasicContext : public NVMeContext
     NVMeBasicContext(boost::asio::io_service& io, int rootBus);
     ~NVMeBasicContext() override = default;
     void pollNVMeDevices() override;
-    void readAndProcessNVMeSensor(
-        std::list<std::shared_ptr<NVMeSensor>>::iterator iter) override;
+    void readAndProcessNVMeSensor() override;
     void processResponse(std::shared_ptr<NVMeSensor>& sensor, void* msg,
                          size_t len) override;
 

diff --git a/include/NVMeContext.hpp b/include/NVMeContext.hpp
@@ -12,7 +12,7 @@ class NVMeContext : public std::enable_shared_from_this<NVMeContext>
 {
   public:
     NVMeContext(boost::asio::io_service& io, int rootBus) :
-        scanTimer(io), rootBus(rootBus)
+        scanTimer(io), rootBus(rootBus), pollCursor(sensors.end())
     {
         if (rootBus < 0)
         {
@@ -45,9 +45,44 @@ class NVMeContext : public std::enable_shared_from_this<NVMeContext>
         return std::nullopt;
     }
 
+    // Post-condition: The sensor list does not contain the provided sensor
+    // Post-condition: pollCursor is a valid iterator for the sensor list
     void removeSensor(const std::shared_ptr<NVMeSensor>& sensor)
     {
-        sensors.remove(sensor);
+        // Locate the sensor that we're removing in the sensor list
+        auto found = std::find(sensors.begin(), sensors.end(), sensor);
+
+        // If we failed to find the sensor in the list the post-condition is
+        // already satisfied
+        if (found == sensors.end())
+        {
+            return;
+        }
+
+        // We've found the sensor in the list
+
+        // If we're not actively polling the sensor list, then remove the sensor
+        if (pollCursor == sensors.end())
+        {
+            sensors.erase(found);
+            return;
+        }
+
+        // We're actively polling the sensor list
+
+        // If we're not polling the specific sensor that has been removed, then
+        // remove the sensor
+        if (*pollCursor != *found)
+        {
+            sensors.erase(found);
+            return;
+        }
+
+        // We're polling the sensor that is being removed
+
+        // Remove the sensor and update the poll cursor so the cursor remains
+        // valid
+        pollCursor = sensors.erase(found);
     }
 
     virtual void close()
@@ -57,16 +92,16 @@ class NVMeContext : public std::enable_shared_from_this<NVMeContext>
 
     virtual void pollNVMeDevices() = 0;
 
-    virtual void readAndProcessNVMeSensor(
-        std::list<std::shared_ptr<NVMeSensor>>::iterator iter) = 0;
+    virtual void readAndProcessNVMeSensor() = 0;
 
     virtual void processResponse(std::shared_ptr<NVMeSensor>& sensor, void* msg,
                                  size_t len) = 0;
 
   protected:
     boost::asio::deadline_timer scanTimer;
-    int rootBus;                                    // Root bus for this drive
-    std::list<std::shared_ptr<NVMeSensor>> sensors; // used as a poll queue
+    int rootBus; // Root bus for this drive
+    std::list<std::shared_ptr<NVMeSensor>> sensors;
+    std::list<std::shared_ptr<NVMeSensor>>::iterator pollCursor;
 };
 
 using NVMEMap = boost::container::flat_map<int, std::shared_ptr<NVMeContext>>;

diff --git a/src/NVMeBasicContext.cpp b/src/NVMeBasicContext.cpp
@@ -246,29 +246,28 @@ NVMeBasicContext::NVMeBasicContext(boost::asio::io_service& io, int rootBus) :
     thread.detach();
 }
 
-void NVMeBasicContext::readAndProcessNVMeSensor(
-    std::list<std::shared_ptr<NVMeSensor>>::iterator iter)
+void NVMeBasicContext::readAndProcessNVMeSensor()
 {
-    if (iter == sensors.end())
+    if (pollCursor == sensors.end())
     {
         this->pollNVMeDevices();
         return;
     }
 
-    std::shared_ptr<NVMeSensor> sensor = *iter++;
+    std::shared_ptr<NVMeSensor> sensor = *pollCursor++;
 
     if (!sensor->readingStateGood())
     {
         sensor->markAvailable(false);
         sensor->updateValue(std::numeric_limits<double>::quiet_NaN());
-        readAndProcessNVMeSensor(iter);
+        readAndProcessNVMeSensor();
         return;
     }
 
     /* Potentially defer sampling the sensor sensor if it is in error */
     if (!sensor->sample())
     {
-        readAndProcessNVMeSensor(iter);
+        readAndProcessNVMeSensor();
         return;
     }
 
@@ -326,7 +325,7 @@ void NVMeBasicContext::readAndProcessNVMeSensor(
             response->prepare(len);
             return len;
         },
-        [self{shared_from_this()}, iter, sensor, response](
+        [self{shared_from_this()}, sensor, response](
             const boost::system::error_code& ec, std::size_t length) mutable {
             if (ec)
             {
@@ -350,30 +349,30 @@ void NVMeBasicContext::readAndProcessNVMeSensor(
             self->processResponse(sensor, data.data(), data.size());
 
             /* Enqueue processing of the next sensor */
-            self->readAndProcessNVMeSensor(iter);
+            self->readAndProcessNVMeSensor();
         });
 }
 
 void NVMeBasicContext::pollNVMeDevices()
 {
-    auto scan = sensors.begin();
+    pollCursor = sensors.begin();
 
     scanTimer.expires_from_now(boost::posix_time::seconds(1));
-    scanTimer.async_wait([self{shared_from_this()},
-                          scan](const boost::system::error_code errorCode) {
-        if (errorCode == boost::asio::error::operation_aborted)
-        {
-            return;
-        }
+    scanTimer.async_wait(
+        [self{shared_from_this()}](const boost::system::error_code errorCode) {
+            if (errorCode == boost::asio::error::operation_aborted)
+            {
+                return;
+            }
 
-        if (errorCode)
-        {
-            std::cerr << errorCode.message() << "\n";
-            return;
-        }
+            if (errorCode)
+            {
+                std::cerr << errorCode.message() << "\n";
+                return;
+            }
 
-        self->readAndProcessNVMeSensor(scan);
-    });
+            self->readAndProcessNVMeSensor();
+        });
 }
 
 static double getTemperatureReading(int8_t reading)