[C++] Fix use-after-free undefined behavior due to object lifetime pr…

…oblem (apache#10220)

* Fixing use-after-free bug due to object lifetime problem

The HandlerBase class has a shared_ptr to a boost ASIO deadline timer object.  This deadline timer instance has a bare reference (internally) to a corresponding io_service object it takes as a parameter, but there's no shared ownership relationship between these objects.  Boost expects io_service  objects to outlive any users of them - it's more designed to be a stack allocated object setting at the top of a worker thread, passed down into any code that needs to use it, opposed to this heap-allocated shared ownership scheme.  This can still be problematic however, if a thread_local/global variable indirectly references it and outlives it (which is how I tripped over this), because deadline timers will reference the io_service in their destructor.

As can been seen by looking through the Pulsar code, all existing use cases carefully ensure that a shared reference to an ExecutorService, which owns the io_service instances, is kept above any deadline timer instances in the class order, so that they always outlive the timers.  This is very easy to mess up, as we see here.  A band-aid solution is to add an additional shared pointer reference to the executor service in the HandlerBase object above the timer object, to explicitly avoid the lifetime issue.  A good long term goal would be to change the ownership model of these ASIO objects entirely to better fit the expectations of the boost library, but I won't attempt that here.

This change fixes the use-after-free crash if a HandlerBase derived object ends up living in a thread local, function-scoped-static, or global context (for example, if client code has a global cache of Producers).

* Realized that ProducerImpl was a subclass of HandlerBase and thus it would be better to simply consolidate this functionality in the base class and expose it as a protected member.  This seems to work nicely and reduces some of the duplication of effort.

Co-authored-by: Matteo Merli <[email protected]>

pulsar-client-cpp/lib/HandlerBase.cc

@@ -31,13 +31,14 @@ HandlerBase::HandlerBase(const ClientImplPtr& client, const std::string& topic,
     : client_(client),
       topic_(topic),
       connection_(),
+      executor_(client->getIOExecutorProvider()->get()),
       mutex_(),
       creationTimestamp_(TimeUtils::now()),
       operationTimeut_(seconds(client->conf().getOperationTimeoutSeconds())),
       state_(Pending),
       backoff_(backoff),
       epoch_(0),
-      timer_(client->getIOExecutorProvider()->get()->createDeadlineTimer()) {}
+      timer_(executor_->createDeadlineTimer()) {}
 
 HandlerBase::~HandlerBase() { timer_->cancel(); }
 

pulsar-client-cpp/lib/HandlerBase.h

@@ -87,6 +87,7 @@ class HandlerBase {
     ClientImplWeakPtr client_;
     const std::string topic_;
     ClientConnectionWeakPtr connection_;
+    ExecutorServicePtr executor_;
     std::mutex mutex_;
     std::mutex pendingReceiveMutex_;
     ptime creationTimestamp_;

pulsar-client-cpp/lib/ProducerImpl.cc

@@ -48,7 +48,6 @@ ProducerImpl::ProducerImpl(ClientImplPtr client, const std::string& topic, const
           client, topic,
           Backoff(milliseconds(100), seconds(60), milliseconds(std::max(100, conf.getSendTimeout() - 100)))),
       conf_(conf),
-      executor_(client->getIOExecutorProvider()->get()),
       semaphore_(),
       pendingMessagesQueue_(),
       partition_(partition),

pulsar-client-cpp/lib/ProducerImpl.h

@@ -141,8 +141,6 @@ class ProducerImpl : public HandlerBase,
 
     ProducerConfiguration conf_;
 
-    ExecutorServicePtr executor_;
-
     std::unique_ptr<Semaphore> semaphore_;
     MessageQueue pendingMessagesQueue_;