List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:November 12 2010 5:40pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306
View as plain text  
Hello,

Alexander Barkov a écrit, Le 12.11.2010 15:45:
> #At file:///home/bar/mysql-bzr/mysql-5.5-bugteam.b57306/ based on
> revid:vvaintroub@stripped
> 
>  3121 Alexander Barkov	2010-11-12
>       Bug#57306 SHOW PROCESSLIST does not display string literals well.
>       
>       Problem: Extended characters outside of ASCII range where not displayed
>       properly in SHOW PROCESSLIST, because thd_info->query was always sent as 
>       system_character_set (utf8). This was wrong, because query buffer
>       is never converted to utf8 - it is always have client character set.
>       
>       Fix: sending query buffer using character_set_client of its thread.
> 
>     modified:
>       mysql-test/r/show_check.result
>       mysql-test/t/show_check.test
>       sql/sql_show.cc

> === modified file 'mysql-test/t/show_check.test'
> --- a/mysql-test/t/show_check.test	2010-07-27 14:32:42 +0000
> +++ b/mysql-test/t/show_check.test	2010-11-12 14:45:32 +0000
> @@ -1328,3 +1328,24 @@ disconnect con1;
>  # Wait till all disconnects are completed
>  --source include/wait_until_count_sessions.inc
>  
> +--echo #
> +--echo # Bug#57306 SHOW PROCESSLIST does not display string literals well.
> +--echo #
> +
> +SET NAMES latin1;
> +SELECT GET_LOCK('t', 1000);
> +--connect (con1,localhost,root,,)
> +--connection con1
> +SET NAMES latin1;
> +--send SELECT GET_LOCK('t',1000) AS 'óóóó';
> +--connection default
> +--replace_column 1 ### 6 ### 7 ###
> +SHOW PROCESSLIST;
> +SET NAMES utf8;
> +--replace_column 1 ### 6 ### 7 ###
> +SHOW PROCESSLIST;
> +SELECT RELEASE_LOCK('t');
> +--connection con1
> +--reap
> +--disconnect con1
> +--connection default

The default connection did "SET NAMES latin1" and then "SET NAMES utf8". 
I guess this last command restores the connection's state to what it was 
before "SET NAMES latin1"?

> === modified file 'sql/sql_show.cc'
> --- a/sql/sql_show.cc	2010-11-02 14:02:16 +0000
> +++ b/sql/sql_show.cc	2010-11-12 14:45:32 +0000
> @@ -1728,6 +1728,7 @@ public:
>    uint   command;
>    const char *user,*host,*db,*proc_info,*state_info;
>    char *query;
> +  CHARSET_INFO *character_set_client;
>  };
>  
>  #ifdef HAVE_EXPLICIT_TEMPLATE_INSTANTIATION
> @@ -1833,6 +1834,7 @@ void mysqld_list_processes(THD *thd,cons
>          }
>          mysql_mutex_unlock(&tmp->LOCK_thd_data);
>          thd_info->start_time= tmp->start_time;
> +        thd_info->character_set_client= tmp->variables.character_set_client;

This is an "unprotected" read of another thread's data 
(tmp->variables.character_set_client). Imagine that this other thread is 
precisely in the process of changing its @@character_set_client: we may 
be reading a pointer value while it's changing, which may in theory give 
garbage (some bytes of the pointer have changed and some not yet); 
imagine this garbage pointer is an invalid pointer (not pointing to any 
charset); then when we print the query in SHOW PROCESSLIST, we'll 
dereference this invalid pointer and crash?
I read somewhere that on modern Intel CPUs, 64-bit reads are atomic, but 
it's always unclear what that exactly means and what happens for other 
CPUs...
So I don't feel very confident about this. Or is there no problem?

I see the query is read for LOCK_thd_data.

If there is a problem, maybe we should change the code which updates 
THD::variables::character_set_client to use LOCK_thd_data, or an atomic 
operation; and above we would use the mutex or the atomic operation...?

>          thread_infos.append(thd_info);
>        }
>      }
> @@ -1857,7 +1859,7 @@ void mysqld_list_processes(THD *thd,cons
>      else
>        protocol->store_null();
>      protocol->store(thd_info->state_info, system_charset_info);
> -    protocol->store(thd_info->query, system_charset_info);
> +    protocol->store(thd_info->query, thd_info->character_set_client);
>      if (protocol->write())
>        break; /* purecov: inspected */
>    }
Thread
bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Alexander Barkov12 Nov
  • Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Guilhem Bichot12 Nov
    • Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Alexander Barkov12 Nov
      • Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Tor Didriksen15 Nov
        • Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Alexander Barkov15 Nov
          • Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Alexander Barkov15 Nov
            • Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Guilhem Bichot15 Nov
              • Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Alexander Barkov16 Nov
      • Re: bzr commit into mysql-5.5-bugteam branch (bar:3121) Bug#57306Guilhem Bichot15 Nov