Bug#25396

"Valgrind leak in closecon_handlerton"
  plugin_shutdown() calls plugin_deinitialize() which calls ha_finalize_handlerton().
  ndbcluster_end() fails to wait for the ndb utility thread to exit which results in
  the handlerton struct being freed before the ndb utility thread has destroyed it's THD
  but before the plugin has been marked as UNINITIALIZED

Bug is caused by misuse of abort_loops variable and not locking mutex during calls to
pthread condition variable functions causing a race in valgrind's pthread_cond_wait
implementation.


sql/ha_ndbcluster.cc:
  Bug25396
  Valgrind requires that mutex be held during call to pthread_cond_signal.
  Change pthread_cond_timedwait() to pthread_cond_wait() where the timeout is not needed.
  Ensure that appropiate variables are protected by mutex.
  Remove use of abort_loop global variable.
  Ensure that ndbcluster_end waits for util thread to exit.
  Add an extra cond_var as insurance against non-conforming pthreads implementations.
sql/mysqld.cc:
  Bug25386
  Valgrind requires that mutex be held during call to pthread_cond_signal.
BUILD/compile-amd64-valgrind-max:
  New BitKeeper file ``BUILD/compile-amd64-valgrind-max''

BUILD/compile-amd64-valgrind-max

@@ -0,0 +1,24 @@
+#! /bin/sh
+
+path=`dirname $0`
+. "$path/SETUP.sh"
+
+extra_flags="$amd64_cflags $debug_cflags $valgrind_flags"
+extra_configs="$amd64_configs $debug_configs $max_configs"
+
+. "$path/FINISH.sh"
+
+if test -z "$just_print"
+then
+  set +v +x
+  echo "\
+******************************************************************************
+Note that by default BUILD/compile-pentium-valgrind-max calls 'configure' with
+--enable-assembler. When Valgrind detects an error involving an assembly
+function (for example an uninitialized value used as an argument of an
+assembly function), Valgrind will not print the stacktrace and 'valgrind
+--gdb-attach=yes' will not work either. If you need a stacktrace in those
+cases, you have to run BUILD/compile-pentium-valgrind-max with the
+--disable-assembler argument.
+******************************************************************************"
+fi

sql/ha_ndbcluster.cc

@@ -133,6 +133,7 @@ static uint ndbcluster_alter_table_flags(uint flags)
 }
 
 static int ndbcluster_inited= 0;
+static int ndbcluster_terminating= 0;
 
 static Ndb* g_ndb= NULL;
 Ndb_cluster_connection* g_ndb_cluster_connection= NULL;
@@ -159,6 +160,7 @@ pthread_t ndb_util_thread;
 int ndb_util_thread_running= 0;
 pthread_mutex_t LOCK_ndb_util_thread;
 pthread_cond_t COND_ndb_util_thread;
+pthread_cond_t COND_ndb_util_ready;
 pthread_handler_t ndb_util_thread_func(void *arg);
 ulong ndb_cache_check_time;
 
@@ -6588,6 +6590,7 @@ int ndbcluster_find_files(handlerton *hton, THD *thd,
 /* Call back after cluster connect */
 static int connect_callback()
 {
+  pthread_mutex_lock(&LOCK_ndb_util_thread);
   update_status_variables(g_ndb_cluster_connection);
 
   uint node_id, i= 0;
@@ -6597,6 +6600,7 @@ static int connect_callback()
     g_node_id_map[node_id]= i++;
 
   pthread_cond_signal(&COND_ndb_util_thread);
+  pthread_mutex_unlock(&LOCK_ndb_util_thread);
   return 0;
 }
 
@@ -6607,6 +6611,15 @@ static int ndbcluster_init(void *p)
   int res;
   DBUG_ENTER("ndbcluster_init");
 
+  if (ndbcluster_inited)
+    DBUG_RETURN(FALSE);
+
+  pthread_mutex_init(&ndbcluster_mutex,MY_MUTEX_INIT_FAST);
+  pthread_mutex_init(&LOCK_ndb_util_thread, MY_MUTEX_INIT_FAST);
+  pthread_cond_init(&COND_ndb_util_thread, NULL);
+  pthread_cond_init(&COND_ndb_util_ready, NULL);
+  ndb_util_thread_running= -1;
+  ndbcluster_terminating= 0;
   ndb_dictionary_is_mysqld= 1;
   ndbcluster_hton= (handlerton *)p;
 
@@ -6705,17 +6718,12 @@ static int ndbcluster_init(void *p)
   
   (void) hash_init(&ndbcluster_open_tables,system_charset_info,32,0,0,
                    (hash_get_key) ndbcluster_get_key,0,0);
-  pthread_mutex_init(&ndbcluster_mutex,MY_MUTEX_INIT_FAST);
 #ifdef HAVE_NDB_BINLOG
   /* start the ndb injector thread */
   if (ndbcluster_binlog_start())
     goto ndbcluster_init_error;
 #endif /* HAVE_NDB_BINLOG */
 
-  pthread_mutex_init(&LOCK_ndb_util_thread, MY_MUTEX_INIT_FAST);
-  pthread_cond_init(&COND_ndb_util_thread, NULL);
-
-
   ndb_cache_check_time = opt_ndb_cache_check_time;
   // Create utility thread
   pthread_t tmp;
@@ -6726,14 +6734,26 @@ static int ndbcluster_init(void *p)
     pthread_mutex_destroy(&ndbcluster_mutex);
     pthread_mutex_destroy(&LOCK_ndb_util_thread);
     pthread_cond_destroy(&COND_ndb_util_thread);
+    pthread_cond_destroy(&COND_ndb_util_ready);
     goto ndbcluster_init_error;
   }
 
   /* Wait for the util thread to start */
   pthread_mutex_lock(&LOCK_ndb_util_thread);
