Capture weak reference of sensor object in lambda
dbus callback methods are capturing 'this' pointer directly. A sensor
object can get deleted while a callback is pending invocation. This is
crashing ipmbsensor daemon.
Capture weak reference in place of this pointer.
Tested: Run ipmbsensor as a binary with entity manager constantly
restarting to simulated destruction and construction of new sensor
object. No crash was observed after running for three hours. Prior to
this ipmbsensor was crashing within one hour
Signed-off-by: Vikash Chandola <vikash.chandola@intel.com>
Change-Id: Idd53cbeb912c87ca3c4c09302279fc9383c569bc
diff --git a/src/IpmbSensor.cpp b/src/IpmbSensor.cpp
index 2af79e4..3fc0ee5 100644
--- a/src/IpmbSensor.cpp
+++ b/src/IpmbSensor.cpp
@@ -48,10 +48,7 @@
static constexpr const char* sensorPathPrefix = "/xyz/openbmc_project/sensors/";
-using IpmbMethodType =
- std::tuple<int, uint8_t, uint8_t, uint8_t, uint8_t, std::vector<uint8_t>>;
-
-boost::container::flat_map<std::string, std::unique_ptr<IpmbSensor>> sensors;
+boost::container::flat_map<std::string, std::shared_ptr<IpmbSensor>> sensors;
std::unique_ptr<boost::asio::steady_timer> initCmdTimer;
@@ -125,25 +122,37 @@
read();
}
+static void initCmdCb(const std::weak_ptr<IpmbSensor>& weakRef,
+ const boost::system::error_code& ec,
+ const IpmbMethodType& response)
+{
+ std::shared_ptr<IpmbSensor> self = weakRef.lock();
+ if (!self)
+ {
+ return;
+ }
+ const int& status = std::get<0>(response);
+ if (ec || (status != 0))
+ {
+ std::cerr << "Error setting init command for device: " << self->name
+ << "\n";
+ }
+}
+
void IpmbSensor::runInitCmd()
{
- if (initCommand)
+ if (!initCommand)
{
- dbusConnection->async_method_call(
- [this](boost::system::error_code ec,
- const IpmbMethodType& response) {
- const int& status = std::get<0>(response);
-
- if (ec || (status != 0))
- {
- std::cerr << "Error setting init command for device: " << name
- << "\n";
- }
- },
- "xyz.openbmc_project.Ipmi.Channel.Ipmb",
- "/xyz/openbmc_project/Ipmi/Channel/Ipmb", "org.openbmc.Ipmb",
- "sendRequest", commandAddress, netfn, lun, *initCommand, initData);
+ return;
}
+ dbusConnection->async_method_call(
+ [weakRef{weak_from_this()}](const boost::system::error_code& ec,
+ const IpmbMethodType& response) {
+ initCmdCb(weakRef, ec, response);
+ },
+ "xyz.openbmc_project.Ipmi.Channel.Ipmb",
+ "/xyz/openbmc_project/Ipmi/Channel/Ipmb", "org.openbmc.Ipmb",
+ "sendRequest", commandAddress, netfn, lun, *initCommand, initData);
}
void IpmbSensor::loadDefaults()
@@ -334,78 +343,100 @@
}
}
+void IpmbSensor::ipmbRequestCompletionCb(const boost::system::error_code& ec,
+ const IpmbMethodType& response)
+{
+ const int& status = std::get<0>(response);
+ if (ec || (status != 0))
+ {
+ incrementError();
+ read();
+ return;
+ }
+ const std::vector<uint8_t>& data = std::get<5>(response);
+ if constexpr (debug)
+ {
+ std::cout << name << ": ";
+ for (size_t d : data)
+ {
+ std::cout << d << " ";
+ }
+ std::cout << "\n";
+ }
+ if (data.empty())
+ {
+ incrementError();
+ read();
+ return;
+ }
+
+ double value = 0;
+
+ if (!processReading(data, value))
+ {
+ incrementError();
+ read();
+ return;
+ }
+
+ // rawValue only used in debug logging
+ // up to 5th byte in data are used to derive value
+ size_t end = std::min(sizeof(uint64_t), data.size());
+ uint64_t rawData = 0;
+ for (size_t i = 0; i < end; i++)
+ {
+ // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
+ reinterpret_cast<uint8_t*>(&rawData)[i] = data[i];
+ }
+ rawValue = static_cast<double>(rawData);
+
+ /* Adjust value as per scale and offset */
+ value = (value * scaleVal) + offsetVal;
+ updateValue(value);
+ read();
+}
+
void IpmbSensor::read(void)
{
waitTimer.expires_from_now(std::chrono::milliseconds(sensorPollMs));
- waitTimer.async_wait([this](const boost::system::error_code& ec) {
+ waitTimer.async_wait(
+ [weakRef{weak_from_this()}](const boost::system::error_code& ec) {
if (ec == boost::asio::error::operation_aborted)
{
return; // we're being canceled
}
- if (!readingStateGood())
+ std::shared_ptr<IpmbSensor> self = weakRef.lock();
+ if (!self)
{
- updateValue(std::numeric_limits<double>::quiet_NaN());
- read();
return;
}
- dbusConnection->async_method_call(
- [this](boost::system::error_code ec,
- const IpmbMethodType& response) {
- const int& status = std::get<0>(response);
- if (ec || (status != 0))
- {
- incrementError();
- read();
- return;
- }
- const std::vector<uint8_t>& data = std::get<5>(response);
- if constexpr (debug)
- {
- std::cout << name << ": ";
- for (size_t d : data)
- {
- std::cout << d << " ";
- }
- std::cout << "\n";
- }
- if (data.empty())
- {
- incrementError();
- read();
- return;
- }
-
- double value = 0;
-
- if (!processReading(data, value))
- {
- incrementError();
- read();
- return;
- }
-
- // rawValue only used in debug logging
- // up to 5th byte in data are used to derive value
- size_t end = std::min(sizeof(uint64_t), data.size());
- uint64_t rawData = 0;
- for (size_t i = 0; i < end; i++)
- {
- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
- reinterpret_cast<uint8_t*>(&rawData)[i] = data[i];
- }
- rawValue = static_cast<double>(rawData);
-
- /* Adjust value as per scale and offset */
- value = (value * scaleVal) + offsetVal;
- updateValue(value);
- read();
- },
- "xyz.openbmc_project.Ipmi.Channel.Ipmb",
- "/xyz/openbmc_project/Ipmi/Channel/Ipmb", "org.openbmc.Ipmb",
- "sendRequest", commandAddress, netfn, lun, command, commandData);
+ self->sendIpmbRequest();
});
}
+void IpmbSensor::sendIpmbRequest()
+{
+ if (!readingStateGood())
+ {
+ updateValue(std::numeric_limits<double>::quiet_NaN());
+ read();
+ return;
+ }
+ dbusConnection->async_method_call(
+ [weakRef{weak_from_this()}](boost::system::error_code ec,
+ const IpmbMethodType& response) {
+ std::shared_ptr<IpmbSensor> self = weakRef.lock();
+ if (!self)
+ {
+ return;
+ }
+ self->ipmbRequestCompletionCb(ec, response);
+ },
+ "xyz.openbmc_project.Ipmi.Channel.Ipmb",
+ "/xyz/openbmc_project/Ipmi/Channel/Ipmb", "org.openbmc.Ipmb",
+ "sendRequest", commandAddress, netfn, lun, command, commandData);
+}
+
bool IpmbSensor::sensorClassType(const std::string& sensorClass)
{
if (sensorClass == "PxeBridgeTemp")
@@ -479,7 +510,7 @@
void createSensors(
boost::asio::io_service& io, sdbusplus::asio::object_server& objectServer,
- boost::container::flat_map<std::string, std::unique_ptr<IpmbSensor>>&
+ boost::container::flat_map<std::string, std::shared_ptr<IpmbSensor>>&
sensors,
std::shared_ptr<sdbusplus::asio::connection>& dbusConnection)
{
@@ -546,7 +577,8 @@
}
auto& sensor = sensors[name];
- sensor = std::make_unique<IpmbSensor>(
+ sensor = nullptr;
+ sensor = std::make_shared<IpmbSensor>(
dbusConnection, io, name, path, objectServer,
std::move(sensorThresholds), deviceAddress, hostSMbusIndex,
pollRate, sensorTypeName);