Hi, Sanja! On Sep 19, sanja@askmonty.org wrote:

=== modified file 'sql/sql_acl.cc' --- a/sql/sql_acl.cc 2013-07-21 14:39:19 +0000 +++ b/sql/sql_acl.cc 2013-09-19 10:19:36 +0000 @@ -4615,7 +4615,8 @@ bool check_grant(THD *thd, ulong want_ac case ACL_INTERNAL_ACCESS_GRANTED: /* Currently, - - the information_schema does not subclass ACL_internal_table_access, + - the information_schema does not subclass ACL_internal_table_access +,

Eh? What was that?

there are no per table privilege checks for I_S, - the performance schema does use per tables checks, but at most returns 'CHECK_GRANT', and never 'ACCESS_GRANTED'. @@ -8222,6 +8223,43 @@ static bool find_mpvio_user(MPVIO_EXT *m mpvio->acl_user->plugin.str)); DBUG_RETURN(0); } + +static bool +read_client_connect_attrs(char **ptr, size_t *max_bytes_available, + const CHARSET_INFO *from_cs) +{ + size_t length, length_length; + char *ptr_save; + /* not enough bytes to hold the length */ + if (*max_bytes_available < 1) + return true; + + /* read the length */ + ptr_save= *ptr; + length= net_field_length_ll((uchar **) ptr); + length_length= *ptr - ptr_save; + if (*max_bytes_available < length_length) + return true;

This is incorrect, it might read past the end of the buffer. it's too late to check the length *after* you've read the data. Correct code might look like if (*max_bytes_available >= 9) { ptr_save= *ptr; length= net_field_length_ll((uchar **) ptr); length_length= *ptr - ptr_save; DBUG_ASSERT(length_length <= 9); *max_bytes_available-= length_length; } else { char buf[10], *len_ptr= buf; strncpy(buf, *ptr, 9) length= net_field_length_ll((uchar **) &len_ptr); length_length= len_ptr - buf; *ptr+= length_length; if (*max_bytes_available < length_length) return true; *max_bytes_available-= length_length; }

+ + *max_bytes_available-= length_length; + + /* length says there're more data than can fit into the packet */ + if (length > *max_bytes_available) + return true; + + /* impose an artificial length limit of 64k */ + if (length > 65535) + return true; + +#ifdef HAVE_PSI_THREAD_INTERFACE + if (PSI_THREAD_CALL(set_thread_connect_attrs)(*ptr, length, from_cs) && + current_thd->variables.log_warnings) + sql_print_warning("Connection attributes of length %lu were truncated", + (unsigned long) length); +#endif + return false; +} + #endif

/* the packet format is described in send_change_user_packet() */ @@ -8349,6 +8387,13 @@ static bool parse_com_change_user_packet } }

+ size_t bytes_remaining_in_packet= end - ptr;

1. This is very wrong, did you ever test it? You need a pointer to *after* the plugin name. But 'ptr' is two bytes *before* the plugin. 2. remove bytes_remaining_in_packet, pass 'end' instead to read_client_connect_attrs().

+ + if ((mpvio->thd->client_capabilities & CLIENT_CONNECT_ATTRS) && + read_client_connect_attrs(&ptr, &bytes_remaining_in_packet, + mpvio->thd->charset())) + return packet_error; + DBUG_PRINT("info", ("client_plugin=%s, restart", client_plugin)); /* Remember the data part of the packet, to present it to plugin in @@ -8487,7 +8532,21 @@ static ulong parse_client_handshake_pack /* strlen() can't be easily deleted without changing protocol */ db_len= db ? strlen(db) : 0;

- char *client_plugin= passwd + passwd_len + (db ? db_len + 1 : 0); + char *client_plugin= end= passwd + passwd_len + (db ? db_len + 1 : 0); + + if ((thd->client_capabilities & CLIENT_PLUGIN_AUTH) && + (client_plugin < (char *)net->read_pos + pkt_len)) + { + end+= strlen(end) + 1; + } + + size_t bytes_remaining_in_packet= (((char *)net->read_pos) + pkt_len) - end;

This is very-very wrong. if there're no connection attributes, then your 'end' will be after the end of the packet, it'll be after net->read_pos + pkt_len. But then you subtract, get -1 and cast it to unsigned size_t ! Don't worry, when you remove bytes_remaining_in_packet as I wrote above, this bug will go away.

+ + if (db && + (thd->client_capabilities & CLIENT_CONNECT_ATTRS) && + read_client_connect_attrs(&end, &bytes_remaining_in_packet, + mpvio->thd->charset())) + return packet_error;

Put it in the same place as in parse_com_change_user_packet, that is after reading the plugin name (and fix_plugin_ptr). And rename 'end' to 'connection_attributes'

/* Since 4.1 all database names are stored in utf8 */ if (db)

=== modified file 'sql/sys_vars.cc' --- a/sql/sys_vars.cc 2013-08-14 08:48:50 +0000 +++ b/sql/sys_vars.cc 2013-09-19 10:19:36 +0000 @@ -71,6 +71,10 @@ #define export /* not static */

#ifdef WITH_PERFSCHEMA_STORAGE_ENGINE +#ifndef EMBEDDED_LIBRARY +#define PFS_TRAILING_PROPERTIES \ + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(NULL), ON_UPDATE(NULL), \ + NULL, sys_var::PARSE_EARLY

Remove PFS_TRAILING_PROPERTIES define (not used in MariaDB)

static Sys_var_mybool Sys_pfs_enabled( "performance_schema", @@ -331,8 +335,10 @@ static Sys_var_long Sys_pfs_connect_attr GLOBAL_VAR(pfs_param.m_session_connect_attrs_sizing), CMD_LINE(REQUIRED_ARG), VALID_RANGE(-1, 1024 * 1024), DEFAULT(-1), - BLOCK_SIZE(1)); + BLOCK_SIZE(1), + NO_MUTEX_GUARD, NOT_IN_BINLOG);

Remove this change too (sysvars are NO_MUTEX_GUARD and NOT_IN_BINLOG by default)

+#endif /* EMBEDDED_LIBRARY */ #endif /* WITH_PERFSCHEMA_STORAGE_ENGINE */

static Sys_var_ulong Sys_auto_increment_increment(

=== modified file 'libmysqld/lib_sql.cc' --- a/libmysqld/lib_sql.cc 2013-08-01 12:09:26 +0000 +++ b/libmysqld/lib_sql.cc 2013-09-19 10:19:36 +0000 @@ -698,12 +698,38 @@ err: }

+static void +emb_transfer_connect_attrs(MYSQL *mysql) +{ +#ifdef HAVE_PSI_THREAD_INTERFACE + if (mysql->options.extension && + mysql->options.extension->connection_attributes_length) + { + uchar *buf, *ptr; + THD *thd= (THD*)mysql->thd; + size_t length= mysql->options.extension->connection_attributes_length; + + /* 9 = max length of the serialized length */ + ptr= buf= (uchar *) my_alloca(length + 9); + send_client_connect_attrs(mysql, buf); + net_field_length_ll(&ptr); + PSI_THREAD_CALL(set_thread_connect_attrs)((char *) ptr, length, thd->charset()); + my_afree(buf); + } +#endif +} + + #ifdef NO_EMBEDDED_ACCESS_CHECKS int check_embedded_connection(MYSQL *mysql, const char *db) { int result; LEX_STRING db_str = { (char*)db, db ? strlen(db) : 0 }; THD *thd= (THD*)mysql->thd; + + /* the server does the same as the client */ + mysql->server_capabilities= mysql->client_flag; +

Remove the similar line in init_embedded_mysql()

thd_init_client_charset(thd, mysql->charset->number); thd->update_charset(); Security_context *sctx= thd->security_ctx; @@ -728,11 +755,17 @@ int check_embedded_connection(MYSQL *mys we emulate a COM_CHANGE_USER user here, it's easier than to emulate the complete 3-way handshake */ - char buf[USERNAME_LENGTH + SCRAMBLE_LENGTH + 1 + 2*NAME_LEN + 2], *end; + char *buf, *end; NET *net= &mysql->net; THD *thd= (THD*)mysql->thd; Security_context *sctx= thd->security_ctx; + size_t connect_attrs_len= + (mysql->server_capabilities & CLIENT_CONNECT_ATTRS &&

This doesn't seem to make any sense. Why does it check for CLIENT_CONNECT_ATTRS if it's part of the server (embedded). If a client connects to the server, then the client needs to check whether the server supports CLIENT_CONNECT_ATTRS, and the server needs to check whether the client supports it. But a server doesn't need to check whether it itself support it, it knows it already :)

+ mysql->options.extension) ? + mysql->options.extension->connection_attributes_length : 0;

+ buf= my_alloca(USERNAME_LENGTH + SCRAMBLE_LENGTH + 1 + 2*NAME_LEN + 2 + + connect_attrs_len + 2); if (mysql->options.client_ip) { sctx->host= my_strdup(mysql->options.client_ip, MYF(0)); @@ -766,6 +799,13 @@ int check_embedded_connection(MYSQL *mys int2store(end, (ushort) mysql->charset->number); end+= 2;

+ //end= strmake(end, "mysql_native_password", NAME_LEN) + 1;

Why is it commented out?

+ + /* the server does the same as the client */ + mysql->server_capabilities= mysql->client_flag; + + end= (char *) send_client_connect_attrs(mysql, (uchar *) end); + /* acl_authenticate() takes the data from thd->net->read_pos */ thd->net.read_pos= (uchar*)buf;

Regards, Sergei