maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sanja!

On Sep 19, sanja@xxxxxxxxxxxx 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