Hi Luis,
Patch conditionally approved.
The patch looks good. However, I was wondering if the fact that some commands call my_ok
and others do not is not a bug.
Could you explain why this happens?
Cheers.
On 06/21/2010 01:05 AM, Luis Soares wrote:
> #At file:///home/lsoares/Workspace/bzr/work/bugfixing/54197/mysql-5.1-bugteam/ based
> on revid:georgi.kodinov@stripped
>
> 3423 Luis Soares 2010-06-21
> BUG#54197: 'BINLOG ' statements crash the server
>
> Some BINLOG statements would make the server hit an assertion,
> because they would lead to setting thd->main_da.m_status
> twice. This was caused because the execution of some BINLOG
> statements (in base 64 output) make my_ok to be called before
> returning from ev->apply_event. Then, my_ok would be called
> again, right before returning from mysql_client_binlog_statement,
> triggering the assertion.
>
> We fix this by:
>
> 1. moving the call to my_ok from inside
> mysql_client_binlog_statement function, to
> mysql_execute_command
>
> 2. changing the signature of mysql_client_binlog_statement so
> that it returns non-zero on error and zero otherwise.
>
> 3. calling my_ok in mysql_execute_command conditionally, ie, if
> the status hasn't been set yet.
>
> modified:
> mysql-test/r/mysqlbinlog_base64.result
> mysql-test/t/mysqlbinlog_base64.test
> sql/mysql_priv.h
> sql/sql_binlog.cc
> sql/sql_parse.cc
> === modified file 'mysql-test/r/mysqlbinlog_base64.result'
> --- a/mysql-test/r/mysqlbinlog_base64.result 2008-07-28 07:15:20 +0000
> +++ b/mysql-test/r/mysqlbinlog_base64.result 2010-06-21 00:05:45 +0000
> @@ -109,3 +109,9 @@ count(*)
> 35840
> drop table t1;
> drop table t2;
> +RESET MASTER;
> +CREATE TABLE t1(c1 INT) ENGINE=myisam;
> +FLUSH LOGS;
> +INSERT INTO t1 VALUES(1),(2);
> +DROP TABLE t1;
> +DROP TABLE t1;
>
> === modified file 'mysql-test/t/mysqlbinlog_base64.test'
> --- a/mysql-test/t/mysqlbinlog_base64.test 2008-07-28 07:15:20 +0000
> +++ b/mysql-test/t/mysqlbinlog_base64.test 2010-06-21 00:05:45 +0000
> @@ -71,3 +71,23 @@ select count(*) from t2;
> --remove_file $MYSQLTEST_VARDIR/tmp/mysqlbinlog_base64.sql
> drop table t1;
> drop table t2;
> +
> +#
> +# BUG#54197 'BINLOG ' statements crash the server
> +#
> +
> +RESET MASTER;
> +
> +CREATE TABLE t1(c1 INT) ENGINE=myisam;
> +FLUSH LOGS;
> +INSERT INTO t1 VALUES(1),(2);
> +
> +-- let $fname_prefix= `SELECT UUID()`;
> +-- let $outfile= $MYSQL_TEST_DIR/var/tmp/$fname_prefix.sql
> +
> +--let $MYSQLD_DATADIR=`select @@datadir`
> +--exec $MYSQL_BINLOG --base64-output $MYSQLD_DATADIR/master-bin.000001>
> $outfile
> +DROP TABLE t1;
> +--exec $MYSQL test< $outfile
> +-- remove_file $outfile
> +DROP TABLE t1;
>
> === modified file 'sql/mysql_priv.h'
> --- a/sql/mysql_priv.h 2010-05-27 20:07:40 +0000
> +++ b/sql/mysql_priv.h 2010-06-21 00:05:45 +0000
> @@ -1045,7 +1045,7 @@ bool mysql_alter_db(THD *thd, const char
> bool mysql_rm_db(THD *thd,char *db,bool if_exists, bool silent);
> bool mysql_upgrade_db(THD *thd, LEX_STRING *old_db);
> void mysql_binlog_send(THD* thd, char* log_ident, my_off_t pos, ushort flags);
> -void mysql_client_binlog_statement(THD *thd);
> +int mysql_client_binlog_statement(THD *thd);
> bool mysql_rm_table(THD *thd,TABLE_LIST *tables, my_bool if_exists,
> my_bool drop_temporary);
> int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
>
> === modified file 'sql/sql_binlog.cc'
> --- a/sql/sql_binlog.cc 2009-10-14 01:39:05 +0000
> +++ b/sql/sql_binlog.cc 2010-06-21 00:05:45 +0000
> @@ -31,7 +31,7 @@
> statement.
> */
>
> -void mysql_client_binlog_statement(THD* thd)
> +int mysql_client_binlog_statement(THD* thd)
> {
> DBUG_ENTER("mysql_client_binlog_statement");
> DBUG_PRINT("info",("binlog base64: '%*s'",
> @@ -40,7 +40,7 @@ void mysql_client_binlog_statement(THD*
> thd->lex->comment.str));
>
> if (check_global_access(thd, SUPER_ACL))
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(1);
>
> size_t coded_len= thd->lex->comment.length + 1;
> size_t decoded_len= base64_needed_decoded_length(coded_len);
> @@ -56,7 +56,7 @@ void mysql_client_binlog_statement(THD*
> Format_description_event.
> */
> my_bool have_fd_event= TRUE;
> - int err;
> + int err= 0;
> Relay_log_info *rli;
> rli= thd->rli_fake;
> if (!rli)
> @@ -85,6 +85,7 @@ void mysql_client_binlog_statement(THD*
> rli->relay_log.description_event_for_exec&&
> buf))
> {
> + err= 1;
> my_error(ER_OUTOFMEMORY, MYF(0), 1); /* needed 1 bytes */
> goto end;
> }
> @@ -111,6 +112,7 @@ void mysql_client_binlog_statement(THD*
>
> if (bytes_decoded< 0)
> {
> + err= 1;
> my_error(ER_BASE64_DECODE_ERROR, MYF(0));
> goto end;
> }
> @@ -150,6 +152,7 @@ void mysql_client_binlog_statement(THD*
> event_len, bytes_decoded));
> if (bytes_decoded< EVENT_LEN_OFFSET || (uint) bytes_decoded<
> event_len)
> {
> + err= 1;
> my_error(ER_SYNTAX_ERROR, MYF(0));
> goto end;
> }
> @@ -166,6 +169,7 @@ void mysql_client_binlog_statement(THD*
> have_fd_event= TRUE;
> else
> {
> + err= 1;
> my_error(ER_NO_FORMAT_DESCRIPTION_EVENT_BEFORE_BINLOG_STATEMENT,
> MYF(0), Log_event::get_type_str((Log_event_type)type));
> goto end;
> @@ -183,6 +187,7 @@ void mysql_client_binlog_statement(THD*
> causes by a bad statement
> */
> my_error(ER_SYNTAX_ERROR, MYF(0));
> + err= 1;
> goto end;
> }
>
> @@ -238,10 +243,9 @@ void mysql_client_binlog_statement(THD*
>
>
> DBUG_PRINT("info",("binlog base64 execution finished successfully"));
> - my_ok(thd);
>
> end:
> rli->clear_tables_to_lock();
> my_free(buf, MYF(MY_ALLOW_ZERO_PTR));
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(err);
> }
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc 2010-06-07 10:01:54 +0000
> +++ b/sql/sql_parse.cc 2010-06-21 00:05:45 +0000
> @@ -4922,7 +4922,15 @@ create_sp_error:
> case SQLCOM_BINLOG_BASE64_EVENT:
> {
> #ifndef EMBEDDED_LIBRARY
> - mysql_client_binlog_statement(thd);
> + if (!mysql_client_binlog_statement(thd)&& !thd->main_da.is_set())
> + /*
> + Applying some 'BINLOG' statements makes my_ok to be called
> + right after applying the event (ev->apply_event). So we
> + need to check if the thd status hasn't yet been set, before
> + we set it here, thence not risking setting OK twice.
> + See BUG#54197.
> + */
> + my_ok(thd);
> #else /* EMBEDDED_LIBRARY */
> my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "embedded");
> #endif /* EMBEDDED_LIBRARY */
>
>
>
>
>
