Fixed compilation failure using clang

Both aria and myisam storage engines feature a logic path in
thr_find_all_keys that leads to undefined behaviour by bypassing the
initialization code of variables after my_thread_init().

By refactoring the nested logic into a separate function, this problem
is resolved.

include/myisam.h

@@ -330,7 +330,8 @@ typedef struct st_sort_info
   my_off_t filelength, dupp, buff_length;
   ha_rows max_records;
   uint current_key, total_keys;
-  uint got_error, threads_running;
+  volatile uint got_error;
+  uint threads_running;
   myf myf_rw;
   enum data_file_type new_data_file_type;
 } MI_SORT_INFO;

storage/maria/ma_sort.c

@@ -333,39 +333,31 @@ err:
 } /* find_all_keys */
 
 
-/* Search after all keys and place them in a temp. file */
-
-pthread_handler_t _ma_thr_find_all_keys(void *arg)
+static my_bool _ma_thr_find_all_keys_exec(MARIA_SORT_PARAM* sort_param)
 {
-  MARIA_SORT_PARAM *sort_param= (MARIA_SORT_PARAM*) arg;
-  int error;
-  size_t memavl,old_memavl;
-  uint sort_length;
-  ulong idx, maxbuffer, keys;
-  uchar **sort_keys=0;
-
-  LINT_INIT(keys);
-
-  error=1;
-
-  if (my_thread_init())
-    goto err;
-
-  { /* Add extra block since DBUG_ENTER declare variables */
   DBUG_ENTER("_ma_thr_find_all_keys");
   DBUG_PRINT("enter", ("master: %d", sort_param->master));
+
+  my_bool error= FALSE;
+  ulonglong memavl, old_memavl;
+  uint UNINIT_VAR(keys), idx;
+  uint sort_length;
+  uint maxbuffer;
+  uchar **sort_keys= NULL;
+
   if (sort_param->sort_info->got_error)
-      goto err;
+    DBUG_RETURN(TRUE);
 
   set_sort_param_read_write(sort_param);
 
+
   my_b_clear(&sort_param->tempfile);
   my_b_clear(&sort_param->tempfile_for_exceptions);
-    bzero((char*) &sort_param->buffpek,sizeof(sort_param->buffpek));
+  bzero((char*) &sort_param->buffpek, sizeof(sort_param->buffpek));
   bzero((char*) &sort_param->unique,  sizeof(sort_param->unique));
 
   memavl=        max(sort_param->sortbuff_size, MIN_SORT_MEMORY);
-    idx=          (uint)sort_param->sort_info->max_records;
+  idx=           sort_param->sort_info->max_records;
   sort_length=   sort_param->key_length;
   maxbuffer=     1;
 
@@ -397,10 +389,10 @@ pthread_handler_t _ma_thr_find_all_keys(void *arg)
                     HA_FT_MAXBYTELEN : 0), MYF(0))))
     {
       if (my_init_dynamic_array(&sort_param->buffpek, sizeof(BUFFPEK),
-                                  maxbuffer, maxbuffer/2))
+                                maxbuffer, maxbuffer / 2))
       {
         my_free(sort_keys);
-          sort_keys= (uchar **) NULL;            /* for err: label */
+        sort_keys= NULL;          /* Safety against double free on error. */
       }
       else
         break;
@@ -410,16 +402,24 @@ pthread_handler_t _ma_thr_find_all_keys(void *arg)
         old_memavl > MIN_SORT_MEMORY)
       memavl= MIN_SORT_MEMORY;
   }
+
   if (memavl < MIN_SORT_MEMORY)
   {
+    /* purecov: begin inspected */
     _ma_check_print_error(sort_param->sort_info->param,
-                            "Aria sort buffer too small");
-      goto err; /* purecov: tested */
+                          "aria_sort_buffer_size is too small. Current "
+                          "aria_sort_buffer_size: %llu  rows: %llu  "
+                          "sort_length: %u",
+                          (ulonglong) sort_param->sortbuff_size,
+                          (ulonglong) idx, sort_length);
+    my_errno= ENOMEM;
+    goto err;
+    /* purecov: end inspected */
   }
 
   if (sort_param->sort_info->param->testflag & T_VERBOSE)
