PS-9723 fix: Crash in xpl::Ssl_context::~Ssl_context() with heavy load of ALTER INSTANCE RELOAD TLS queries

https://perconadev.atlassian.net/browse/PS-9723

This is a fix for regressions introduced by PS-7125
"X Plugin: Reconfigure Certificate at runtime."
https://perconadev.atlassian.net/browse/PS-7125
(commit 351354c).

Fixed a race condition caused by multiple
'ALTER INSTANCE RELOAD TLS' queries running in parallel.

Although 'std::shared_ptr' can be considered thread-safe for operations
modyfing counters in its control block (like creating a copy of
'std::shared_ptr' via copy constructor), operations modyfying
std::shared_ptr itself (like rewriting its state with copy assignment
operator) are not thread safe and additional synchronization must be
added to avoid race conditions.

As 'm_ssl_context' shared pointer inside 'ngs::Server' class
instance can be read and modified from multiple threads, we
now make sure that it is read properly via 'std::atomic_load()'
and modified via 'std::atomic_store()'.

Just for convenience, in order to avoid unnecessary casts,
'Ssl_context_builder::get_result_context()' member function now
returns 'std::shared_ptr' instead of 'std::unique_ptr'.

Also, fixed a problem in 'xpl::global_status_variable()' when the
reference to the 'options' object that could point to an object
destrojed in another thread.

Added 'x.percona_bug_ps9723' MTR test case that simulates a
scenario when 'ALTER INSTANCE RELOAD TLS' query is executed
in a loop from multiple connections simultaneously.

Author: percona-ysorokin

mysql-test/suite/x/r/percona_bug_ps9723.result

@@ -0,0 +1,49 @@
+#
+# PS-9723: Crash in xpl::Ssl_context::~Ssl_context() with heavy load of ALTER INSTANCE RELOAD TLS queries
+# (https://perconadev.atlassian.net/browse/PS-9723)
+#
+CREATE PROCEDURE reload_tls_loop(IN iterations INT UNSIGNED)
+BEGIN
+DECLARE idx INT UNSIGNED DEFAULT iterations;
+WHILE idx > 0 DO
+ALTER INSTANCE RELOAD TLS NO ROLLBACK ON ERROR;
+SET idx = idx - 1;
+END WHILE;
+END//
+# Establising 32 connections
+# Calling reload_tls_loop(1024) from each connection
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+CALL reload_tls_loop(1024);
+# Waiting for reload_tls_loop(1024) to finish in each connection
+# Closing the connections
+DROP PROCEDURE reload_tls_loop;

mysql-test/suite/x/t/percona_bug_ps9723.test

@@ -0,0 +1,65 @@
+--echo #
+--echo # PS-9723: Crash in xpl::Ssl_context::~Ssl_context() with heavy load of ALTER INSTANCE RELOAD TLS queries
+--echo # (https://perconadev.atlassian.net/browse/PS-9723)
+--echo #
+
+--source include/have_mysqlx_plugin.inc
+--source include/big_test.inc
+
+--source include/count_sessions.inc
+
+DELIMITER //;
+
+CREATE PROCEDURE reload_tls_loop(IN iterations INT UNSIGNED)
+BEGIN
+  DECLARE idx INT UNSIGNED DEFAULT iterations;
+
+  WHILE idx > 0 DO
+    ALTER INSTANCE RELOAD TLS NO ROLLBACK ON ERROR;
+    SET idx = idx - 1;
+  END WHILE;
+END//
+
+DELIMITER ;//
+
+--let $number_of_connections = 32
+--let $number_of_iterations = 1024
+
+--echo # Establising $number_of_connections connections
+--let $i = 0
+while ($i < $number_of_connections)
+{
+  --connect(con$i,127.0.0.1,root,,,,,SSL)
+  --inc $i
+}
+
+--echo # Calling reload_tls_loop($number_of_iterations) from each connection
+--let $i = 0
+while ($i < $number_of_connections)
+{
+  --connection con$i
+  --send_eval CALL reload_tls_loop($number_of_iterations)
+  --inc $i
+}
+
+--echo # Waiting for reload_tls_loop($number_of_iterations) to finish in each connection
+--let $i = 0
+while ($i < $number_of_connections)
+{
+  --connection con$i
+  --reap
+  --inc $i
+}
+
+--echo # Closing the connections
+--let $i = 0
+while ($i < $number_of_connections)
+{
+  --disconnect con$i
+  --inc $i
+}
+--connection default
+
+--source include/wait_until_count_sessions.inc
+
+DROP PROCEDURE reload_tls_loop;

