commit 99ff0ddcc083c149d3a64190e6c1b0caab1d77f2
author Myung Bae <myungbae@us.ibm.com> Tue Apr 22 14:38:36 2025 -0500
committer Ed Tanous <ed@tanous.net> Thu Apr 24 23:20:50 2025 +0000
tree e05551bf582e26e73c50ed58857f39739638849d
parent 5ffbc9ae424658c15c2b6d93b86b752f09d2b6dc

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>