-      printf("Key %d - Allocating buffer for %lu keys\n",
-             sort_param->key+1, (ulong) keys);
+    printf("Key %d - Allocating buffer for %llu keys\n",
+           sort_param->key + 1, (ulonglong) keys);
   sort_param->sort_keys= sort_keys;
 
   idx= error= 0;
@@ -431,7 +431,7 @@ pthread_handler_t _ma_thr_find_all_keys(void *arg)
   {
     if (sort_param->real_key_length > sort_param->key_length)
     {
-        if (write_key(sort_param,sort_keys[idx],
+      if (write_key(sort_param, sort_keys[idx],
                     &sort_param->tempfile_for_exceptions))
         goto err;
       continue;
@@ -440,24 +440,21 @@ pthread_handler_t _ma_thr_find_all_keys(void *arg)
     if (++idx == keys)
     {
       if (sort_param->write_keys(sort_param, sort_keys, idx - 1,
-                                   (BUFFPEK *)alloc_dynamic(&sort_param->
-                                                            buffpek),
+                                 (BUFFPEK *)alloc_dynamic(&sort_param->buffpek),
                                  &sort_param->tempfile))
         goto err;
       sort_keys[0]= (uchar*) (sort_keys+keys);
-        memcpy(sort_keys[0], sort_keys[idx - 1],
-               (size_t) sort_param->key_length);
+      memcpy(sort_keys[0], sort_keys[idx - 1], (size_t) sort_param->key_length);
       idx= 1;
     }
-      sort_keys[idx]=sort_keys[idx - 1] + sort_param->key_length;
+    sort_keys[idx]= sort_keys[idx - 1] + sort_param->key_length;
   }
   if (error > 0)
     goto err;
   if (sort_param->buffpek.elements)
   {
     if (sort_param->write_keys(sort_param,sort_keys, idx,
-                                 (BUFFPEK *) alloc_dynamic(&sort_param->
-                                                           buffpek),
+                               (BUFFPEK *) alloc_dynamic(&sort_param->buffpek),
                                &sort_param->tempfile))
       goto err;
     sort_param->keys= (sort_param->buffpek.elements - 1) * (keys - 1) + idx;
@@ -466,25 +463,43 @@ pthread_handler_t _ma_thr_find_all_keys(void *arg)
     sort_param->keys= idx;
 
   sort_param->sort_keys_length= keys;
-    goto ok;
+  DBUG_RETURN(FALSE);
 
 err:
   DBUG_PRINT("error", ("got some error"));
-    sort_param->sort_info->got_error= 1; /* no need to protect with a mutex */
   my_free(sort_keys);
-    sort_param->sort_keys=0;
+  sort_param->sort_keys= 0;
   delete_dynamic(& sort_param->buffpek);
   close_cached_file(&sort_param->tempfile);
   close_cached_file(&sort_param->tempfile_for_exceptions);
 
-ok:
+  DBUG_RETURN(TRUE);
+}
+
+/* Search after all keys and place them in a temp. file */
+
+pthread_handler_t _ma_thr_find_all_keys(void *arg)
+{
+  MARIA_SORT_PARAM *sort_param= (MARIA_SORT_PARAM*) arg;
+  my_bool error= FALSE;
+  /* If my_thread_init fails */
+  if (my_thread_init() || _ma_thr_find_all_keys_exec(sort_param))
+    error= TRUE;
+
+  /*
+     Thread must clean up after itself.
+  */
   free_root(&sort_param->wordroot, MYF(0));
   /*
      Detach from the share if the writer is involved. Avoid others to
      be blocked. This includes a flush of the write buffer. This will
      also indicate EOF to the readers.
+     That means that a writer always gets here first and readers -
+     only when they see EOF. But if a reader finishes prematurely
+     because of an error it may reach this earlier - don't allow it
+     to detach the writer thread.
      */
-    if (sort_param->sort_info->info->rec_cache.share)
+  if (sort_param->master && sort_param->sort_info->info->rec_cache.share)
     remove_io_thread(&sort_param->sort_info->info->rec_cache);
 
   /* Readers detach from the share if any. Avoid others to be blocked. */
@@ -492,11 +507,13 @@ ok:
     remove_io_thread(&sort_param->read_cache);
 
   mysql_mutex_lock(&sort_param->sort_info->mutex);
+  if (error)
+    sort_param->sort_info->got_error= 1;
+
   if (!--sort_param->sort_info->threads_running)
     mysql_cond_signal(&sort_param->sort_info->cond);
   mysql_mutex_unlock(&sort_param->sort_info->mutex);
-    DBUG_PRINT("exit", ("======== ending thread ========"));
-  }
+
   my_thread_end();
   return NULL;
 }

storage/maria/maria_def.h

@@ -67,7 +67,8 @@ typedef struct st_maria_sort_info
   pgcache_page_no_t page;
   ha_rows max_records;
   uint current_key, total_keys;
-  uint got_error, threads_running;
+  volatile uint got_error;
+  uint threads_running;
   myf myf_rw;
   enum data_file_type new_data_file_type, org_data_file_type;
 } MARIA_SORT_INFO;

storage/myisam/sort.c

@@ -318,29 +318,22 @@ static ha_rows find_all_keys(MI_SORT_PARAM *info, uint keys,
   DBUG_RETURN((*maxbuffer)*(keys-1)+idx);
 } /* find_all_keys */
 
-/* Search after all keys and place them in a temp. file */
-
-pthread_handler_t thr_find_all_keys(void *arg)
+static my_bool thr_find_all_keys_exec(MI_SORT_PARAM *sort_param)
 {
-  MI_SORT_PARAM *sort_param= (MI_SORT_PARAM*) arg;
-  int error;
-  ulonglong memavl, old_memavl;
-  uint keys, sort_length;
-  uint idx, maxbuffer;
-  uchar **sort_keys=0;
-
-  LINT_INIT(keys);
-
-  error=1;
-
-  if (my_thread_init())
-    goto err;
-
-  { /* Add extra block since DBUG_ENTER declare variables */
   DBUG_ENTER("thr_find_all_keys");
   DBUG_PRINT("enter", ("master: %d", sort_param->master));
+
+  ulonglong memavl, old_memavl;
+  uint UNINIT_VAR(keys), idx;
+  uint sort_length;
+  uint maxbuffer;
+  uchar **sort_keys= NULL;
+
+  my_bool error= FALSE;
   if (sort_param->sort_info->got_error)
-      goto err;
+    error= TRUE;
+  if (error)
+    DBUG_RETURN(error);
 
   set_sort_param_read_write(sort_param);
 
@@ -348,10 +341,9 @@ pthread_handler_t thr_find_all_keys(void *arg)
   my_b_clear(&sort_param->tempfile_for_exceptions);
   bzero((char*) &sort_param->buffpek, sizeof(sort_param->buffpek));
   bzero((char*) &sort_param->unique,  sizeof(sort_param->unique));
-    sort_keys= (uchar **) NULL;
 
   memavl=        max(sort_param->sortbuff_size, MIN_SORT_BUFFER);
-    idx=          (uint)sort_param->sort_info->max_records;
+  idx=           (uint) sort_param->sort_info->max_records;
   sort_length=   sort_param->key_length;
   maxbuffer=     1;
 
@@ -376,22 +368,23 @@ pthread_handler_t thr_find_all_keys(void *arg)
             keys < (uint) maxbuffer)
         {
           mi_check_print_error(sort_param->sort_info->param,
-                                 "myisam_sort_buffer_size is too small");
-            goto err;
+              "myisam_sort_buffer_size is too small. Current "
+              "myisam_sort_buffer_size: %llu  rows: %llu  sort_length: %u",
+              sort_param->sortbuff_size, (ulonglong) idx, sort_length);
+          DBUG_RETURN(TRUE);
         }
       }
       while ((maxbuffer= (int) (idx/(keys-1)+1)) != skr);
     }
