cpuSensor:Check object validity in async callback
Sometimes the cpusensor crash was observed. The coredump was pointing
to segmentation fault while executing callback function inside
setupRead().
Similar issue was observed in adcsensor.
Reference to the fix: 1afda6bbb6db0e266795af3229b962c32775b928
The timer cancel() in destructor cannot cancel the expired callback
handlers. In such cases if the callback handler is executed, the
object is deleted already.
Use weak_ptr to check if the object is still valid before using these
member variables/functions.
The buffer used by async_read_util must guarantee that they remain
valid until the handler is called.
Tested: Stress tested by restarting the service. Service restart will
force re-creating the CPUSensor objects.
`count=0; \
while true; \
do systemctl restart xyz.openbmc_project.cpusensor.service; \
count=$((count + 1)); \
echo $count; \
sleep 40; \
done `
Signed-off-by: gokulsanker <gokul.sanker.v.g@intel.com>
Signed-off-by: Arun P. Mohanan <arun.p.m@linux.intel.com>
Change-Id: I7410feb0e17c5f6d555cd042f5e5b327de1910c5
diff --git a/include/CPUSensor.hpp b/include/CPUSensor.hpp
index 882d2bd..84a1e32 100644
--- a/include/CPUSensor.hpp
+++ b/include/CPUSensor.hpp
@@ -16,7 +16,7 @@
#include <variant>
#include <vector>
-class CPUSensor : public Sensor
+class CPUSensor : public Sensor, public std::enable_shared_from_this<CPUSensor>
{
public:
CPUSensor(const std::string& path, const std::string& objectType,
@@ -31,6 +31,7 @@
static constexpr unsigned int sensorPollMs = 1000;
static constexpr size_t warnAfterErrorCount = 10;
static constexpr const char* labelTcontrol = "Tcontrol";
+ void setupRead(void);
private:
sdbusplus::asio::object_server& objServer;
@@ -45,13 +46,12 @@
size_t pollTime;
bool loggedInterfaceDown = false;
uint8_t minMaxReadCounter;
- void setupRead(void);
void handleResponse(const boost::system::error_code& err);
void checkThresholds(void) override;
void updateMinMaxValues(void);
};
-extern boost::container::flat_map<std::string, std::unique_ptr<CPUSensor>>
+extern boost::container::flat_map<std::string, std::shared_ptr<CPUSensor>>
gCpuSensors;
// this is added to cpusensor.hpp to avoid having every sensor have to link
diff --git a/src/CPUSensor.cpp b/src/CPUSensor.cpp
index e955fef..cd1038f 100644
--- a/src/CPUSensor.cpp
+++ b/src/CPUSensor.cpp
@@ -42,7 +42,8 @@
bool show, double dtsOffset) :
Sensor(escapeName(sensorName), std::move(thresholdsIn), sensorConfiguration,
objectType, false, false, 0, 0, conn, PowerState::on),
- objServer(objectServer), inputDev(io), waitTimer(io), path(path),
+ std::enable_shared_from_this<CPUSensor>(), objServer(objectServer),
+ inputDev(io), waitTimer(io), path(path),
privTcontrol(std::numeric_limits<double>::quiet_NaN()),
dtsOffset(dtsOffset), show(show), pollTime(CPUSensor::sensorPollMs),
minMaxReadCounter(0)
@@ -95,7 +96,6 @@
// call setup always as not all sensors call setInitialProperties
setupPowerMatch(conn);
- setupRead();
}
CPUSensor::~CPUSensor()
@@ -116,6 +116,8 @@
void CPUSensor::setupRead(void)
{
+ std::weak_ptr<CPUSensor> weakRef = weak_from_this();
+
if (readingStateGood())
{
inputDev.close();
@@ -126,8 +128,15 @@
boost::asio::async_read_until(
inputDev, readBuf, '\n',
- [&](const boost::system::error_code& ec,
- std::size_t /*bytes_transfered*/) { handleResponse(ec); });
+ [weakRef](const boost::system::error_code& ec,
+ std::size_t /*bytes_transfered*/) {
+ std::shared_ptr<CPUSensor> self = weakRef.lock();
+ if (!self)
+ {
+ return;
+ }
+ self->handleResponse(ec);
+ });
}
else
{
@@ -141,12 +150,17 @@
markAvailable(false);
}
waitTimer.expires_from_now(boost::posix_time::milliseconds(pollTime));
- waitTimer.async_wait([&](const boost::system::error_code& ec) {
+ waitTimer.async_wait([weakRef](const boost::system::error_code& ec) {
if (ec == boost::asio::error::operation_aborted)
{
return; // we're being canceled
}
- setupRead();
+ std::shared_ptr<CPUSensor> self = weakRef.lock();
+ if (!self)
+ {
+ return;
+ }
+ self->setupRead();
});
}
diff --git a/src/CPUSensorMain.cpp b/src/CPUSensorMain.cpp
index b344a6c..a352735 100644
--- a/src/CPUSensorMain.cpp
+++ b/src/CPUSensorMain.cpp
@@ -51,7 +51,7 @@
static constexpr bool debug = false;
-boost::container::flat_map<std::string, std::unique_ptr<CPUSensor>> gCpuSensors;
+boost::container::flat_map<std::string, std::shared_ptr<CPUSensor>> gCpuSensors;
boost::container::flat_map<std::string,
std::shared_ptr<sdbusplus::asio::dbus_interface>>
inventoryIfaces;
@@ -381,10 +381,11 @@
auto& sensorPtr = gCpuSensors[sensorName];
// make sure destructor fires before creating a new one
sensorPtr = nullptr;
- sensorPtr = std::make_unique<CPUSensor>(
+ sensorPtr = std::make_shared<CPUSensor>(
inputPathStr, sensorType, objectServer, dbusConnection, io,
sensorName, std::move(sensorThresholds), *interfacePath, cpuId,
show, dtsOffset);
+ sensorPtr->setupRead();
createdSensors.insert(sensorName);
if (debug)
{