-  while (!ndb_util_thread_running)
-    pthread_cond_wait(&COND_ndb_util_thread, &LOCK_ndb_util_thread);
+  while (ndb_util_thread_running < 0)
+    pthread_cond_wait(&COND_ndb_util_ready, &LOCK_ndb_util_thread);
   pthread_mutex_unlock(&LOCK_ndb_util_thread);
+  
+  if (!ndb_util_thread_running)
+  {
+    DBUG_PRINT("error", ("ndb utility thread exited prematurely"));
+    hash_free(&ndbcluster_open_tables);
+    pthread_mutex_destroy(&ndbcluster_mutex);
+    pthread_mutex_destroy(&LOCK_ndb_util_thread);
+    pthread_cond_destroy(&COND_ndb_util_thread);
+    pthread_cond_destroy(&COND_ndb_util_ready);
+    goto ndbcluster_init_error;
+  }
 
   ndbcluster_inited= 1;
   DBUG_RETURN(FALSE);
@@ -6760,22 +6780,12 @@ static int ndbcluster_end(handlerton *hton, ha_panic_function type)
   ndbcluster_inited= 0;
 
   /* wait for util thread to finish */
+  sql_print_information("Stopping Cluster Utility thread");
   pthread_mutex_lock(&LOCK_ndb_util_thread);
-  if (ndb_util_thread_running > 0)
-  {
-    pthread_cond_signal(&COND_ndb_util_thread);
-    pthread_mutex_unlock(&LOCK_ndb_util_thread);
-
-    pthread_mutex_lock(&LOCK_ndb_util_thread);
-    while (ndb_util_thread_running > 0)
-    {
-      struct timespec abstime;
-      set_timespec(abstime, 1);
-      pthread_cond_timedwait(&COND_ndb_util_thread,
-                             &LOCK_ndb_util_thread,
-                             &abstime);
-    }
-  }
+  ndbcluster_terminating= 1;
+  pthread_cond_signal(&COND_ndb_util_thread);
+  while (ndb_util_thread_running > 0)
+    pthread_cond_wait(&COND_ndb_util_ready, &LOCK_ndb_util_thread);
   pthread_mutex_unlock(&LOCK_ndb_util_thread);
 
 
@@ -6824,6 +6834,7 @@ static int ndbcluster_end(handlerton *hton, ha_panic_function type)
   pthread_mutex_destroy(&ndbcluster_mutex);
   pthread_mutex_destroy(&LOCK_ndb_util_thread);
   pthread_cond_destroy(&COND_ndb_util_thread);
+  pthread_cond_destroy(&COND_ndb_util_ready);
   DBUG_RETURN(0);
 }
 
@@ -8357,6 +8368,8 @@ pthread_handler_t ndb_util_thread_func(void *arg __attribute__((unused)))
   my_thread_init();
   DBUG_ENTER("ndb_util_thread");
   DBUG_PRINT("enter", ("ndb_cache_check_time: %lu", ndb_cache_check_time));
+ 
+   pthread_mutex_lock(&LOCK_ndb_util_thread);
 
   thd= new THD; /* note that contructor of THD uses DBUG_ */
   THD_CHECK_SENTRY(thd);
@@ -8366,12 +8379,7 @@ pthread_handler_t ndb_util_thread_func(void *arg __attribute__((unused)))
 
   thd->thread_stack= (char*)&thd; /* remember where our stack is */
   if (thd->store_globals())
-  {
-    thd->cleanup();
-    delete thd;
-    ndb_util_thread_running= 0;
-    DBUG_RETURN(NULL);
-  }
+    goto ndb_util_thread_fail;
   thd->init_for_queries();
   thd->version=refresh_version;
   thd->set_time();
@@ -8382,15 +8390,27 @@ pthread_handler_t ndb_util_thread_func(void *arg __attribute__((unused)))
   thd->main_security_ctx.priv_user = 0;
   thd->current_stmt_binlog_row_based= TRUE;     // If in mixed mode
 
+  /* Signal successful initialization */
   ndb_util_thread_running= 1;
