Check if the object is still valid in the timeout callback function
When the SOL module in netipmid is busy, there is a chance that the
timeout callback function is executed after the context object is
destructed. This will cause the process to crash with this error:
terminate called after throwing an instance of
'boost::wrapexcept<boost::asio::bad_executor>'
what(): bad executor
The root cause is that the cancel() cannot cancel the expired callback
handlers. When the callback handler is executed, the object is deleted
already.
This uses proper reference counting on the objects captured in the
lambda so that they are not referencing memory that has already gone out
of scope.
Tested:
Decrease the accumulateInterval to 50ms for easy reproducing.
Run "ipmitool sel list", "ipmitool sensor list"
and "ipmitool sol looptest 200 500" at the same time,
no "sol looptest fail" error
Signed-off-by: Vernon Mauery <vernon.mauery@linux.intel.com>
Change-Id: I4b9e4ebce14ff5fca8e991aed96643186c9ea5d9
diff --git a/sol/sol_context.cpp b/sol/sol_context.cpp
index 215f725..3b2b892 100644
--- a/sol/sol_context.cpp
+++ b/sol/sol_context.cpp
@@ -19,7 +19,17 @@
sessionID(sessionID)
{
session = std::get<session::Manager&>(singletonPool).getSession(sessionID);
- enableAccumulateTimer(true);
+}
+
+std::shared_ptr<Context>
+ Context::makeContext(std::shared_ptr<boost::asio::io_context> io,
+ uint8_t maxRetryCount, uint8_t sendThreshold,
+ uint8_t instance, session::SessionID sessionID)
+{
+ auto ctx = std::make_shared<Context>(io, maxRetryCount, sendThreshold,
+ instance, sessionID);
+ ctx->enableAccumulateTimer(true);
+ return ctx;
}
void Context::enableAccumulateTimer(bool enable)
@@ -30,12 +40,15 @@
if (enable)
{
accumulateTimer.expires_after(interval);
- accumulateTimer.async_wait([this](const boost::system::error_code& ec) {
- if (!ec)
- {
- charAccTimerHandler();
- }
- });
+ std::weak_ptr<Context> weakRef = weak_from_this();
+ accumulateTimer.async_wait(
+ [weakRef](const boost::system::error_code& ec) {
+ std::shared_ptr<Context> self = weakRef.lock();
+ if (!ec && self)
+ {
+ self->charAccTimerHandler();
+ }
+ });
}
else
{
@@ -51,10 +64,12 @@
std::chrono::microseconds interval =
std::get<sol::Manager&>(singletonPool).retryInterval;
retryTimer.expires_after(interval);
- retryTimer.async_wait([this](const boost::system::error_code& ec) {
- if (!ec)
+ std::weak_ptr<Context> weakRef = weak_from_this();
+ retryTimer.async_wait([weakRef](const boost::system::error_code& ec) {
+ std::shared_ptr<Context> self = weakRef.lock();
+ if (!ec && self)
{
- retryTimerHandler();
+ self->retryTimerHandler();
}
});
}
diff --git a/sol/sol_context.hpp b/sol/sol_context.hpp
index 8d0fc87..50be65f 100644
--- a/sol/sol_context.hpp
+++ b/sol/sol_context.hpp
@@ -149,7 +149,7 @@
* interfaces to handle incoming SOL payload, send response and send outbound
* SOL payload.
*/
-class Context
+class Context : std::enable_shared_from_this<Context>
{
public:
Context() = delete;
@@ -159,10 +159,28 @@
Context(Context&&) = delete;
Context& operator=(Context&&) = delete;
+ /** @brief Context Factory
+ *
+ * This is called by the SOL Manager when a SOL payload instance is
+ * started for the activate payload command. Its purpose is to be able
+ * to perform post-creation tasks on the object without changing the
+ * code flow
+ *
+ * @param[in] io - boost::asio io context for event scheduling.
+ * @param[in] maxRetryCount - Retry count max value.
+ * @param[in] sendThreshold - Character send threshold.
+ * @param[in] instance - SOL payload instance.
+ * @param[in] sessionID - BMC session ID.
+ */
+ static std::shared_ptr<Context>
+ makeContext(std::shared_ptr<boost::asio::io_context> io,
+ uint8_t maxRetryCount, uint8_t sendThreshold,
+ uint8_t instance, session::SessionID sessionID);
+
/** @brief Context Constructor.
*
- * This is issued by the SOL Manager when a SOL payload instance is
- * started for the activate payload command.
+ * This should only be used by the Context factory makeContext
+ * or the accumulate timer will not be initialized properly
*
* @param[in] io - boost::asio io context for event scheduling.
* @param[in] maxRetryCount - Retry count max value.
@@ -248,6 +266,12 @@
*/
void resendPayload(bool clear);
+ /** @brief accumlate timer handler called by timer */
+ void charAccTimerHandler();
+
+ /** @brief retry timer handler called by timer */
+ void retryTimerHandler();
+
private:
/** @brief Expected character count.
*
@@ -281,12 +305,6 @@
* @param[in] out - buffer containing the SOL payload.
*/
void sendPayload(const std::vector<uint8_t>& out) const;
-
- /** @brief accumlate timer handler called by timer */
- void charAccTimerHandler();
-
- /** @brief retry timer handler called by timer */
- void retryTimerHandler();
};
} // namespace sol
diff --git a/sol/sol_manager.cpp b/sol/sol_manager.cpp
index 3db7313..87f9ed7 100644
--- a/sol/sol_manager.cpp
+++ b/sol/sol_manager.cpp
@@ -113,8 +113,8 @@
}
// Create the SOL Context data for payload instance
- auto context = std::make_unique<Context>(io, retryCount, sendThreshold,
- payloadInstance, sessionID);
+ std::shared_ptr<Context> context = Context::makeContext(
+ io, retryCount, sendThreshold, payloadInstance, sessionID);
payloadMap.emplace(payloadInstance, std::move(context));
}
diff --git a/sol/sol_manager.hpp b/sol/sol_manager.hpp
index 86d24d5..caeaa39 100644
--- a/sol/sol_manager.hpp
+++ b/sol/sol_manager.hpp
@@ -38,7 +38,7 @@
/** @brief SOL Payload Instance is the key for the map, the value is the
* SOL context.
*/
- using SOLPayloadMap = std::map<Instance, std::unique_ptr<Context>>;
+ using SOLPayloadMap = std::map<Instance, std::shared_ptr<Context>>;
Manager() = delete;
~Manager() = default;