Merge lp:~gl-az/percona-server/ST-31919-5.6 into lp:percona-server/5.6

[George Ormond Lorch III]

Fix for upstream 68354 and lp 1182572:
  Server crashes on update/join FEDERATED + local table when only 1 local row

This issue seems to be the result of a poor or changed assumption in the order
in which handler calls are made. In this specific scenario (a join with a local
table that has exactly one matching record) there is an assumption in the code
that a call to position will always come after a 'rnd_init/index_init' and
'rnd_next/index_read' but before 'rnd_end/index_end' is called.

The failing call order is:
  ha_federated::index_init
  ha_federated::index_read
  ha_federated::index_end
  ha_federated::position
  ha_federated::rnd_init
  ha_federated::extra
  ha_federated::rnd_pos

So here you can see position is getting called _outside_ of the scope of an
_init.._end table and/or index scan. This does indicate a change/problem in
the server but that is outside the scope of this fix.

As results are fetched, they are added to an interal list of result sets called
'results'. The last result set/current position is also held in the
'stored_result' value. If 'index_end' or 'rnd_end' is called before 'position'
has been called, the 'stored_result' is zeroed out and that result is removed
from the 'stored_results' list. When 'position' is called, a NULL result is
returned, which is then passed back through 'rnd_pos', which then dereferences
the NULL value and tries to manipulate some of the result internals, crashing.

So seems that the intention of the whole 'stored_result' was to have a place
to track all results allocated for the purposes of keeping them valid for the
'position' and 'rnd_pos' calls.

My approch to fixing this is to restructure this somewhan and instead of
tracking all results allocated in the 'results' list and popping them out when
they are discovered to not be needed, I went the other way. The results only
get addes to a storage list when it has been proven that they are in fact
needed later. So now for each new result set, it is temporarily stored in a
new value called 'last_result'. Whenever a new last_result needs created, it is
set by calling a new function 'set_last_result'. This function will check to
see if 'position' has been called on that result and if so, add it to the
(renamed) list 'stored_results' and clears the 'position_called' flag, else it
will free the 'last_result'. It then sets the new 'last_result' value.

Because of the out of scan scope call of position, we can no longer clear the
'stored_results' lists on '_end'. It will be cleared now when 'reset' is
called.

I have created a test case (suite/federated/federated_bug_68354.test) and
verified that it fails without the fix. Everything tests out under the federated
suite and the issue appears fixed.

The only downside is that a lengthy query could use a lot of memory while
hanging onto old 'position'ed results but I can't see how that can be safely
avoided in this particular handler.

[George Ormond Lorch III (gl-az)]

http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/147/

[Alexey Kopytov (akopytov)]

Same comments as in the 5.5 MP.

review: Needs Fixing

=== added file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.result'
--- Percona-Server/mysql-test/suite/federated/federated_bug_68354.result	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/federated/federated_bug_68354.result	2013-06-18 18:42:38 +0000
@@ -0,0 +1,80 @@
+CREATE DATABASE federated;
+CREATE DATABASE federated;
+SET @OLD_MASTER_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
+SET @@GLOBAL.CONCURRENT_INSERT= 0;
+SET @OLD_SLAVE_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
+SET @@GLOBAL.CONCURRENT_INSERT= 0;
+DROP TABLE IF EXISTS federated.t1;
+Warnings:
+Note	1051	Unknown table 'federated.t1'
+CREATE TABLE federated.t1 (
+`id` int(20) NOT NULL,
+`dummy` int(20) DEFAULT NULL,
+PRIMARY KEY (`id`))
+ENGINE="MyISAM"
+  DEFAULT CHARSET=latin1;
+INSERT INTO federated.t1 (`id`) VALUES
+(1001), (1002), (1003), (1004), (1005),
+(1006), (1007), (1008), (1009), (1010);
+CREATE TABLE federated.t1 (
+`id` int(20) NOT NULL,
+`dummy` int(20) DEFAULT NULL,
+PRIMARY KEY (`id`))
+ENGINE="FEDERATED"
+  DEFAULT CHARSET=latin1
+CONNECTION='mysql://root@127.0.0.1:SLAVE_PORT/federated/t1';
+CREATE TABLE federated.t2 (
+`id` int(20) NOT NULL,
+`dummy` int(20) NOT NULL,
+PRIMARY KEY (`id`))
+ENGINE="MyISAM"
+  DEFAULT CHARSET=latin1;
+INSERT INTO federated.t2 (`id`, `dummy`) VALUES (1005, 5);
+SELECT * FROM federated.t1;
+id	dummy
+1001	NULL
+1002	NULL
+1003	NULL
+1004	NULL
+1005	NULL
+1006	NULL
+1007	NULL
+1008	NULL
+1009	NULL
+1010	NULL
+UPDATE federated.t1 ft1 INNER JOIN federated.t2 ft2 ON ft1.id = ft2.id SET ft1.dummy = ft2.dummy WHERE ft1.dummy IS NULL;
+SELECT * FROM federated.t1;
+id	dummy
+1001	NULL
+1002	NULL
+1003	NULL
+1004	NULL
+1005	5
+1006	NULL
+1007	NULL
+1008	NULL
+1009	NULL
+1010	NULL
+DELETE FROM federated.t1 WHERE id = 1005;
+SELECT * FROM federated.t1 WHERE id = 1005;
+id	dummy
+SELECT * FROM federated.t1;
+id	dummy
+1001	NULL
+1002	NULL
+1003	NULL
+1004	NULL
+1006	NULL
+1007	NULL
+1008	NULL
+1009	NULL
+1010	NULL
+DROP TABLE federated.t2;
+DROP TABLE federated.t1;
+DROP TABLE federated.t1;
+SET @@GLOBAL.CONCURRENT_INSERT= @OLD_MASTER_CONCURRENT_INSERT;
+SET @@GLOBAL.CONCURRENT_INSERT= @OLD_SLAVE_CONCURRENT_INSERT;
+DROP TABLE IF EXISTS federated.t1;
+DROP DATABASE federated;
+DROP TABLE IF EXISTS federated.t1;
+DROP DATABASE federated;

=== added file 'Percona-Server/mysql-test/suite/federated/federated_bug_68354.test'
--- Percona-Server/mysql-test/suite/federated/federated_bug_68354.test	1970-01-01 00:00:00 +0000
+++ Percona-Server/mysql-test/suite/federated/federated_bug_68354.test	2013-06-18 18:42:38 +0000
@@ -0,0 +1,79 @@
+# Bug 1182572 68354: Server crashes on update/join FEDERATED + local table when 
+# only 1 local row. 
+# 
+# When updating a federated table with UPDATE ... JOIN, the server consistently 
+# crashes with Signal 11 when only 1 row exists in the local table involved in 
+# the join and that 1 row can be joined with a row in the federated table. 
+# 
+
+--source suite/federated/include/federated.inc
+
+connection default;
+
+# Disable concurrent inserts to avoid test failures when reading
+# data from concurrent connections (insert might return before
+# the data is actually in the table).
+SET @OLD_MASTER_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
+SET @@GLOBAL.CONCURRENT_INSERT= 0;
+
+connection slave;
+SET @OLD_SLAVE_CONCURRENT_INSERT= @@GLOBAL.CONCURRENT_INSERT;
+SET @@GLOBAL.CONCURRENT_INSERT= 0;
+DROP TABLE IF EXISTS federated.t1;
+CREATE TABLE federated.t1 (
+    `id` int(20) NOT NULL,
+    `dummy` int(20) DEFAULT NULL,
+    PRIMARY KEY (`id`))
+  ENGINE="MyISAM"
+  DEFAULT CHARSET=latin1;
+
+INSERT INTO federated.t1 (`id`) VALUES
+    (1001), (1002), (1003), (1004), (1005),
+    (1006), (1007), (1008), (1009), (1010);
+
+connection master;
+--replace_result $SLAVE_MYPORT SLAVE_PORT
+eval CREATE TABLE federated.t1 (
+    `id` int(20) NOT NULL,
+    `dummy` int(20) DEFAULT NULL,
+    PRIMARY KEY (`id`))
+  ENGINE="FEDERATED"
+  DEFAULT CHARSET=latin1
+  CONNECTION='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1';
+
+CREATE TABLE federated.t2 (
+    `id` int(20) NOT NULL,
+    `dummy` int(20) NOT NULL,
+    PRIMARY KEY (`id`))
+  ENGINE="MyISAM"
+  DEFAULT CHARSET=latin1;
+
+INSERT INTO federated.t2 (`id`, `dummy`) VALUES (1005, 5);
+
+SELECT * FROM federated.t1;
+
+# master would crash here before bug fix
+UPDATE federated.t1 ft1 INNER JOIN federated.t2 ft2 ON ft1.id = ft2.id SET ft1.dummy = ft2.dummy WHERE ft1.dummy IS NULL;
+
+# sanity test to make sure correct rows are being returned and server doesn't
+# also crash during DELETE
+SELECT * FROM federated.t1;
+DELETE FROM federated.t1 WHERE id = 1005;
+SELECT * FROM federated.t1 WHERE id = 1005;
+
+SELECT * FROM federated.t1;
+
+DROP TABLE federated.t2;
+DROP TABLE federated.t1;
+
+connection slave;
+DROP TABLE federated.t1;
+
+connection default;
+
+SET @@GLOBAL.CONCURRENT_INSERT= @OLD_MASTER_CONCURRENT_INSERT;
+connection slave;
+SET @@GLOBAL.CONCURRENT_INSERT= @OLD_SLAVE_CONCURRENT_INSERT;
+
+connection default;
+--source suite/federated/include/federated_cleanup.inc