plugin/x/src/server/builder/ssl_context_builder.cc

@@ -114,9 +114,9 @@ void Ssl_context_builder::setup_ssl_context(
   }
 }
 
-std::unique_ptr<iface::Ssl_context> Ssl_context_builder::get_result_context()
+std::shared_ptr<iface::Ssl_context> Ssl_context_builder::get_result_context()
     const {
-  std::unique_ptr<iface::Ssl_context> result = std::make_unique<Ssl_context>();
+  std::shared_ptr<iface::Ssl_context> result = std::make_shared<Ssl_context>();
 
   setup_ssl_context(result.get());
 

plugin/x/src/server/builder/ssl_context_builder.h

@@ -38,7 +38,7 @@ class Ssl_context_builder {
  public:
   Ssl_context_builder() = default;
 
-  std::unique_ptr<iface::Ssl_context> get_result_context() const;
+  std::shared_ptr<iface::Ssl_context> get_result_context() const;
 
  private:
   struct Ssl_config_local {

plugin/x/src/server/server.cc

@@ -136,13 +136,15 @@ void Server::delayed_start_tasks() {
 }
 
 void Server::reload_ssl_context() {
-  m_ssl_context = xpl::Ssl_context_builder().get_result_context();
+  std::atomic_store(&m_ssl_context,
+                    xpl::Ssl_context_builder().get_result_context());
 }
 
 void Server::start_tasks() {
   // We can't fetch the servers ssl config at plugin-load
   // this method allows to setup it at better time.
-  m_ssl_context = xpl::Ssl_context_builder().get_result_context();
+  std::atomic_store(&m_ssl_context,
+                    xpl::Ssl_context_builder().get_result_context());
 
   if (m_state.exchange(State::State_initializing, State_running)) {
     for (auto task : m_tasks) {
@@ -436,7 +438,8 @@ bool Server::reset() {
 
   m_state.wait_for(allowed_values);
 
-  m_ssl_context->reset();
+  auto context = std::atomic_load(&m_ssl_context);
+  context->reset();
   m_id_generator.reset(new Document_id_generator());
   m_factory.reset(new xpl::Server_factory());
 

plugin/x/src/server/server.h

@@ -28,6 +28,7 @@
 
 #include <stdint.h>
 
+#include <atomic>
 #include <cstdint>
 #include <functional>
 #include <list>
@@ -84,7 +85,7 @@ class Server : public xpl::iface::Server {
          std::shared_ptr<xpl::iface::Timeout_callback> timeout_callback);
 
   std::shared_ptr<xpl::iface::Ssl_context> ssl_context() const override {
-    return m_ssl_context;
+    return std::atomic_load(&m_ssl_context);
   }
 
   bool reset() override;

plugin/x/src/variables/status_variables.cc

@@ -197,11 +197,14 @@ int global_status_variable(THD *, SHOW_VAR *var, char *buff) {
 
   if (!server.container()) return 0;
 
-  if (!server->ssl_context()) return 0;
+  // creating a copy of the context's shared_pointer in order to
+  // extend its lifetime
+  auto context = server->ssl_context();
+  if (!context) return 0;
 
-  auto &context = server->ssl_context()->options();
+  auto &options = context->options();
   auto context_method = method;  // workaround on VC compiler internal error
-  ReturnType result = (context.*context_method)();
+  ReturnType result = (options.*context_method)();
 
   mysqld::xpl_show_var(var).assign(result);
   return 0;