-  pthread_cond_signal(&COND_ndb_util_thread);
+  pthread_cond_signal(&COND_ndb_util_ready);
+  pthread_mutex_unlock(&LOCK_ndb_util_thread);
 
   /*
     wait for mysql server to start
   */
   pthread_mutex_lock(&LOCK_server_started);
   while (!mysqld_server_started)
-    pthread_cond_wait(&COND_server_started, &LOCK_server_started);
+  {
+    set_timespec(abstime, 1);
+    pthread_cond_timedwait(&COND_server_started, &LOCK_server_started,
+	                       &abstime);
+    if (ndbcluster_terminating)
+    {
+      pthread_mutex_unlock(&LOCK_server_started);
+      pthread_mutex_lock(&LOCK_ndb_util_thread);
+      goto ndb_util_thread_end;
+    }
+  }
   pthread_mutex_unlock(&LOCK_server_started);
 
   /*
@@ -8400,15 +8420,9 @@ pthread_handler_t ndb_util_thread_func(void *arg __attribute__((unused)))
   while (!ndb_cluster_node_id && (ndbcluster_hton->slot != ~(uint)0))
   {
     /* ndb not connected yet */
-    set_timespec(abstime, 1);
-    pthread_cond_timedwait(&COND_ndb_util_thread,
-                           &LOCK_ndb_util_thread,
-                           &abstime);
-    if (abort_loop)
-    {
-      pthread_mutex_unlock(&LOCK_ndb_util_thread);
+    pthread_cond_wait(&COND_ndb_util_thread, &LOCK_ndb_util_thread);
+    if (ndbcluster_terminating)
       goto ndb_util_thread_end;
-    }
   }
   pthread_mutex_unlock(&LOCK_ndb_util_thread);
 
@@ -8416,6 +8430,7 @@ pthread_handler_t ndb_util_thread_func(void *arg __attribute__((unused)))
   if (!(thd_ndb= ha_ndbcluster::seize_thd_ndb()))
   {
     sql_print_error("Could not allocate Thd_ndb object");
+    pthread_mutex_lock(&LOCK_ndb_util_thread);
     goto ndb_util_thread_end;
   }
   set_thd_ndb(thd, thd_ndb);
@@ -8434,19 +8449,20 @@ pthread_handler_t ndb_util_thread_func(void *arg __attribute__((unused)))
 #endif
 
   set_timespec(abstime, 0);
-  for (;!abort_loop;)
+  for (;;)
   {
     pthread_mutex_lock(&LOCK_ndb_util_thread);
-    pthread_cond_timedwait(&COND_ndb_util_thread,
-                           &LOCK_ndb_util_thread,
-                           &abstime);
+    if (!ndbcluster_terminating)
+      pthread_cond_timedwait(&COND_ndb_util_thread,
+                             &LOCK_ndb_util_thread,
+                             &abstime);
+    if (ndbcluster_terminating) /* Shutting down server */
+      goto ndb_util_thread_end;
     pthread_mutex_unlock(&LOCK_ndb_util_thread);
 #ifdef NDB_EXTRA_DEBUG_UTIL_THREAD
     DBUG_PRINT("ndb_util_thread", ("Started, ndb_cache_check_time: %lu",
                                    ndb_cache_check_time));
 #endif
-    if (abort_loop)
-      break; /* Shutting down server */
 
 #ifdef HAVE_NDB_BINLOG
     /*
@@ -8569,13 +8585,18 @@ pthread_handler_t ndb_util_thread_func(void *arg __attribute__((unused)))
       abstime.tv_nsec-= 1000000000;
     }
   }
+
+  pthread_mutex_lock(&LOCK_ndb_util_thread);
+
 ndb_util_thread_end:
-  sql_print_information("Stopping Cluster Utility thread");
   net_end(&thd->net);
+ndb_util_thread_fail:
   thd->cleanup();
   delete thd;
-  pthread_mutex_lock(&LOCK_ndb_util_thread);
+  
+  /* signal termination */
   ndb_util_thread_running= 0;
+  pthread_cond_signal(&COND_ndb_util_ready);
   pthread_mutex_unlock(&LOCK_ndb_util_thread);
   DBUG_PRINT("exit", ("ndb_util_thread"));
   my_thread_end();

sql/mysqld.cc

@@ -3706,15 +3706,18 @@ we force server id to 2, but this MySQL server will not act as a slave.");
                          mysqld_port,
                          MYSQL_COMPILATION_COMMENT);
 
-  // Signal threads waiting for server to be started
-  mysqld_server_started= 1;
-  pthread_cond_signal(&COND_server_started);
-
   if (!opt_noacl)
   {
     if (Events::get_instance()->init())
       unireg_abort(1);
   }
+
+  /* Signal threads waiting for server to be started */
+  pthread_mutex_lock(&LOCK_server_started);
+  mysqld_server_started= 1;
+  pthread_cond_signal(&COND_server_started);
+  pthread_mutex_unlock(&LOCK_server_started);
+
 #if defined(__NT__) || defined(HAVE_SMEM)
   handle_connections_methods();
 #else