=== modified file 'Percona-Server/storage/federated/ha_federated.cc'
--- Percona-Server/storage/federated/ha_federated.cc	2013-05-12 06:24:46 +0000
+++ Percona-Server/storage/federated/ha_federated.cc	2013-06-18 18:42:38 +0000
@@ -1,4 +1,4 @@
-/* Copyright (c) 2004, 2011, Oracle and/or its affiliates. All rights reserved.
+/* Copyright (c) 2004, 2012, Oracle and/or its affiliates. All rights reserved.
 
   This program is free software; you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
@@ -900,7 +900,7 @@
 ha_federated::ha_federated(handlerton *hton,
                            TABLE_SHARE *table_arg)
   :handler(hton, table_arg),
-  mysql(0), stored_result(0)
+  mysql(0), last_result(0), position_called(false)
 {
   trx_next= 0;
   memset(&bulk_insert, 0, sizeof(bulk_insert));
@@ -1653,7 +1653,7 @@
   ref_length= sizeof(MYSQL_RES *) + sizeof(MYSQL_ROW_OFFSET);
   DBUG_PRINT("info", ("ref_length: %u", ref_length));
 
-  my_init_dynamic_array(&results, sizeof(MYSQL_RES *), 4, 4);
+  my_init_dynamic_array(&stored_results, sizeof(MYSQL_RES *), 4, 4);
   reset();
 
   DBUG_RETURN(0);
@@ -1677,9 +1677,7 @@
 
   DBUG_ENTER("ha_federated::close");
 
-  free_result();
-  
-  delete_dynamic(&results);
+  delete_dynamic(&stored_results);
   
   /* Disconnect from mysql */
   mysql_close(mysql);
@@ -2326,13 +2324,18 @@
                              uint key_len, ha_rkey_function find_flag)
 {
   int rc;
+  MYSQL_RES* mysql_result;
+
   DBUG_ENTER("ha_federated::index_read");
 
   MYSQL_INDEX_READ_ROW_START(table_share->db.str, table_share->table_name.str);
-  free_result();
   rc= index_read_idx_with_result_set(buf, active_index, key,
                                      key_len, find_flag,
-                                     &stored_result);
+                                     &mysql_result);
+
+  if (rc == 0)
+    set_last_result(mysql_result);
+
   MYSQL_INDEX_READ_ROW_DONE(rc);
   DBUG_RETURN(rc);
 }
