BUG#37644518: Race condition on rpl_opt_tracker

A race condition was found on `Rpl_opt_tracker` that can make
instance pointers invalid.
The scenario is:
1) `Rpl_opt_tracker::worker()` calls
   `acquire_option_tracker_service()`;
2) Concurrently `Rpl_opt_tracker::track_replication_replica()` is
   called, which calls `acquire_option_tracker_service()` again and
   then calls `release_option_tracker_service()`.
This results in one service reference lost,
`m_option_tracker_handle` is overridden, and the invalidation of
`m_option_tracker_service`, which will cause a server error on the
next steps of `Rpl_opt_tracker::worker()`.

To solve the above issue, the helper methods were refactored to use
a guard that handles the service acquire/release on the methods that
interact with the service.

Change-Id: I433d4ef0009b33ac2319943a4defd9ec22d89a23

Author: nacarvalho

sql/rpl_opt_tracker.cc

@@ -95,25 +95,6 @@ Rpl_opt_tracker::~Rpl_opt_tracker() {
       });
 }
 
-bool Rpl_opt_tracker::acquire_option_tracker_service() {
-  if (srv_registry->acquire("mysql_option_tracker_option",
-                            &m_option_tracker_handle)) {
-    return true;
-  }
-  m_option_tracker_service =
-      reinterpret_cast<SERVICE_TYPE(mysql_option_tracker_option) *>(
-          m_option_tracker_handle);
-  return false;
-}
-
-void Rpl_opt_tracker::release_option_tracker_service() {
-  m_option_tracker_service = nullptr;
-  if (nullptr != m_option_tracker_handle) {
-    srv_registry->release(m_option_tracker_handle);
-    m_option_tracker_handle = nullptr;
-  }
-}
-
 bool Rpl_opt_tracker::is_replication_replica_enabled() {
   bool replication_replica_enabled = false;
 
@@ -129,31 +110,32 @@ bool Rpl_opt_tracker::is_replication_replica_enabled() {
   return replication_replica_enabled;
 }
 
-void Rpl_opt_tracker::track_binary_log_internal(bool enabled) {
-  m_option_tracker_service->set_enabled(s_f_name_binary_log.c_str(),
-                                        enabled ? 1 : 0);
-
-  if (enabled) {
-    ++m_opt_option_tracker_usage_binary_log;
+void Rpl_opt_tracker::track(Tracker_service_guard &service_guard, bool enabled,
+                            const std::string &fname,
+                            unsigned long long &usage_counter) {
+  if (service_guard.is_valid()) {
+    service_guard->set_enabled(fname.c_str(), enabled ? 1 : 0);
+    if (enabled) {
+      ++usage_counter;
+    }
   }
 }
 
-void Rpl_opt_tracker::track_replication_replica(bool enabled) {
-  if (acquire_option_tracker_service()) {
-    return;
-  }
-
-  track_replication_replica_internal(enabled);
-  release_option_tracker_service();
+void Rpl_opt_tracker::track_binary_log(Tracker_service_guard &service_guard,
+                                       bool enabled) const {
+  track(service_guard, enabled, s_f_name_binary_log,
+        m_opt_option_tracker_usage_binary_log);
 }
 
-void Rpl_opt_tracker::track_replication_replica_internal(bool enabled) {
-  m_option_tracker_service->set_enabled(s_f_name_replication_replica.c_str(),
-                                        enabled ? 1 : 0);
+void Rpl_opt_tracker::track_replication_replica(
+    Tracker_service_guard &service_guard, bool enabled) const {
+  track(service_guard, enabled, s_f_name_replication_replica,
+        m_opt_option_tracker_usage_replication_replica);
+}
 
-  if (enabled) {
-    ++m_opt_option_tracker_usage_replication_replica;
-  }
+void Rpl_opt_tracker::track_replication_replica(bool enabled) const {
+  Tracker_service_guard service_guard(m_service_name, srv_registry);
+  track_replication_replica(service_guard, enabled);
 }
 
 void Rpl_opt_tracker::worker() {
@@ -172,19 +154,19 @@ void Rpl_opt_tracker::worker() {
       Only track features if the option tracker service is
       installed.
     */
-    if (!acquire_option_tracker_service()) {
+    {
+      Tracker_service_guard service_guard(m_service_name, srv_registry);
+
       /*
         Binary Log
       */
-      rpl_opt_tracker->track_binary_log_internal(opt_bin_log);
+      track_binary_log(service_guard, opt_bin_log);
 
       /*
         Replication Replica
       */
-      rpl_opt_tracker->track_replication_replica_internal(
-          is_replication_replica_enabled());
-
-      release_option_tracker_service();
+      track_replication_replica(service_guard,
+                                is_replication_replica_enabled());
     }
 
     mysql_mutex_lock(&LOCK_rpl_opt_tracker);

sql/rpl_opt_tracker.h

@@ -26,6 +26,7 @@
 
 #include <my_systime.h>
 #include <mysql/components/minimal_chassis.h>
+#include <mysql/components/my_service.h>
 #include <mysql/components/services/bits/psi_thread_bits.h>
 #include <mysql/components/services/mysql_option_tracker.h>
 #include <string>
@@ -70,19 +71,12 @@ class Rpl_opt_tracker {
     @param enabled  true:  tracks as enabled
                     false: tracks as disabled
   */
-  void track_replication_replica(bool enabled);
+  void track_replication_replica(bool enabled) const;
 
  private:
-  /**
-    Tracks the Binary Log feature, including the usage data.
-    It only updates usage data if the feature is enabled.
-    Internal method to be used after the mysql_option_tracker_option
-    service is acquired.
-
-    @param enabled  true:  tracks as enabled
-                    false: tracks as disabled
-  */
-  void track_binary_log_internal(bool enabled);
+  using Service_type = SERVICE_TYPE(mysql_option_tracker_option);
+  using Tracker_service_guard = my_service<Service_type>;
+  static constexpr auto m_service_name = "mysql_option_tracker_option";
 
   /**
     Helper method to get Replication Replica feature status.
@@ -94,36 +88,47 @@ class Rpl_opt_tracker {
   static bool is_replication_replica_enabled();
 
   /**
-    Tracks the Replication Replica feature, including the usage data.
+    Tracks a feature, including the usage data.
     It only updates usage data if the feature is enabled.
     Internal method to be used after the mysql_option_tracker_option
     service is acquired.
 
+    @param service_guard acquired service guard
     @param enabled  true:  tracks as enabled
                     false: tracks as disabled
+    @param fname feature name
+    @param usage_counter feature usage counter
   */
-  void track_replication_replica_internal(bool enabled);
+  static void track(Tracker_service_guard &service_guard, bool enabled,
+                    const std::string &fname,
+                    unsigned long long &usage_counter);
 
   /**
-    Helper method to acquire the mysql_option_tracker_option
-    service.
+    Tracks the Binary Log feature, including the usage data.
+    It only updates usage data if the feature is enabled.
+    Internal method to be used after the mysql_option_tracker_option
+    service is acquired.
 
-    @return the operation status
-      @retval false  Successful
-      @retval true   Error
+    @param service_guard acquired service guard
+    @param enabled  true:  tracks as enabled
+                    false: tracks as disabled
   */
-  bool acquire_option_tracker_service();
+  void track_binary_log(Tracker_service_guard &service_guard,
+                        bool enabled) const;
 
   /**
-    Helper method to release the mysql_option_tracker_option
-    service.
+    Tracks the Replication Replica feature, including the usage data.
+    It only updates usage data if the feature is enabled.
+
+    @param service_guard acquired service guard
+    @param enabled  true:  tracks as enabled
+                    false: tracks as disabled
   */
-  void release_option_tracker_service();
+  void track_replication_replica(Tracker_service_guard &service_guard,
+                                 bool enabled) const;
 
   SERVICE_TYPE_NO_CONST(registry_registration) *
       m_srv_registry_registration_no_lock{nullptr};
-  SERVICE_TYPE(mysql_option_tracker_option) * m_option_tracker_service{nullptr};
-  my_h_service m_option_tracker_handle{nullptr};
 
   my_thread_handle m_thread_id;
   bool m_stop_worker{false};