List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:January 31 2011 11:16am
Subject:Re: bzr commit into mysql-5.1 branch (alexander.barkov:3565)
Bug#58036
View as plain text  
Alexander,

On Mon, Jan 31, 2011 at 12:35:31PM +0300, Alexander Barkov wrote:
> Hi Sergey,
> 
> Sergey Vojtovich wrote:
> >Hi Alexander,
> >
> >the patch looks nice, but I have two questions...
> >
> >Do you think it is worth to restore old connection character set if
> >check_user() of COM_CHANGE_USER failed?
> 
> I don't think we should do it.
Usually I expect failing functions to restore context. Or at least
have it documented, that certain things are not restored.

In our case it makes slightly more complex to restore from failing
change user attempt. An application needs additionally restore
character set.

What are reasons for not restoring the context?

> 
> 
> >
> >Manual for mysql_real_connect() says:
> >The user and passwd  parameters use whatever character set has been
> >configured for the MYSQL object. By default, this is latin1, but can
> >be changed by calling mysql_options(mysql, MYSQL_SET_CHARSET_NAME,
> >"charset_name") prior to connecting.
> >
> >However it doesn't state if the same rule applies to db,
> 
> Thanks for noticing this!
> Manual should be updated to state the same for db.
> All input from client must be considered as having
> the same character set.
> 
> (
> The only exception I can think of are literals with
> character set introducers, e.g.:
> SET NAMES latin1;
> SELECT 'xxx', _latin2'yyy';
> ).
> 
> 
> >as well
> >as there is no indication that this rule applies to mysql_change_user().
> >
> > If I understand correctly, prior to this patch mysql_change_user()
> > used old connection character set for user, passwd and db.
> 
> True. I'd say this was a bug too, as this contradicted with the manual:
> 
> http://dev.mysql.com/doc/refman/5.0/en/mysql-change-user.html
> says that mysql_change_user()
> "resets the state as if one had done a new connect."
> 
> Therefore,
> 
>   mysql_options(mysql, MYSQL_SET_CHARSET_NAME, "charsetname");
>   mysql_change_user(mysql, user, passwd, db);
> 
> should do effectively the same with:
> 
>   mysql_close(mysql);
>   mysql= mysql_init(NULL);
>   mysql_options(mysql, MYSQL_SET_CHARSET_NAME, "charsetname");
>   mysql_real_connect(mysql, user, passwd, db, ...);
> 
> which means use NEW character set for user, passwd and db.
> 
> 
> >With this patch it uses old connection character set for db,
> >but new connection character set for user and passwd.
> >It looks inconsistent and seem to be change in behaviour.
> 
> Thanks for good catch! I seems thd_init_client_charset()
> should be moved even earlier, before this call:
> 
>    /* Convert database name to utf8 */
>     db_buff[copy_and_convert(db_buff, sizeof(db_buff)-1,
>                              system_charset_info, db, db_length,
>                              thd->charset(), &dummy_errors)]= 0;
> 
> I will send a new patch if you agree.
Yes, I agree.

Regards,
Sergey