@@ -2363,7 +2366,6 @@
                                               &mysql_result)))
     DBUG_RETURN(retval);
   mysql_free_result(mysql_result);
-  results.elements--;
   DBUG_RETURN(0);
 }
 
@@ -2428,7 +2430,6 @@
   if ((retval= read_next(buf, *result)))
   {
     mysql_free_result(*result);
-    results.elements--;
     *result= 0;
     table->status= STATUS_NOT_FOUND;
     DBUG_RETURN(retval);
@@ -2480,6 +2481,7 @@
                                    bool eq_range_arg, bool sorted)
 {
   char sql_query_buffer[FEDERATED_QUERY_BUFFER_SIZE];
+  MYSQL_RES *mysql_result;
   int retval;
   String sql_query(sql_query_buffer,
                    sizeof(sql_query_buffer),
@@ -2501,13 +2503,15 @@
   }
   sql_query.length(0);
 
-  if (!(stored_result= store_result(mysql)))
+  if (!(mysql_result= store_result(mysql)))
   {
     retval= HA_ERR_END_OF_FILE;
     goto error;
   }
 
-  retval= read_next(table->record[0], stored_result);
+  set_last_result(mysql_result);
+
+  retval= read_next(table->record[0], mysql_result);
   MYSQL_INDEX_READ_ROW_DONE(retval);
   DBUG_RETURN(retval);
 
@@ -2536,7 +2540,7 @@
   DBUG_ENTER("ha_federated::index_next");
   MYSQL_INDEX_READ_ROW_START(table_share->db.str, table_share->table_name.str);
   ha_statistic_increment(&SSV::ha_read_next_count);
-  retval= read_next(buf, stored_result);
+  retval= read_next(buf, get_last_result());
   MYSQL_INDEX_READ_ROW_DONE(retval);
   DBUG_RETURN(retval);
 }
@@ -2557,6 +2561,7 @@
 
 int ha_federated::rnd_init(bool scan)
 {
+  MYSQL_RES *mysql_result;
   DBUG_ENTER("ha_federated::rnd_init");
   /*
     The use of the 'scan' flag is incredibly important for this handler
@@ -2596,8 +2601,9 @@
   if (scan)
   {
     if (real_query(share->select_query, strlen(share->select_query)) ||
-        !(stored_result= store_result(mysql)))
+        !(mysql_result= store_result(mysql)))
       DBUG_RETURN(stash_remote_error());
+    set_last_result(mysql_result);
   }
   DBUG_RETURN(0);
 }
@@ -2606,14 +2612,14 @@
 int ha_federated::rnd_end()
 {
   DBUG_ENTER("ha_federated::rnd_end");
-  DBUG_RETURN(index_end());
+  active_index= MAX_KEY;
+  DBUG_RETURN(0);
 }
 
 
 int ha_federated::index_end(void)
 {
   DBUG_ENTER("ha_federated::index_end");
-  free_result();
   active_index= MAX_KEY;
   DBUG_RETURN(0);
 }
@@ -2644,7 +2650,7 @@
 {
   DBUG_ENTER("ha_federated::rnd_next_int");
 
-  if (stored_result == 0)
+  if (get_last_result() == 0)
   {
     /*
       Return value of rnd_init is not always checked (see records.cc),
@@ -2653,7 +2659,7 @@
     */
     DBUG_RETURN(1);
   }
-  DBUG_RETURN(read_next(buf, stored_result));
+  DBUG_RETURN(read_next(buf, get_last_result()));
 }
 
 
