Fix race condition between subscription deletions
After the connection max-retry with the policy of
"TerminateAfterRetries", coredump may have been occurred although it
seems rare.
Stack frame of One occurrency indicates that an invalid memory access
happened inside deleteSubscription().
1) http_client is waiting for async_read() to handle
sendEventToSubscriber().
```
- recvMessage via async_read() [hold Connection with shared_from_this()] --> call afterRead().
- afterRead() find the invalid response (i.e. probably disconnected from the other end?), and call waitAndRetry()
- waitAndRetry() detects the maxRetryAttempts (with the policy of "TerminateAfterRetries"),
and invokes callback callback is ConnectionPool::afterSendData().
```
2) Meanwhile, the subscription is explicitly requested to delete via
Redfish API.
```
BMCWEB_ROUTE(app, "/redfish/v1/EventService/Subscriptions/<str>/")
...
.methods(boost::beast::http::verb::delete_)(
...
if (!event.deleteSubscription(param))
```
3) Later on, http_client invokes resHandler() but this resHandler() is
bound with its own subscription object like this.
```
bool Subscription::sendEventToSubscriber(std::string&& msg)
{
client->sendDataWithCallback(
std::move(msg), userSub->destinationUrl,
static_cast<ensuressl::VerifyCertificate>(
userSub->verifyCertificate),
httpHeadersCopy, boost::beast::http::verb::post,
std::bind_front(&Subscription::resHandler, this)); <==
```
As the result, if the subscription object is already deleted outside
(i.e. Redfish API delete), resHandler() which is from async_read
callback may be accessing the invalid object.
```
void Subscription::resHandler(const crow::Response& res)
{
...
if (client->isTerminated())
{
hbTimer.cancel();
if (deleter)
{
deleter(); --> This invokes deleteSubscription()
}
}
}
```
Quick summary of stack frame:
```
0 __GI_memcmp (s1=<optimized out>, s2=<optimized out>, len=<optimized out>)
at memcmp.c:342
warning: 342 memcmp.c: No such file or directory
at memcmp.c:342
at /usr/include/c++/13.2.0/bits/basic_string.h:3177
...
this=0x814fa8 <redfish::EventServiceManager::getInstance(boost::asio::io_context*)::handler>, id=...)
at /usr/src/debug/bmcweb/1.0+git/redfish-core/include/event_service_manager.hpp:537
resHandler=..., keepAlive=false, connId=0, res=...)
at /usr/src/debug/bmcweb/1.0+git/http/http_client.hpp:803
...
at /usr/src/debug/bmcweb/1.0+git/http/http_client.hpp:461
at /usr/src/debug/bmcweb/1.0+git/http/http_client.hpp:440
bytesTransferred=<optimized out>)
at /usr/src/debug/bmcweb/1.0+git/http/http_client.hpp:398
```
So, we would need to hold the subscription object until resHandler() is
completed by holding up using `shared_from_this()` for
sendEventToSubscriber()..
```
bool Subscription::sendEventToSubscriber(std::string&& msg)
{
client->sendDataWithCallback(
...
std::bind_front(&Subscription::resHandler, this, shared_from_this())); <===
```
Tested:
- Compiles
- Event Listener works
- Attempt to delete subscriptions
Change-Id: I5172c96e9d1bd2f03831916a95167e0ea532e9f2
Signed-off-by: Myung Bae <myungbae@us.ibm.com>
diff --git a/redfish-core/src/subscription.cpp b/redfish-core/src/subscription.cpp
index c0416e3..f81fdc6 100644
--- a/redfish-core/src/subscription.cpp
+++ b/redfish-core/src/subscription.cpp
@@ -64,7 +64,8 @@
{}
// callback for subscription sendData
-void Subscription::resHandler(const crow::Response& res)
+void Subscription::resHandler(const std::shared_ptr<Subscription>& /*self*/,
+ const crow::Response& res)
{
BMCWEB_LOG_DEBUG("Response handled with return code: {}", res.resultInt());
@@ -187,7 +188,8 @@
static_cast<ensuressl::VerifyCertificate>(
userSub->verifyCertificate),
httpHeadersCopy, boost::beast::http::verb::post,
- std::bind_front(&Subscription::resHandler, this));
+ std::bind_front(&Subscription::resHandler, this,
+ shared_from_this()));
return true;
}