Bug#34144617: Signal 11 in vio_shutdown when closing(?) connection

Problem: Killing a connection resulted in signal 11. Root cause is assumed
to be (unable to verify) that an attempt was made at sending SIGALRM to
a non-existent thread id. This is not unusual but normally
pthread_kill() returns an error code which gets printed in the log, and
does not trigger sig 11.

This situation can occur when running with TP, or any other time when a
THD is re-assigned to a different OS thread, because the thread id used
to send SIGALRM is recorded in the Vio object when the THD is created
and not updated when the THD is reassigned to a different OS thread.
And this is incorrect even if when it does not trigger sig 11, because
we are sending SIGALRM to the wrong thread. While a stray SIGALRM is
ignored, it also means that the the thread that is actually waiting in
ppoll will not be woken up and will hang until it times out.

Solution: Make sure THD::real_id is stored in the Vio object before
calling vio_shutdown(). Change thd_close_connection() so that it calls
THD::shutdown_active_vio() instead of Protocol_classic::shutdown() which
is meant to be used from a different thread and is what is being used
when killing connections without TP.

Change-Id: Id9b7e54c5b4388977a39fd268e0ff173d701db2b

include/mysys_err.h

@@ -134,7 +134,8 @@ extern const char *globerrs[]; /* my_error_messages is here */
 #define EE_UNKNOWN_LDML_TAG 90
 #define EE_FAILED_TO_RESET_BEFORE_SECONDARY_IGNORABLE_CHAR 91
 #define EE_FAILED_PROCESSING_DIRECTIVE 92
-#define EE_ERROR_LAST 92 /* Copy last error nr */
+#define EE_PTHREAD_KILL_FAILED 93
+#define EE_ERROR_LAST 93 /* Copy last error nr */
 /* Add error numbers before EE_ERROR_LAST and change it accordingly. */
 
 /* Exit codes for option processing. When exiting from server use the

include/violite.h

@@ -58,6 +58,7 @@ struct Vio;
 #if defined(__cplusplus) && defined(USE_PPOLL_IN_VIO)
 #include <signal.h>
 #include <atomic>
+#include <optional>
 #elif defined(__cplusplus) && defined(HAVE_KQUEUE)
 #include <sys/event.h>
 #include <atomic>
@@ -332,8 +333,18 @@ struct Vio {
   char *read_end = {nullptr};     /* end of unfetched data */
 
 #ifdef USE_PPOLL_IN_VIO
-  my_thread_t thread_id = {0};  // Thread PID
-  sigset_t signal_mask;         // Signal mask
+  /** Thread PID which is to be sent SIGALRM to terminate ppoll
+    wait when shutting down vio. It is made an std::optional so
+    that server code has the ability to set this to an illegal value
+    and thereby ensure that it is set before shutting down vio. In the server
+    the THD and thereby the Vio can switch between OS threads, so it does not
+    make sense to assign the thread id when creating the THD/Vio.
+
+    It is initialized to 0 here, meaning don't attempt to send a signal, to
+    keep non-server code unaffected.
+  */
+  std::optional<my_thread_t> thread_id = 0;
+  sigset_t signal_mask;  // Signal mask
   /*
     Flag to indicate whether we are in poll or shutdown.
     A true value of flag indicates either the socket

mysys/errors.cc

@@ -139,7 +139,8 @@ const char *globerrs[GLOBERRS] = {
     "Reset character out of range: %s.",
     "Unknown LDML tag: '%.*s'.",
     "Failed to reset before a secondary ignorable character %s.",
-    "Stopped processing the '%s' directive in file %s at line %d."};
+    "Stopped processing the '%s' directive in file %s at line %d.",
+    "pthread_kill(thread_id:%lu, signal:%s) returned '%s'."};
 
 /*
  We cannot call my_error/my_printf_error here in this function.

sql/conn_handler/socket_connection.cc

@@ -151,7 +151,9 @@ class Channel_info_local_socket : public Channel_info {
         mysql_socket_vio_new(m_connect_sock, VIO_TYPE_SOCKET, VIO_LOCALHOST);
 #ifdef USE_PPOLL_IN_VIO
     if (vio != nullptr) {
-      vio->thread_id = my_thread_self();
+      // Unset thread_id, to ensure that all shutdowns explicitly set the
+      // current real_id from the THD.
+      vio->thread_id.reset();
       vio->signal_mask = mysqld_signal_mask;
     }
 #endif
@@ -214,7 +216,7 @@ class Channel_info_tcpip_socket : public Channel_info {
     Vio *vio = mysql_socket_vio_new(m_connect_sock, VIO_TYPE_TCPIP, 0);
 #ifdef USE_PPOLL_IN_VIO
     if (vio != nullptr) {
-      vio->thread_id = my_thread_self();
+      vio->thread_id.reset();
       vio->signal_mask = mysqld_signal_mask;
     }
 #endif

sql/sql_class.cc

@@ -1698,14 +1698,16 @@ void THD::store_globals() {
   */
   set_my_thread_var_id(m_thread_id);
 #endif
-  real_id = my_thread_self();  // For debugging
+  real_id = my_thread_self();
 }
 
 /*
   Remove the thread specific info (THD and mem_root pointer) stored during
   store_global call for this thread.
 */
 void THD::restore_globals() {
+  // Remove reference to specific OS thread.
+  real_id = 0;
   /*
     Assert that thread_stack is initialized: it's necessary to be able
     to track stack overrun.
@@ -1922,6 +1924,14 @@ void THD::shutdown_active_vio() {
   DBUG_TRACE;
   mysql_mutex_assert_owner(&LOCK_thd_data);
   if (active_vio) {
+#ifdef USE_PPOLL_IN_VIO
+    // Vio::thread_id may not be correct if the THD has been
+    // associated with a different OS thread since the Vio object was
+    // created. So we store the current THD::real_id here so that
+    // vio_shutdown() will not try to send SIGALRM to an incorrect, or
+    // invalid thread id.
+    active_vio->thread_id = real_id;
+#endif /* USE_PPOLL_IN_VIO */
     vio_shutdown(active_vio);
     active_vio = nullptr;
     m_SSL = nullptr;
@@ -1932,6 +1942,14 @@ void THD::shutdown_clone_vio() {
   DBUG_TRACE;
   mysql_mutex_assert_owner(&LOCK_thd_data);
   if (clone_vio != nullptr) {
+#ifdef USE_PPOLL_IN_VIO
+    // Vio::thread_id may not be correct if the THD has been
+    // associated with a different OS thread since the Vio object was
+    // created. So we store the current THD::real_id here so that
+    // vio_shutdown() will not try to send SIGALRM to an incorrect, or
+    // invalid thread id.
+    clone_vio->thread_id = real_id;
+#endif /* USE_PPOLL_IN_VIO */
     vio_shutdown(clone_vio);
     clone_vio = nullptr;
   }

sql/sql_class.h

@@ -2399,17 +2399,17 @@ class THD : public MDL_context_owner,
   /* Statement id is thread-wide. This counter is used to generate ids */
   ulong statement_id_counter;
   ulong rand_saved_seed1, rand_saved_seed2;
-  my_thread_t real_id; /* For debugging */
-                       /**
-                         This counter is 32 bit because of the client protocol.
-                     
-                         @note It is not meant to be used for my_thread_self(), see @c real_id for
-                         this.
-                     
-                         @note Set to reserved_thread_id on initialization. This is a magic
-                         value that is only to be used for temporary THDs not present in
-                         the global THD list.
-                       */
+  my_thread_t real_id;
+  /**
+    This counter is 32 bit because of the client protocol.
+
+    @note It is not meant to be used for my_thread_self(), see @c real_id for
+    this.
+
+    @note Set to reserved_thread_id on initialization. This is a magic
+    value that is only to be used for temporary THDs not present in
+    the global THD list.
+  */
  private:
   my_thread_id m_thread_id;
 

sql/sql_thd_api.cc

@@ -159,9 +159,13 @@ void thd_clear_errors(THD *thd [[maybe_unused]]) { set_my_errno(0); }
   Close the socket used by this connection
 
   @param thd                THD object
+  @note Expects lock on thd->LOCK_thd_data.
 */
 
-void thd_close_connection(THD *thd) { thd->get_protocol_classic()->shutdown(); }
+void thd_close_connection(THD *thd) {
+  mysql_mutex_assert_owner(&thd->LOCK_thd_data);
+  thd->shutdown_active_vio();
+}
 
 /**
   Get current THD object from thread local data

vio/viosocket.cc

@@ -52,9 +52,9 @@
 #include "my_inttypes.h"
 #include "my_io.h"
 #include "my_macros.h"
+#include "mysys_err.h"
 #include "template_utils.h"
 #include "vio/vio_priv.h"
-
 #ifdef FIONREAD_IN_SYS_FILIO
 #include <sys/filio.h>
 #endif
@@ -470,12 +470,21 @@ int vio_shutdown(Vio *vio) {
     if (mysql_socket_shutdown(vio->mysql_socket, SHUT_RDWR)) r = -1;
 
 #ifdef USE_PPOLL_IN_VIO
-    if (vio->thread_id != 0 && vio->poll_shutdown_flag.test_and_set()) {
+    assert(vio->thread_id.has_value());
+    DBUG_PRINT("tp_exit",
+               ("Shutting down vio for thread: %lu", vio->thread_id.value()));
+    if (vio->thread_id.value() != 0 && vio->poll_shutdown_flag.test_and_set()) {
       // Send signal to wake up from poll.
-      if (pthread_kill(vio->thread_id, SIGALRM) == 0)
+      int en = pthread_kill(vio->thread_id.value(), SIGALRM);
+      if (en == 0)
         vio_wait_until_woken(vio);
-      else
-        perror("Error in pthread_kill");
+      else {
+        char buf[512];
+        my_message_local(WARNING_LEVEL, EE_PTHREAD_KILL_FAILED,
+                         vio->thread_id.value(), "SIGALRM",
+                         strerror_r(en, buf, sizeof(buf)));
+        assert("pthread_kill failure" == nullptr);
+      }
     }
 #elif defined HAVE_KQUEUE
     if (vio->kq_fd != -1 && vio->kevent_wakeup_flag.test_and_set())
@@ -846,12 +855,14 @@ int vio_io_wait(Vio *vio, enum enum_vio_io_event event, int timeout) {
   do {
 #ifdef USE_PPOLL_IN_VIO
     /*
-      vio->signal_mask is only useful when thread_id != 0.
-      thread_id is only set for servers, so signal_mask is unused for client
-      libraries.
+      thread_id is only set to std::nullopt or a non-zero value for servers, so
+      signal_mask is unused for client libraries. A valid value for thread_id
+      is not assigned until there is attempt to shut down the connection.
     */
     ret = ppoll(&pfd, 1, ts_ptr,
-                vio->thread_id != 0 ? &vio->signal_mask : nullptr);
+                !vio->thread_id.has_value() || (vio->thread_id.value() != 0)
+                    ? &vio->signal_mask
+                    : nullptr);
 #else
     ret = poll(&pfd, 1, timeout);
 #endif