@@ -2721,16 +2727,20 @@
 
 void ha_federated::position(const uchar *record __attribute__ ((unused)))
 {
+  MYSQL_RES* mysql_result;
   DBUG_ENTER("ha_federated::position");
-  
-  DBUG_ASSERT(stored_result);
-
-  position_called= TRUE;
+
+  mysql_result= get_last_result();
+
+  DBUG_ASSERT(mysql_result);
+
   /* Store result set address. */
-  memcpy(ref, &stored_result, sizeof(MYSQL_RES *));
+  memcpy(ref, &mysql_result, sizeof(MYSQL_RES *));
   /* Store data cursor position. */
   memcpy(ref + sizeof(MYSQL_RES *), &current_position,
                sizeof(MYSQL_ROW_OFFSET));
+
+  position_called= true;
   DBUG_VOID_RETURN;
 }
 
@@ -2966,14 +2976,20 @@
   replace_duplicates= FALSE;
 
   /* Free stored result sets. */
-  for (uint i= 0; i < results.elements; i++)
+  for (uint i= 0; i < stored_results.elements; i++)
   {
     MYSQL_RES *result;
-    get_dynamic(&results, (uchar *) &result, i);
+    get_dynamic(&stored_results, (uchar *) &result, i);
     mysql_free_result(result);
   }
-  reset_dynamic(&results);
+  reset_dynamic(&stored_results);
 
+  if (last_result)
+  {
+    mysql_free_result(last_result);
+    last_result= 0;
+  }
+  position_called= false;
   return 0;
 }
 
@@ -3261,31 +3277,28 @@
 
 MYSQL_RES *ha_federated::store_result(MYSQL *mysql_arg)
 {
-  MYSQL_RES *result= mysql_store_result(mysql_arg);
   DBUG_ENTER("ha_federated::store_result");
-  if (result)
-  {
-    (void) insert_dynamic(&results, &result);
-  }
-  position_called= FALSE;
-  DBUG_RETURN(result);
+  DBUG_RETURN(mysql_store_result(mysql_arg));
 }
 
 
-void ha_federated::free_result()
+void ha_federated::set_last_result(MYSQL_RES *result)
 {
-  DBUG_ENTER("ha_federated::free_result");
-  if (stored_result && !position_called)
-  {
-    mysql_free_result(stored_result);
-    stored_result= 0;
-    if (results.elements > 0)
-      results.elements--;
-  }
+  DBUG_ENTER("ha_federated::set_last_result");
+  if (position_called)
+  {
+    insert_dynamic(&stored_results, (uchar*) &last_result);
+    position_called= false;
+  }
+  else
+  {
+    mysql_free_result(last_result);
+  }
+  last_result= result;
   DBUG_VOID_RETURN;
 }
 
- 
+
 int ha_federated::external_lock(THD *thd, int lock_type)
 {
   int error= 0;

=== modified file 'Percona-Server/storage/federated/ha_federated.h'
--- Percona-Server/storage/federated/ha_federated.h	2012-08-22 01:40:20 +0000
+++ Percona-Server/storage/federated/ha_federated.h	2013-06-18 18:42:38 +0000
@@ -79,11 +79,19 @@
   THR_LOCK_DATA lock;      /* MySQL lock */
   FEDERATED_SHARE *share;    /* Shared lock info */
   MYSQL *mysql; /* MySQL connection */
-  MYSQL_RES *stored_result;
-  /**
-    Array of all stored results we get during a query execution.
-  */
-  DYNAMIC_ARRAY results;
+  /*
+    Last fetched result, freed if !position_called when set to next result.
+  */
+  MYSQL_RES *last_result;
+  /*
+    Array of stored results that exist because of position call.
+  */
+  DYNAMIC_ARRAY stored_results;
+  /*
+    Indicates if position has been called since last_result was set, used to
+    determine if last_result needs pushed into stored_results when
+    set_last_result is called.
+  */
   bool position_called;
   uint fetch_num; // stores the fetch num
   MYSQL_ROW_OFFSET current_position;  // Current position used by ::position()
@@ -258,8 +266,9 @@
   bool get_error_message(int error, String *buf);
   
   MYSQL_RES *store_result(MYSQL *mysql);
-  void free_result();
-  
+  void set_last_result(MYSQL_RES *result);
+  MYSQL_RES *get_last_result() const{ return last_result; }
+
   int external_lock(THD *thd, int lock_type);
   int connection_commit();
   int connection_rollback();