-      if ((sort_keys= (uchar**)
-           my_malloc(keys*(sort_length+sizeof(char*))+
+    if ((sort_keys= my_malloc(keys * (sort_length + sizeof(char *)) +
                               ((sort_param->keyinfo->flag & HA_FULLTEXT) ?
                                  HA_FT_MAXBYTELEN : 0), MYF(0))))
     {
       if (my_init_dynamic_array(&sort_param->buffpek, sizeof(BUFFPEK),
-                                  maxbuffer, maxbuffer/2))
+                                maxbuffer, min(maxbuffer / 2, 1000)))
       {
         my_free(sort_keys);
-          sort_keys= (uchar **) NULL; /* for err: label */
+        sort_keys= NULL;          /* Safety against double free on error. */
       }
       else
         break;
@@ -403,14 +396,19 @@ pthread_handler_t thr_find_all_keys(void *arg)
   }
   if (memavl < MIN_SORT_BUFFER)
   {
+    /* purecov: begin inspected */
     mi_check_print_error(sort_param->sort_info->param,
-                           "MyISAM sort buffer too small");
-      goto err; /* purecov: tested */
+        "myisam_sort_buffer_size is too small. Current "
+        "myisam_sort_buffer_size: %llu  rows: %llu  sort_length: %u",
+        sort_param->sortbuff_size, (ulonglong) idx, sort_length);
+    my_errno= ENOMEM;
+    goto err;
+    /* purecov: end inspected */
   }
 
   if (sort_param->sort_info->param->testflag & T_VERBOSE)
