MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:antony Date:July 25 2007 6:18pm
Subject:bk commit into 5.0 tree (acurtis:1.2482) BUG#25679
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of antony. When antony does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2007-07-25 11:18:18-07:00, acurtis@stripped +4 -0
  Bug#25679
    "Federated Denial of Service"
    Federated storage engine used to attempt to open connections within
    the ::create() and ::open() methods which are invoked while LOCK_open
    mutex is being held by mysqld. As a result, no other client sessions
    can open tables while Federated is attempting to open a connection.
    Long DNS lookup times would stall mysqld's operation and a rogue
    connection string which connects to a remote server which simply
    stalls during handshake can stall mysqld for a much longer period of
    time.
    This patch moves the opening of the connection much later, when the
    federated actually issues queries, by which time the LOCK_open mutex is
    no longer being held.

  mysql-test/r/federated.result@stripped, 2007-07-25 11:18:07-07:00, acurtis@stripped +6 -5
    change of test/results due to patch for bug25679

  mysql-test/t/federated.test@stripped, 2007-07-25 11:18:07-07:00, acurtis@stripped +6 -3
    change of test/results due to patch for bug25679

  sql/ha_federated.cc@stripped, 2007-07-25 11:18:07-07:00, acurtis@stripped +144 -156
    bug25679
      remove function check_foreign_fata_source()
      ha_federated::open() no longer opens the federated connection.
      ha_federated::create() no longer opens and tests connection.
      ha_federated::real_connect() opens connection and tests presence of table.
      ha_federated::real_query() sends query, calling real_connect() if neccessary.

  sql/ha_federated.h@stripped, 2007-07-25 11:18:08-07:00, acurtis@stripped +2 -0
    bug25679
      new methods real_query() and real_connect()

diff -Nrup a/mysql-test/r/federated.result b/mysql-test/r/federated.result
--- a/mysql-test/r/federated.result	2007-06-28 13:36:17 -07:00
+++ b/mysql-test/r/federated.result	2007-07-25 11:18:07 -07:00
@@ -40,17 +40,18 @@ CREATE TABLE federated.t1 (
     )
 ENGINE="FEDERATED" DEFAULT CHARSET=latin1
 CONNECTION='mysql://root@stripped:SLAVE_PORT/federated/t3';
-ERROR HY000: Can't create federated table. Foreign data src error:  error: 1146  'Table 'federated.t3' doesn't exist'
+SELECT * FROM federated.t1;
+ERROR HY000: The foreign data source you are trying to reference does not exist. Data source error:  error: 1146  'Table 'federated.t3' doesn't exist'
+DROP TABLE federated.t1;
 CREATE TABLE federated.t1 (
 `id` int(20) NOT NULL,
 `name` varchar(32) NOT NULL default ''
     )
 ENGINE="FEDERATED" DEFAULT CHARSET=latin1
 CONNECTION='mysql://user:pass@stripped:SLAVE_PORT/federated/t1';
-ERROR HY000: Unable to connect to foreign data source: database: 'federated'  username: 'user'  hostname: '127.0.0.1'
-DROP TABLE IF EXISTS federated.t1;
-Warnings:
-Note	1051	Unknown table 't1'
+SELECT * FROM federated.t1;
+ERROR HY000: Unable to connect to foreign data source: Access denied for user 'user'@'localhost' (using password: YES)
+DROP TABLE federated.t1;
 CREATE TABLE federated.t1 (
 `id` int(20) NOT NULL,
 `name` varchar(32) NOT NULL default ''
diff -Nrup a/mysql-test/t/federated.test b/mysql-test/t/federated.test
--- a/mysql-test/t/federated.test	2007-06-28 13:36:17 -07:00
+++ b/mysql-test/t/federated.test	2007-07-25 11:18:07 -07:00
@@ -30,25 +30,28 @@ CREATE TABLE federated.t1 (
 
 # test non-existant table
 --replace_result $SLAVE_MYPORT SLAVE_PORT
---error 1434
 eval CREATE TABLE federated.t1 (
     `id` int(20) NOT NULL,
     `name` varchar(32) NOT NULL default ''
     )
   ENGINE="FEDERATED" DEFAULT CHARSET=latin1
   CONNECTION='mysql://root@stripped:$SLAVE_MYPORT/federated/t3';
+--error 1431
+SELECT * FROM federated.t1;
+DROP TABLE federated.t1;
 
 # test bad user/password 
 --replace_result $SLAVE_MYPORT SLAVE_PORT
---error 1429
 eval CREATE TABLE federated.t1 (
     `id` int(20) NOT NULL,
     `name` varchar(32) NOT NULL default ''
     )
   ENGINE="FEDERATED" DEFAULT CHARSET=latin1
   CONNECTION='mysql://user:pass@stripped:$SLAVE_MYPORT/federated/t1';
+--error 1429
+SELECT * FROM federated.t1;
+DROP TABLE federated.t1;
 
-DROP TABLE IF EXISTS federated.t1;
 # # correct connection, same named tables
 --replace_result $SLAVE_MYPORT SLAVE_PORT
 eval CREATE TABLE federated.t1 (
diff -Nrup a/sql/ha_federated.cc b/sql/ha_federated.cc
--- a/sql/ha_federated.cc	2007-07-23 23:35:40 -07:00
+++ b/sql/ha_federated.cc	2007-07-25 11:18:07 -07:00
@@ -497,105 +497,6 @@ err:
 }
 
 
-/*
- Check (in create) whether the tables exists, and that it can be connected to
-
-  SYNOPSIS
-    check_foreign_data_source()
-      share               pointer to FEDERATED share
-      table_create_flag   tells us that ::create is the caller, 
-                          therefore, return CANT_CREATE_FEDERATED_TABLE
-
-  DESCRIPTION
-    This method first checks that the connection information that parse url
-    has populated into the share will be sufficient to connect to the foreign
-    table, and if so, does the foreign table exist.
-*/
-
-static int check_foreign_data_source(FEDERATED_SHARE *share,
-                                     bool table_create_flag)
-{
-  char query_buffer[FEDERATED_QUERY_BUFFER_SIZE];
-  char error_buffer[FEDERATED_QUERY_BUFFER_SIZE];
-  uint error_code;
-  String query(query_buffer, sizeof(query_buffer), &my_charset_bin);
-  MYSQL *mysql;
-  DBUG_ENTER("ha_federated::check_foreign_data_source");
-
-  /* Zero the length, otherwise the string will have misc chars */
-  query.length(0);
-
-  /* error out if we can't alloc memory for mysql_init(NULL) (per Georg) */
-  if (!(mysql= mysql_init(NULL)))
-    DBUG_RETURN(HA_ERR_OUT_OF_MEM);
-  /* check if we can connect */
-  if (!mysql_real_connect(mysql,
-                          share->hostname,
-                          share->username,
-                          share->password,
-                          share->database,
-                          share->port,
-                          share->socket, 0))
-  {
-    /*
-      we want the correct error message, but it to return
-      ER_CANT_CREATE_FEDERATED_TABLE if called by ::create
-    */
-    error_code= (table_create_flag ?
-                 ER_CANT_CREATE_FEDERATED_TABLE :
-                 ER_CONNECT_TO_FOREIGN_DATA_SOURCE);
-
-    my_sprintf(error_buffer,
-               (error_buffer,
-                "database: '%s'  username: '%s'  hostname: '%s'",
-                share->database, share->username, share->hostname));
-
-    my_error(ER_CONNECT_TO_FOREIGN_DATA_SOURCE, MYF(0), error_buffer);
-    goto error;
-  }
-  else
-  {
-    /*
-      Since we do not support transactions at this version, we can let the 
-      client API silently reconnect. For future versions, we will need more 
-      logic to deal with transactions
-    */
-    mysql->reconnect= 1;
-    /*
-      Note: I am not using INORMATION_SCHEMA because this needs to work with 
-      versions prior to 5.0
-      
-      if we can connect, then make sure the table exists 
-
-      the query will be: SELECT * FROM `tablename` WHERE 1=0
-    */
-    query.append(FEDERATED_SELECT);
-    query.append(FEDERATED_STAR);
-    query.append(FEDERATED_FROM);
-    append_ident(&query, share->table_name, share->table_name_length,
-                 ident_quote_char);
-    query.append(FEDERATED_WHERE);
-    query.append(FEDERATED_FALSE);
-
-    if (mysql_real_query(mysql, query.ptr(), query.length()))
-    {
-      error_code= table_create_flag ?
-        ER_CANT_CREATE_FEDERATED_TABLE : ER_FOREIGN_DATA_SOURCE_DOESNT_EXIST;
-      my_sprintf(error_buffer, (error_buffer, "error: %d  '%s'",
-                                mysql_errno(mysql), mysql_error(mysql)));
-
-      my_error(error_code, MYF(0), error_buffer);
-      goto error;
-    }
-  }
-  error_code=0;
-
-error:
-    mysql_close(mysql);
-    DBUG_RETURN(error_code);
-}
-
-
 static int parse_url_error(FEDERATED_SHARE *share, TABLE *table, int error_num)
 {
   char buf[FEDERATED_QUERY_BUFFER_SIZE];
@@ -1478,36 +1379,7 @@ int ha_federated::open(const char *name,
     DBUG_RETURN(1);
   thr_lock_data_init(&share->lock, &lock, NULL);
 
-  /* Connect to foreign database mysql_real_connect() */
-  mysql= mysql_init(0);
-
-  /*
-    BUG# 17044 Federated Storage Engine is not UTF8 clean
-    Add set names to whatever charset the table is at open
-    of table
-  */
-  /* this sets the csname like 'set names utf8' */
-  mysql_options(mysql,MYSQL_SET_CHARSET_NAME,
-                this->table->s->table_charset->csname);
-
-  if (!mysql || !mysql_real_connect(mysql,
-                                   share->hostname,
-                                   share->username,
-                                   share->password,
-                                   share->database,
-                                   share->port,
-                                   share->socket, 0))
-  {
-    free_share(share);
-    DBUG_RETURN(stash_remote_error());
-  }
-  /*
-    Since we do not support transactions at this version, we can let the client
-    API silently reconnect. For future versions, we will need more logic to
-    deal with transactions
-  */
-
-  mysql->reconnect= 1;
+  DBUG_ASSERT(mysql == NULL);
 
   ref_length= (table->s->primary_key != MAX_KEY ?
                table->key_info[table->s->primary_key].key_length :
@@ -1543,8 +1415,8 @@ int ha_federated::close(void)
     stored_result= 0;
   }
   /* Disconnect from mysql */
-  if (mysql)                                    // QQ is this really needed
-    mysql_close(mysql);
+  mysql_close(mysql);
+  mysql= NULL;
   retval= free_share(share);
   DBUG_RETURN(retval);
 
@@ -1774,7 +1646,7 @@ int ha_federated::write_row(byte *buf)
     if (bulk_insert.length + values_string.length() + bulk_padding >
         mysql->net.max_packet_size && bulk_insert.length)
     {
-      error= mysql_real_query(mysql, bulk_insert.str, bulk_insert.length);
+      error= real_query(bulk_insert.str, bulk_insert.length);
       bulk_insert.length= 0;
     }
     else
@@ -1798,10 +1670,10 @@ int ha_federated::write_row(byte *buf)
   }  
   else
   {
-    error= mysql_real_query(mysql, values_string.ptr(), 
-                            values_string.length());
+    error= real_query(values_string.ptr(), values_string.length());
   }
-  
+
+err:  
   if (error)
   {
     DBUG_RETURN(stash_remote_error());
@@ -1837,6 +1709,8 @@ void ha_federated::start_bulk_insert(ha_
   DBUG_ENTER("ha_federated::start_bulk_insert");
 
   dynstr_free(&bulk_insert);
+
+  DBUG_PRINT("info",("rows = %lld", (long long) rows));
   
   /**
     We don't bother with bulk-insert semantics when the estimated rows == 1
@@ -1847,6 +1721,13 @@ void ha_federated::start_bulk_insert(ha_
   if (rows == 1)
     DBUG_VOID_RETURN;
 
+  /*
+    Make sure we have an open connection so that we know the 
+    maximum packet size.
+  */
+  if (!mysql && real_connect())
+    DBUG_VOID_RETURN;
+
   page_size= (uint) my_getpagesize();
 
   if (init_dynamic_string(&bulk_insert, NULL, page_size, page_size))
@@ -1875,7 +1756,7 @@ int ha_federated::end_bulk_insert()
   
   if (bulk_insert.str && bulk_insert.length)
   {
-    if (mysql_real_query(mysql, bulk_insert.str, bulk_insert.length))
+    if (real_query(bulk_insert.str, bulk_insert.length))
       error= stash_remote_error();
     else
     if (table->next_number_field)
@@ -1921,7 +1802,7 @@ int ha_federated::optimize(THD* thd, HA_
   append_ident(&query, share->table_name, share->table_name_length, 
                ident_quote_char);
 
-  if (mysql_real_query(mysql, query.ptr(), query.length()))
+  if (real_query(query.ptr(), query.length()))
   {
     DBUG_RETURN(stash_remote_error());
   }
@@ -1949,7 +1830,7 @@ int ha_federated::repair(THD* thd, HA_CH
   if (check_opt->sql_flags & TT_USEFRM)
     query.append(FEDERATED_USE_FRM);
 
-  if (mysql_real_query(mysql, query.ptr(), query.length()))
+  if (real_query(query.ptr(), query.length()))
   {
     DBUG_RETURN(stash_remote_error());
   }
@@ -2087,7 +1968,7 @@ int ha_federated::update_row(const byte 
   if (!has_a_primary_key)
     update_string.append(FEDERATED_LIMIT1);
 
-  if (mysql_real_query(mysql, update_string.ptr(), update_string.length()))
+  if (real_query(update_string.ptr(), update_string.length()))
   {
     DBUG_RETURN(stash_remote_error());
   }
@@ -2152,7 +2033,7 @@ int ha_federated::delete_row(const byte 
   delete_string.append(FEDERATED_LIMIT1);
   DBUG_PRINT("info",
              ("Delete sql: %s", delete_string.c_ptr_quick()));
-  if (mysql_real_query(mysql, delete_string.ptr(), delete_string.length()))
+  if (real_query(delete_string.ptr(), delete_string.length()))
   {
     DBUG_RETURN(stash_remote_error());
   }
@@ -2261,7 +2142,7 @@ int ha_federated::index_read_idx_with_re
                         NULL, 0);
   sql_query.append(index_string);
 
-  if (mysql_real_query(mysql, sql_query.ptr(), sql_query.length()))
+  if (real_query(sql_query.ptr(), sql_query.length()))
   {
     my_sprintf(error_buffer, (error_buffer, "error: %d '%s'",
                               mysql_errno(mysql), mysql_error(mysql)));
@@ -2327,7 +2208,7 @@ int ha_federated::read_range_first(const
     mysql_free_result(stored_result);
     stored_result= 0;
   }
-  if (mysql_real_query(mysql, sql_query.ptr(), sql_query.length()))
+  if (real_query(sql_query.ptr(), sql_query.length()))
   {
     retval= ER_QUERY_ON_FOREIGN_DATA_SOURCE;
     goto error;
@@ -2427,9 +2308,7 @@ int ha_federated::rnd_init(bool scan)
       stored_result= 0;
     }
 
-    if (mysql_real_query(mysql,
-                         share->select_query,
-                         strlen(share->select_query)))
+    if (real_query(share->select_query, strlen(share->select_query)))
       goto error;
 
     stored_result= mysql_store_result(mysql);
@@ -2646,8 +2525,7 @@ int ha_federated::info(uint flag)
     append_ident(&status_query_string, share->table_name,
                  share->table_name_length, value_quote_char);
 
-    if (mysql_real_query(mysql, status_query_string.ptr(),
-                         status_query_string.length()))
+    if (real_query(status_query_string.ptr(), status_query_string.length()))
       goto error;
 
     status_query_string.length(0);
@@ -2702,12 +2580,18 @@ int ha_federated::info(uint flag)
   DBUG_RETURN(0);
 
 error:
-  if (result)
-    mysql_free_result(result);
-
-  my_sprintf(error_buffer, (error_buffer, ": %d : %s",
-                            mysql_errno(mysql), mysql_error(mysql)));
-  my_error(error_code, MYF(0), error_buffer);
+  mysql_free_result(result);
+  if (mysql)
+  {
+    my_sprintf(error_buffer, (error_buffer, ": %d : %s",
+                              mysql_errno(mysql), mysql_error(mysql)));
+    my_error(error_code, MYF(0), error_buffer);
+  }
+  else
+  {
+    error_code= remote_error_number;
+    my_error(error_code, MYF(0), ER(error_code));
+  }
   DBUG_RETURN(error_code);
 }
 
@@ -2785,7 +2669,7 @@ int ha_federated::delete_all_rows()
   /*
     TRUNCATE won't return anything in mysql_affected_rows
   */
-  if (mysql_real_query(mysql, query.ptr(), query.length()))
+  if (real_query(query.ptr(), query.length()))
   {
     DBUG_RETURN(stash_remote_error());
   }
@@ -2874,8 +2758,7 @@ int ha_federated::create(const char *nam
   FEDERATED_SHARE tmp_share; // Only a temporary share, to test the url
   DBUG_ENTER("ha_federated::create");
 
-  if (!(retval= parse_url(&tmp_share, table_arg, 1)))
-    retval= check_foreign_data_source(&tmp_share, 1);
+  retval= parse_url(&tmp_share, table_arg, 1);
 
   my_free((gptr) tmp_share.scheme, MYF(MY_ALLOW_ZERO_PTR));
   DBUG_RETURN(retval);
@@ -2883,9 +2766,114 @@ int ha_federated::create(const char *nam
 }
 
 
+int ha_federated::real_connect()
+{
+  char buffer[FEDERATED_QUERY_BUFFER_SIZE];
+  String sql_query(buffer, sizeof(buffer), &my_charset_bin);
+  DBUG_ENTER("ha_federated::real_connect");
+
+  /* 
+    Bug#25679
+    Ensure that we do not hold the LOCK_open mutex while attempting
+    to establish Federated connection to guard against a trivial
+    Denial of Service scenerio.
+  */
+  safe_mutex_assert_not_owner(&LOCK_open);
+
+  DBUG_ASSERT(mysql == NULL);
+
+  if (!(mysql= mysql_init(NULL)))
+  {
+    remote_error_number= HA_ERR_OUT_OF_MEM;
+    DBUG_RETURN(-1);
+  }
+
+  /*
+    BUG# 17044 Federated Storage Engine is not UTF8 clean
+    Add set names to whatever charset the table is at open
+    of table
+  */
+  /* this sets the csname like 'set names utf8' */
+  mysql_options(mysql,MYSQL_SET_CHARSET_NAME,
+                this->table->s->table_charset->csname);
+
+  sql_query.length(0);
+
+  if (!mysql_real_connect(mysql,
+                          share->hostname,
+                          share->username,
+                          share->password,
+                          share->database,
+                          share->port,
+                          share->socket, 0))
+  {
+    stash_remote_error();
+    mysql_close(mysql);
+    mysql= NULL;
+    my_error(ER_CONNECT_TO_FOREIGN_DATA_SOURCE, MYF(0), remote_error_buf);
+    remote_error_number= -1;
+    DBUG_RETURN(-1);
+  }
+
+  /*
+    We have established a connection, lets try a simple dummy query just 
+    to check that the table and expected columns are present.
+  */
+  sql_query.append(share->select_query);
+  sql_query.append(FEDERATED_WHERE);
+  sql_query.append(FEDERATED_FALSE);
+  if (mysql_real_query(mysql, sql_query.ptr(), sql_query.length()))
+  {
+    sql_query.length(0);
+    sql_query.append("error: ");
+    sql_query.qs_append(mysql_errno(mysql));
+    sql_query.append("  '");
+    sql_query.append(mysql_error(mysql));
+    sql_query.append("'");
+    mysql_close(mysql);
+    mysql= NULL;
+    my_error(ER_FOREIGN_DATA_SOURCE_DOESNT_EXIST, MYF(0), sql_query.ptr());
+    remote_error_number= -1;
+    DBUG_RETURN(-1);
+  }
+
+  /* Just throw away the result, no rows anyways but need to keep in sync */
+  mysql_free_result(mysql_store_result(mysql));
+
+  /*
+    Since we do not support transactions at this version, we can let the client
+    API silently reconnect. For future versions, we will need more logic to
+    deal with transactions
+  */
+
+  mysql->reconnect= 1;
+  DBUG_RETURN(0);
+}
+
+
+int ha_federated::real_query(const char *query, uint length)
+{
+  int rc;
+  DBUG_ENTER("ha_federated::real_query");
+
+  if (!mysql && (rc= real_connect()))
+    goto end;
+
+  if (!query || !length)
+    goto end;
+
+  rc= mysql_real_query(mysql, query, length);
+  
+end:
+  DBUG_RETURN(rc);
+}
+
+
 int ha_federated::stash_remote_error()
 {
   DBUG_ENTER("ha_federated::stash_remote_error()");
+  if (!mysql)
+    DBUG_RETURN(remote_error_number);
   remote_error_number= mysql_errno(mysql);
   strmake(remote_error_buf, mysql_error(mysql), sizeof(remote_error_buf)-1);
   if (remote_error_number == ER_DUP_ENTRY ||
diff -Nrup a/sql/ha_federated.h b/sql/ha_federated.h
--- a/sql/ha_federated.h	2007-06-28 16:02:58 -07:00
+++ b/sql/ha_federated.h	2007-07-25 11:18:08 -07:00
@@ -182,6 +182,8 @@ private:
                                      uint key_len,
                                      ha_rkey_function find_flag,
                                      MYSQL_RES **result);
+  int real_query(const char *query, uint length);
+  int real_connect();
 public:
   ha_federated(TABLE *table_arg);
   ~ha_federated()
Thread
bk commit into 5.0 tree (acurtis:1.2482) BUG#25679antony25 Jul