Skip to content
Commit 99ff0ddc authored by Myung Bae's avatar Myung Bae Committed by Ed Tanous
Browse files

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: default avatarMyung Bae <myungbae@us.ibm.com>
parent 5ffbc9ae
Loading
Loading
Loading
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment