Bug#36804874: The table access service from a non-THD thread

 always frees the mysys thread

When the calling thread does not have a THD attached
(current_thd thread local is null) but it has mysys thread local set
(THR_mysys is non-null) table_access_factory_v1::create() will assume
that there's neither of the two and will call my_thread_init().
If the calling thread already has THR_mysys set, my_thread_init() is a
no-op.
However, table_access_factory_v1::destroy() does call my_thread_end()
if there's no THD "parent" thread context to restore.
This is never a no-op and as a result the calling thread looses its
THR_mysys value.
Thus, if further calls to THD methods (as e.g. done by the
group_membership_listener::notify_view_change() calling
THD::store_globals() that in turn calls set_my_thread_var_id()) that
assume that THR_mysys is set are going to crash.

Fixed by:
1. implementing a mysys API to check if THR_mysys is initialized: bool my_thread_is_inited()
2. Call this method in table_access_factory_v1::create() and cache the
result in the Table_access structure.
3. At table_access_factory_v1::destroy(), call if the mysys THD was
inited and only call my_thread_end() if it wasn't.

No test case added as the sequence is used in an upcoming change and
thus is going to be properly tested.

Also removed a superfluous call to restore_globals(), as it's already
called by THD::release_resources().

Change-Id: I5d1271cc62b6dc6f4eeab1aa7ec53d68f514d956

Author: gkodinov

include/my_thread.h

@@ -178,5 +178,6 @@ extern void my_thread_global_end();
 // Need to be extern "C" for the time being, due to memcached.
 extern "C" bool my_thread_init();
 extern "C" void my_thread_end();
+extern bool my_thread_is_inited();
 
 #endif /* MY_THREAD_INCLUDED */

mysys/my_thr_init.cc

@@ -293,6 +293,20 @@ extern "C" bool my_thread_init() {
   return false;
 }
 
+bool my_thread_is_inited() {
+#ifndef NDEBUG
+  return mysys_thread_var() != nullptr;
+#else
+  /*
+    Return true here because for non-debug binaries my_thread_end() should not
+    be called on native threads because my_thread_init() is a no-op for these
+    and my_thread_end() just deregisters the thread from P_S.
+    And since there's no registration in P_S at my_thread_init() we should not
+    be calling my_thread_end() for these threads.
+  */
+  return true;
+#endif
+}
 /**
   Deallocate memory used by the thread for book-keeping
 

sql/server_component/table_access_service.cc

@@ -528,6 +528,8 @@ class Table_access_impl {
   size_t m_max_count;
   bool m_write;
   bool m_in_tx;
+  /** true if the mysys thread state needs cleanup */
+  bool m_clear_mysys;
 
   THD *m_parent_thd;
   THD *m_child_thd;
@@ -584,7 +586,11 @@ void TA_key_impl::key_copy(uchar *record, uint key_length) {
 }
 
 Table_access_impl::Table_access_impl(THD *thd, size_t count)
-    : m_current_count(0), m_max_count(count), m_write(false), m_in_tx(false) {
+    : m_current_count(0),
+      m_max_count(count),
+      m_write(false),
+      m_in_tx(false),
+      m_clear_mysys{false} {
   m_parent_thd = thd;
 
   m_child_thd = new THD(true);
@@ -597,6 +603,7 @@ Table_access_impl::Table_access_impl(THD *thd, size_t count)
     m_child_thd->security_context()->assign_user(
         STRING_WITH_LEN("table_access"));
     m_child_thd->security_context()->skip_grants("", "");
+    this->m_clear_mysys = !my_thread_is_inited();
     my_thread_init();
   }
 
@@ -635,7 +642,6 @@ Table_access_impl::~Table_access_impl() {
   }
 
   m_child_thd->release_resources();
-  m_child_thd->restore_globals();
 
   if (m_parent_thd) m_parent_thd->store_globals();
 
@@ -646,7 +652,7 @@ Table_access_impl::~Table_access_impl() {
   delete[] m_table_array;
   delete[] m_table_state_array;
 
-  if (!m_parent_thd) my_thread_end();
+  if (m_clear_mysys && !m_parent_thd) my_thread_end();
 
   // FIXME : kill flag ?
   // FIXME : nested THD status variables ?