-      printf("Key %d - Allocating buffer for %d keys\n",
-             sort_param->key + 1, keys);
+    printf("Key %d - Allocating buffer for %llu keys\n",
+           sort_param->key + 1, (ulonglong) keys);
   sort_param->sort_keys= sort_keys;
 
   idx= error= 0;
@@ -440,8 +438,10 @@ pthread_handler_t thr_find_all_keys(void *arg)
     }
     sort_keys[idx]= sort_keys[idx - 1] + sort_param->key_length;
   }
+
   if (error > 0)
     goto err;
+
   if (sort_param->buffpek.elements)
   {
     if (sort_param->write_keys(sort_param, sort_keys, idx,
@@ -454,7 +454,8 @@ pthread_handler_t thr_find_all_keys(void *arg)
     sort_param->keys= idx;
 
   sort_param->sort_keys_length= keys;
-    goto ok;
+
+  DBUG_RETURN(FALSE);
 
 err:
   DBUG_PRINT("error", ("got some error"));
@@ -465,7 +466,22 @@ err:
   close_cached_file(&sort_param->tempfile);
   close_cached_file(&sort_param->tempfile_for_exceptions);
 
-ok:
+  DBUG_RETURN(TRUE);
+}
+
+/* Search after all keys and place them in a temp. file */
+
+pthread_handler_t thr_find_all_keys(void *arg)
+{
+  MI_SORT_PARAM *sort_param= (MI_SORT_PARAM*) arg;
+  my_bool error= FALSE;
+  /* If my_thread_init fails */
+  if (my_thread_init() || thr_find_all_keys_exec(sort_param))
+    error= TRUE;
+
+  /*
+     Thread must clean up after itself.
+  */
   free_root(&sort_param->wordroot, MYF(0));
   /*
      Detach from the share if the writer is involved. Avoid others to
@@ -484,11 +500,12 @@ ok:
     remove_io_thread(&sort_param->read_cache);
 
   mysql_mutex_lock(&sort_param->sort_info->mutex);
+  if (error)
+    sort_param->sort_info->got_error= 1;
+
   if (!--sort_param->sort_info->threads_running)
     mysql_cond_signal(&sort_param->sort_info->cond);
   mysql_mutex_unlock(&sort_param->sort_info->mutex);
-    DBUG_PRINT("exit", ("======== ending thread ========"));
-  }
   my_thread_end();
   return NULL;
 }