> >
> >What character set do we expect for user, passwd and db?
> 
> 
> All three parameters (user, passwd, db) should be considered
> to be in the same character set, given in mysql_options().
> 
> I'll ask Paul to check what can be added in the manual
> to make this clearer stated.
> 
> >
> >Regards,
> >Sergey
> >
> >On Fri, Jan 28, 2011 at 11:50:17AM -0000, Alexander Barkov wrote:
> >>#At file:///home/bar/mysql-bzr/mysql-5.1.b58036/ based on
> revid:anders.song@stripped
> >>
> >> 3565 Alexander Barkov	2011-01-28
> >>      Bug#58036 	client utf32, utf16, ucs2 should be disallowed, they crash
> server
> >>      Problem: ucs2 was correctly disallowed in "SET NAMES" only,
> >>      while mysql_real_connect() and mysql_change_user() still allowed
> >>      to use ucs2, which made server crash.
> >>      Fix: disallow ucs2 in mysql_real_connect() and mysql_change_user().
> >>        @ sql/mysql_priv.h
> >>          - introducing a new function, to reuse in all places where we need
> >>          to check client character set.
> >>          - changing return type for thd_init_client_charset() to bool,
> >>            to return errors to the caller
> >>        @ sql/set_var.cc
> >>          Using new function.
> >>        @ sql/sql_connect.cc
> >>          - in case of unsupported client character set send error and return
> true
> >>          - moving thd->update_charset() inside
> thd_init_client_charset(),
> >>            as they are always called in pair.
> >>          - in case of success return false
> >>        @ sql/sql_parse.cc
> >>          - moving thd_init_client_charset() before check_user(),
> >>           because DBUG_ASSERT forbids calling my_error()
> >>after check_user() has authenticated the user and has done
> >>my_ok().
> >>          - removing thd->update_charset(), as it's now done in
> >>            thd_init_client_charset().
> >>        @ tests/mysql_client_test.c
> >>          Adding test
> >>
> >>    modified:
> >>      sql/mysql_priv.h
> >>      sql/set_var.cc
> >>      sql/sql_connect.cc
> >>      sql/sql_parse.cc
> >>      tests/mysql_client_test.c
> >>=== modified file 'sql/mysql_priv.h'
> >>--- a/sql/mysql_priv.h	2011-01-15 05:51:41 +0000
> >>+++ b/sql/mysql_priv.h	2011-01-28 11:28:05 +0000
> >>@@ -1019,7 +1019,7 @@ void reset_mqh(LEX_USER *lu, bool get_th
> >> bool check_mqh(THD *thd, uint check_command);
> >> void time_out_user_resource_limits(THD *thd, USER_CONN *uc);
> >> void decrease_user_connections(USER_CONN *uc);
> >>-void thd_init_client_charset(THD *thd, uint cs_number);
> >>+bool thd_init_client_charset(THD *thd, uint cs_number);
> >> bool setup_connection_thread_globals(THD *thd);
> >> int mysql_create_db(THD *thd, char *db, HA_CREATE_INFO *create, bool
> silent);
> >>@@ -2536,6 +2536,10 @@ bool load_collation(MEM_ROOT *mem_root,
> >>                     CHARSET_INFO *dflt_cl,
> >>                     CHARSET_INFO **cl);
> >>+inline bool charset_is_good_for_parser(CHARSET_INFO *cs)
> >>+{ return test(cs->mbminlen == 1); }
> >>+
> >>+
> >> #endif /* MYSQL_SERVER */
> >> extern "C" int test_if_data_home_dir(const char *dir);
> >>
> >>=== modified file 'sql/set_var.cc'
> >>--- a/sql/set_var.cc	2010-12-28 23:47:05 +0000
> >>+++ b/sql/set_var.cc	2011-01-28 11:28:05 +0000
> >>@@ -2187,7 +2187,7 @@ bool sys_var_character_set_client::check
> >>   if (sys_var_character_set_sv::check(thd, var))
> >>     return 1;
> >>   /* Currently, UCS-2 cannot be used as a client character set */
> >>-  if (var->save_result.charset->mbminlen > 1)
> >>+  if (!charset_is_good_for_parser(var->save_result.charset))
> >>   {
> >>     my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), name,
> >>var->save_result.charset->csname);
> >>
> >>=== modified file 'sql/sql_connect.cc'
> >>--- a/sql/sql_connect.cc	2010-11-11 07:34:14 +0000
> >>+++ b/sql/sql_connect.cc	2011-01-28 11:28:05 +0000
> >>@@ -582,8 +582,23 @@ void reset_mqh(LEX_USER *lu, bool get_th
> >> }
> >>-void thd_init_client_charset(THD *thd, uint cs_number)
> >>+/**
> >>+  Set thread character set variables from the given ID
> >>+
> >>+  @param  thd         thread handle
> >>+  @param  cs_number   character set and collation ID
> >>+
> >>+  @retval  0  OK; character_set_client, collation_connection and
> >>+              character_set_results are set to the new value,
> >>+              or to the default global values.
> >>+
> >>+  @retval  1  error, e.g. the given ID is not supported by parser.
> >>+              Corresponding SQL error is sent.
> >>+*/
> >>+
> >>+bool thd_init_client_charset(THD *thd, uint cs_number)
> >> {
> >>+  CHARSET_INFO *cs;
> >>   /*
> >>    Use server character set and collation if
> >>    - opt_character_set_client_handshake is not set
> >>@@ -592,10 +607,10 @@ void thd_init_client_charset(THD *thd, u
> >>    - client character set doesn't exists in server
> >>   */
> >>   if (!opt_character_set_client_handshake ||
> >>-      !(thd->variables.character_set_client= get_charset(cs_number,
> MYF(0))) ||
> >>+      !(cs= get_charset(cs_number, MYF(0))) ||
> >>       !my_strcasecmp(&my_charset_latin1,
> >>                      global_system_variables.character_set_client->name,
> >>-                     thd->variables.character_set_client->name))
> >>+                     cs->name))
> >>   {
> >>     thd->variables.character_set_client=
> >>       global_system_variables.character_set_client;
> >>@@ -606,10 +621,19 @@ void thd_init_client_charset(THD *thd, u
> >>   }
> >>   else
> >>   {
> >>+    if (!charset_is_good_for_parser(cs))
> >>+    {
> >>+      /* Disallow non-supported parser character sets: UCS2, UTF16, UTF32
> */
> >>+      my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0), "character_set_client",
> >>+               cs->csname);
> >>+      return true;
> >>+    }
> >>     thd->variables.character_set_results=
> >>       thd->variables.collation_connection= -
> >>thd->variables.character_set_client;
> >>+      thd->variables.character_set_client= cs;
> >>   }
> >>+  thd->update_charset();
> >>+  return false;
> >> }
> >>@@ -782,8 +806,8 @@ static int check_connection(THD *thd)
> >>     thd->client_capabilities|= ((ulong) uint2korr(net->read_pos+2))
> << 16;
> >>     thd->max_client_packet_length= uint4korr(net->read_pos+4);
> >>     DBUG_PRINT("info", ("client_character_set: %d", (uint)
> net->read_pos[8]));
> >>-    thd_init_client_charset(thd, (uint) net->read_pos[8]);
> >>-    thd->update_charset();
> >>+    if (thd_init_client_charset(thd, (uint) net->read_pos[8]))
> >>+      return 1;
> >>     end= (char*) net->read_pos+32;
> >>   }
> >>   else
> >>
> >>=== modified file 'sql/sql_parse.cc'
> >>--- a/sql/sql_parse.cc	2011-01-15 05:51:41 +0000
> >>+++ b/sql/sql_parse.cc	2011-01-28 11:28:05 +0000
> >>@@ -1183,8 +1183,18 @@ bool dispatch_command(enum enum_server_c
> >>     /* Clear variables that are allocated */
> >>     thd->user_connect= 0;
> >>     thd->security_ctx->priv_user= thd->security_ctx->user;
> >>+    if (cs_number)
> >>+    {
> >>+      if (thd_init_client_charset(thd, cs_number))
> >>+      {
> >>+        res= true;
> >>+        goto com_change_user_finalize;
> >>+      }
> >>+    }
> >>     res= check_user(thd, COM_CHANGE_USER, passwd, passwd_len, db, FALSE);
> >>+com_change_user_finalize:
> >>+
> >>     if (res)
> >>     {
> >>       x_free(thd->security_ctx->user);
> >>@@ -1202,12 +1212,6 @@ bool dispatch_command(enum enum_server_c
> >> #endif /* NO_EMBEDDED_ACCESS_CHECKS */
> >>       x_free(save_db);
> >>       x_free(save_security_ctx.user);
> >>-
> >>-      if (cs_number)
> >>-      {
> >>-        thd_init_client_charset(thd, cs_number);
> >>-        thd->update_charset();
> >>-      }
> >>     }
> >>     break;
> >>   }
> >>
> >>=== modified file 'tests/mysql_client_test.c'
> >>--- a/tests/mysql_client_test.c	2010-12-28 23:47:05 +0000
> >>+++ b/tests/mysql_client_test.c	2011-01-28 11:28:05 +0000
> >>@@ -18399,6 +18399,71 @@ static void test_bug47485()
> >> /*
> >>+  Bug#58036 client utf32, utf16, ucs2 should be disallowed, they crash
> server
> >>+*/
> >>+static void test_bug58036()
> >>+{
> >>+  MYSQL *conn;
> >>+  DBUG_ENTER("test_bug47485");
> >>+  myheader("test_bug58036");
> >>+
> >>+  /* Part1: try to connect with ucs2 client character set */
> >>+  conn= mysql_client_init(NULL);
> >>+  mysql_options(conn, MYSQL_SET_CHARSET_NAME, "ucs2");
> >>+  if (mysql_real_connect(conn, opt_host, opt_user,
> >>+                         opt_password,  opt_db ? opt_db : "test",
> >>+                         opt_port, opt_unix_socket, 0))
> >>+  {
> >>+    if (!opt_silent)
> >>+      printf("mysql_real_connect() succeeded (failure expected)\n");
> >>+    mysql_close(conn);
> >>+    DIE();
> >>+  }
> >>+
> >>+  if (!opt_silent)
> >>+    printf("Got mysql_real_connect() error (expected): %s (%d)\n",
> >>+           mysql_error(conn), mysql_errno(conn));  +
> >>DIE_UNLESS(mysql_errno(conn) == ER_WRONG_VALUE_FOR_VAR);
> >>+  mysql_close(conn);
> >>+
> >>+
> >>+  /*
> >>+    Part2:
> >>+    - connect with latin1
> >>+    - then change client character set to ucs2
> >>+    - then try mysql_change_user()
> >>+  */
> >>+  conn= mysql_client_init(NULL);
> >>+  mysql_options(conn, MYSQL_SET_CHARSET_NAME, "latin1");
> >>+  if (!mysql_real_connect(conn, opt_host, opt_user,
> >>+                         opt_password, opt_db ? opt_db : "test",
> >>+                         opt_port, opt_unix_socket, 0))
> >>+  {
> >>+    if (!opt_silent)
> >>+      printf("mysql_real_connect() failed: %s (%d)\n",
> >>+             mysql_error(conn), mysql_errno(conn));
> >>+    mysql_close(conn);
> >>+    DIE();
> >>+  }
> >>+
> >>+  mysql_options(conn, MYSQL_SET_CHARSET_NAME, "ucs2");
> >>+  if (!mysql_change_user(conn, opt_user, opt_password, NULL))
> >>+  {
> >>+    if (!opt_silent)
> >>+      printf("mysql_change_user() succedded, error expected!");
> >>+    mysql_close(conn);
> >>+    DIE();
> >>+  }
> >>+
> >>+  if (!opt_silent)
> >>+    printf("Got mysql_change_user() error (expected): %s (%d)\n",
> >>+           mysql_error(conn), mysql_errno(conn));
> >>+  mysql_close(conn);
> >>+
> >>+  DBUG_VOID_RETURN;
> >>+}
> >>+
> >>+/*
> >>   Read and parse arguments and MySQL options from my.cnf
> >> */
> >>@@ -18724,6 +18789,7 @@ static struct my_tests_st my_tests[]= {
> >>   { "test_bug42373", test_bug42373 },
> >>   { "test_bug54041", test_bug54041 },
> >>   { "test_bug47485", test_bug47485 },
> >>+  { "test_bug58036", test_bug58036 },
> >>   { 0, 0 }
> >> };
> >>
> >
> >
> >>-- 
> >>MySQL Code Commits Mailing List
> >>For list archives: http://lists.mysql.com/commits
> >>To unsubscribe:    http://lists.mysql.com/commits?unsub=1
> >
> >
> 

-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-5.1 branch (alexander.barkov:3565) Bug#58036Alexander Barkov28 Jan
  • Re: bzr commit into mysql-5.1 branch (alexander.barkov:3565)Bug#58036Sergey Vojtovich31 Jan
    • Re: bzr commit into mysql-5.1 branch (alexander.barkov:3565) Bug#58036Alexander Barkov31 Jan
      • Re: bzr commit into mysql-5.1 branch (alexander.barkov:3565)Bug#58036Sergey Vojtovich31 Jan