Hi Kristofer,
On 7/27/09 1:08 PM, Kristofer Pettersson wrote:
> #At file:///Users/thek/Development/51-bug44521/ based on
> revid:pstoev@stripped
>
> 2935 Kristofer Pettersson 2009-07-27
> Bug#44521 Executing a stored procedure as a prepared statement can sometimes
> cause
> an assertion in a debug build.
>
> The reason is that the C API doesn't support multiple result sets for
> prepared
> statements and attempting to execute a stored routine which returns multiple
> result
> sets sometimes lead to a network error. The network error sets the diagnostic
> area
> prematurely which later leads to the assert when an attempt is made to set a
> second
> server state.
>
> This patch fixes the issue by changing the scope of the error code returned
> by
> sp_instr_stmt::execute() to include any error which happened during the
> execution.
> To assure that Diagnostic_area::is_sent really mean that the message was sent
> all
> network related functions are checked for return status.
> @ sql/net_serv.cc
> * Added error injection code.
> * Removed debug assert in order to get proper error code in debug build.
> @ sql/protocol.cc
> * Changed prototype to return success/failure status on
> net_send_error_packet(),
> net_send_ok(), net_send_eof(), write_eof_packet().
> @ sql/protocol.h
> * Changed prototype on net_send_error() so that a success/failure status is
> reported.
> @ sql/sp_head.cc
> * If any error has been reported inside sp_instr_stmt::execute() the entire
> method
> should return a failure status.
> @ sql/sql_parse.cc
> * Added error injection code for the regression test.
>
> modified:
> mysql-test/r/sp.result
> mysql-test/t/sp.test
> sql/net_serv.cc
> sql/protocol.cc
> sql/protocol.h
> sql/sp_head.cc
> sql/sql_parse.cc
> === modified file 'mysql-test/r/sp.result'
> --- a/mysql-test/r/sp.result 2009-06-04 11:53:15 +0000
> +++ b/mysql-test/r/sp.result 2009-07-27 16:08:36 +0000
> @@ -6963,6 +6963,17 @@ CALL p1();
> CALL p1();
> DROP PROCEDURE p1;
> DROP TABLE t1;
> +#
> +# Bug44521 Prepared Statement: CALL p() - crashes: `! thd->main_da.is_sent'
> failed et.al.
> +#
> +DROP PROCEDURE IF EXISTS p;
> +CREATE PROCEDURE p() BEGIN SELECT 1; SELECT 2; END$
> +set session debug="+d,net_real_write_failure";
> +CALL p();
> +ERROR HY000: Lost connection to MySQL server during query
> +# Server should still be up and running on the default connection.
> +DROP PROCEDURE p;
> +#
> # ------------------------------------------------------------------
> # -- End of 5.1 tests
> # ------------------------------------------------------------------
>
> === modified file 'mysql-test/t/sp.test'
> --- a/mysql-test/t/sp.test 2009-06-04 11:53:15 +0000
> +++ b/mysql-test/t/sp.test 2009-07-27 16:08:36 +0000
> @@ -8242,6 +8242,25 @@ while ($tab_count)
> DROP PROCEDURE p1;
> DROP TABLE t1;
>
> +--echo #
> +--echo # Bug44521 Prepared Statement: CALL p() - crashes: `!
> thd->main_da.is_sent' failed et.al.
> +--echo #
> +--disable_warnings
> +DROP PROCEDURE IF EXISTS p;
> +--enable_warnings
> +--connect (con1,localhost,root,,)
> +--connection con1
> +delimiter $;
> +CREATE PROCEDURE p() BEGIN SELECT 1; SELECT 2; END$
> +delimiter ;$
> +set session debug="+d,net_real_write_failure";
Test needs to be run under debug server only.
> +--error 2013
> +CALL p();
> +--connection default
> +--echo # Server should still be up and running on the default connection.
> +DROP PROCEDURE p;
> +--echo #
disconnect con1 so that mysqltest resources associated with the
connection are freed.
> +
> --echo # ------------------------------------------------------------------
> --echo # -- End of 5.1 tests
> --echo # ------------------------------------------------------------------
>
> === modified file 'sql/net_serv.cc'
> --- a/sql/net_serv.cc 2009-02-13 16:41:47 +0000
> +++ b/sql/net_serv.cc 2009-07-27 16:08:36 +0000
> @@ -48,6 +48,7 @@
> #include<violite.h>
> #include<signal.h>
> #include<errno.h>
> +#include<ctype.h>
> #ifdef __NETWARE__
> #include<sys/select.h>
> #endif
> @@ -330,13 +331,13 @@ void net_clear(NET *net, my_bool clear_b
> DBUG_VOID_RETURN;
> }
>
> -
> /** Flush write_buffer if not empty. */
>
> my_bool net_flush(NET *net)
> {
> my_bool error= 0;
> DBUG_ENTER("net_flush");
> +
> if (net->buff != net->write_pos)
> {
> error=test(net_real_write(net, net->buff,
> @@ -561,6 +562,10 @@ net_real_write(NET *net,const uchar *pac
> my_bool net_blocking = vio_is_blocking(net->vio);
> DBUG_ENTER("net_real_write");
>
> +#ifndef DBUG_OFF
> + DBUG_EXECUTE_IF("net_real_write_failure2", DBUG_RETURN(-1););
> +#endif
> +
> #if defined(MYSQL_SERVER)&& defined(USE_QUERY_CACHE)
> query_cache_insert(net, (char*) packet, len);
> #endif
> @@ -907,7 +912,6 @@ my_real_read(NET *net, size_t *complen)
> (int) net->buff[net->where_b + 3],
> (uint) (uchar) net->pkt_nr);
> fflush(stderr);
> - DBUG_ASSERT(0);
No. I haven't seen a explanation for this.
> #endif
> }
> len= packet_error;
>
> === modified file 'sql/protocol.cc'
> --- a/sql/protocol.cc 2009-03-24 13:58:52 +0000
> +++ b/sql/protocol.cc 2009-07-27 16:08:36 +0000
> @@ -29,11 +29,11 @@
>
> static const unsigned int PACKET_BUFFER_EXTRA_ALLOC= 1024;
> /* Declared non-static only because of the embedded library. */
> -void net_send_error_packet(THD *thd, uint sql_errno, const char *err);
> -void net_send_ok(THD *, uint, uint, ha_rows, ulonglong, const char *);
> -void net_send_eof(THD *thd, uint server_status, uint total_warn_count);
> +bool net_send_error_packet(THD *thd, uint sql_errno, const char *err);
> +bool net_send_ok(THD *, uint, uint, ha_rows, ulonglong, const char *);
> +bool net_send_eof(THD *thd, uint server_status, uint total_warn_count);
> #ifndef EMBEDDED_LIBRARY
> -static void write_eof_packet(THD *thd, NET *net,
> +static bool write_eof_packet(THD *thd, NET *net,
> uint server_status, uint total_warn_count);
> #endif
>
> @@ -70,8 +70,17 @@ bool Protocol_binary::net_store_data(con
> For SIGNAL/RESIGNAL and GET DIAGNOSTICS functionality it's
> critical that every error that can be intercepted is issued in one
> place only, my_message_sql.
> +
> + @param thd Thread handler
> + @param sql_errno The error code to send
> + @param err A pointer to the error message
> +
> + @return
> + @retval FALSE The message was sent to the client
> + @retval TRUE An error occurred and the message wasn't sent properly
> */
> -void net_send_error(THD *thd, uint sql_errno, const char *err)
> +
> +bool net_send_error(THD *thd, uint sql_errno, const char *err)
> {
> DBUG_ENTER("net_send_error");
>
> @@ -80,6 +89,7 @@ void net_send_error(THD *thd, uint sql_e
> DBUG_ASSERT(err&& err[0]);
>
> DBUG_PRINT("enter",("sql_errno: %d err: %s", sql_errno, err));
> + bool error;
>
> /*
> It's one case when we can push an error even though there
> @@ -90,11 +100,11 @@ void net_send_error(THD *thd, uint sql_e
> /* Abort multi-result sets */
> thd->server_status&= ~SERVER_MORE_RESULTS_EXISTS;
>
> - net_send_error_packet(thd, sql_errno, err);
> + error= net_send_error_packet(thd, sql_errno, err);
>
> thd->main_da.can_overwrite_status= FALSE;
>
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(error);
> }
>
> /**
> @@ -113,25 +123,33 @@ void net_send_error(THD *thd, uint sql_e
> Is not stored if no message.
>
> @param thd Thread handler
> + @param server_status The server status
> + @param total_warn_count Total number of warnings
> @param affected_rows Number of rows changed by statement
> @param id Auto_increment id for first row (if used)
> @param message Message to send to the client (Used by mysql_status)
> +
> + @return
> + @retval FALSE The message was successfully sent
> + @retval TRUE An error occurred and the messages wasn't sent properly
> +
> */
>
> #ifndef EMBEDDED_LIBRARY
> -void
> +bool
> net_send_ok(THD *thd,
> uint server_status, uint total_warn_count,
> ha_rows affected_rows, ulonglong id, const char *message)
> {
> NET *net=&thd->net;
> uchar buff[MYSQL_ERRMSG_SIZE+10],*pos;
> + bool error= FALSE;
> DBUG_ENTER("my_ok");
>
> if (! net->vio) // hack for re-parsing queries
> {
> DBUG_PRINT("info", ("vio present: NO"));
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(FALSE);
> }
>
> buff[0]=0; // No fields
> @@ -162,13 +180,14 @@ net_send_ok(THD *thd,
>
> if (message&& message[0])
> pos= net_store_data(pos, (uchar*) message, strlen(message));
> - VOID(my_net_write(net, buff, (size_t) (pos-buff)));
> - VOID(net_flush(net));
> + error= my_net_write(net, buff, (size_t) (pos-buff));
> + if (!error)
> + error= net_flush(net);
>
> thd->main_da.can_overwrite_status= FALSE;
> DBUG_PRINT("info", ("OK sent, so no more error sending allowed"));
>
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(error);
> }
>
> static uchar eof_buff[1]= { (uchar) 254 }; /* Marker for end of fields */
> @@ -188,37 +207,54 @@ static uchar eof_buff[1]= { (uchar) 254
> client.
>
> @param thd Thread handler
> - @param no_flush Set to 1 if there will be more data to the client,
> - like in send_fields().
> + @param server_status The server status
> + @param total_warn_count Total number of warnings
> +
> + @return
> + @retval FALSE The message was successfully sent
> + @retval TRUE An error occurred and the message wasn't sent properly
> */
>
> -void
> +bool
> net_send_eof(THD *thd, uint server_status, uint total_warn_count)
> {
> NET *net=&thd->net;
> + bool error= FALSE;
> DBUG_ENTER("net_send_eof");
> /* Set to TRUE if no active vio, to work well in case of --init-file */
> if (net->vio != 0)
> {
> thd->main_da.can_overwrite_status= TRUE;
> - write_eof_packet(thd, net, server_status, total_warn_count);
> - VOID(net_flush(net));
> + error= write_eof_packet(thd, net, server_status, total_warn_count);
> + if (!error)
> + error= test(net_flush(net));
> thd->main_da.can_overwrite_status= FALSE;
> DBUG_PRINT("info", ("EOF sent, so no more error sending allowed"));
> }
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(error);
> }
>
>
> /**
> Format EOF packet according to the current protocol and
> write it to the network output buffer.
> +
> + @param thd The thread handler
> + @param net The network handler
> + @param server_status The server status
> + @param total_warn_count The number of warnings
> +
> +
> + @return
> + @retval FALSE The message was sent successfully
> + @retval TRUE An error occurred and the messages wasn't sent properly
> */
>
> -static void write_eof_packet(THD *thd, NET *net,
> +static bool write_eof_packet(THD *thd, NET *net,
> uint server_status,
> uint total_warn_count)
> {
> + bool error;
> if (thd->client_capabilities& CLIENT_PROTOCOL_41)
> {
> uchar buff[5];
> @@ -237,10 +273,12 @@ static void write_eof_packet(THD *thd, N
> if (thd->is_fatal_error)
> server_status&= ~SERVER_MORE_RESULTS_EXISTS;
> int2store(buff + 3, server_status);
> - VOID(my_net_write(net, buff, 5));
> + error= test(my_net_write(net, buff, 5));
> }
> else
> - VOID(my_net_write(net, eof_buff, 1));
> + error= test(my_net_write(net, eof_buff, 1));
> +
> + return error;
> }
>
> /**
> @@ -261,7 +299,17 @@ bool send_old_password_request(THD *thd)
> }
>
>
> -void net_send_error_packet(THD *thd, uint sql_errno, const char *err)
> +/**
> + @param thd Thread handler
> + @param sql_errno The error code to send
> + @param err A pointer to the error message
> +
> + @return
> + @retval FALSE The message was successfully sent
> + @retval TRUE An error occurred and the messages wasn't sent properly
> +*/
> +
> +bool net_send_error_packet(THD *thd, uint sql_errno, const char *err)
> {
> NET *net=&thd->net;
> uint length;
> @@ -279,7 +327,7 @@ void net_send_error_packet(THD *thd, uin
> /* In bootstrap it's ok to print on stderr */
> fprintf(stderr,"ERROR: %d %s\n",sql_errno,err);
> }
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(FALSE);
> }
>
> if (net->return_errno)
> @@ -301,9 +349,8 @@ void net_send_error_packet(THD *thd, uin
> length=(uint) strlen(err);
> set_if_smaller(length,MYSQL_ERRMSG_SIZE-1);
> }
> - VOID(net_write_command(net,(uchar) 255, (uchar*) "", 0, (uchar*) err,
> - length));
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(test(net_write_command(net,(uchar) 255, (uchar*) "", 0,
> + (uchar*) err, length)));
> }
>
> #endif /* EMBEDDED_LIBRARY */
> @@ -389,36 +436,46 @@ void net_end_statement(THD *thd)
> if (thd->main_da.is_sent)
> return;
>
> + bool error= FALSE;
> +
> switch (thd->main_da.status()) {
> case Diagnostics_area::DA_ERROR:
> /* The query failed, send error to log and abort bootstrap. */
> - net_send_error(thd,
> - thd->main_da.sql_errno(),
> - thd->main_da.message());
> + error= net_send_error(thd,
> + thd->main_da.sql_errno(),
> + thd->main_da.message());
> break;
> case Diagnostics_area::DA_EOF:
> - net_send_eof(thd,
> - thd->main_da.server_status(),
> - thd->main_da.total_warn_count());
> + error= net_send_eof(thd,
> + thd->main_da.server_status(),
> + thd->main_da.total_warn_count());
> break;
> case Diagnostics_area::DA_OK:
> - net_send_ok(thd,
> - thd->main_da.server_status(),
> - thd->main_da.total_warn_count(),
> - thd->main_da.affected_rows(),
> - thd->main_da.last_insert_id(),
> - thd->main_da.message());
> + error= net_send_ok(thd,
> + thd->main_da.server_status(),
> + thd->main_da.total_warn_count(),
> + thd->main_da.affected_rows(),
> + thd->main_da.last_insert_id(),
> + thd->main_da.message());
> break;
> case Diagnostics_area::DA_DISABLED:
> break;
> case Diagnostics_area::DA_EMPTY:
> default:
> DBUG_ASSERT(0);
> - net_send_ok(thd, thd->server_status, thd->total_warn_count,
> - 0, 0, NULL);
> + error= net_send_ok(thd, thd->server_status, thd->total_warn_count,
> + 0, 0, NULL);
> break;
> }
> - thd->main_da.is_sent= TRUE;
> + if (!error)
> + thd->main_da.is_sent= TRUE;
> + else
> + {
> + /*
> + Drop the connection if the network reports an error.
> + */
> + thd->killed= THD::KILL_CONNECTION;
> + }
> }
>
>
>
> === modified file 'sql/protocol.h'
> --- a/sql/protocol.h 2007-12-20 21:11:37 +0000
> +++ b/sql/protocol.h 2009-07-27 16:08:36 +0000
> @@ -173,7 +173,7 @@ public:
> };
>
> void send_warning(THD *thd, uint sql_errno, const char *err=0);
> -void net_send_error(THD *thd, uint sql_errno=0, const char *err=0);
> +bool net_send_error(THD *thd, uint sql_errno=0, const char *err=0);
> void net_end_statement(THD *thd);
> bool send_old_password_request(THD *thd);
> uchar *net_store_data(uchar *to,const uchar *from, size_t length);
>
> === modified file 'sql/sp_head.cc'
> --- a/sql/sp_head.cc 2009-05-30 13:32:28 +0000
> +++ b/sql/sp_head.cc 2009-07-27 16:08:36 +0000
> @@ -1249,7 +1249,7 @@ sp_head::execute(THD *thd)
> */
> if (thd->prelocked_mode == NON_PRELOCKED)
> thd->user_var_events_alloc= thd->mem_root;
> -
> +
> err_status= i->execute(thd,&ip);
>
> if (i->free_list)
> @@ -2865,7 +2865,7 @@ sp_instr_stmt::execute(THD *thd, uint *n
> if (!thd->is_error())
> thd->main_da.reset_diagnostics_area();
> }
> - DBUG_RETURN(res);
> + DBUG_RETURN(res || thd->is_error());
> }
>
>
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc 2009-06-05 11:23:58 +0000
> +++ b/sql/sql_parse.cc 2009-07-27 16:08:36 +0000
> @@ -791,6 +791,7 @@ bool do_command(THD *thd)
> thd->clear_error(); // Clear error message
> thd->main_da.reset_diagnostics_area();
>
> +
New line...
> net_new_transaction(net);
>
> packet_length= my_net_read(net);
> @@ -4968,6 +4969,10 @@ static bool execute_sqlcom_select(THD *t
> LEX *lex= thd->lex;
> select_result *result=lex->result;
> bool res;
> +
> + DBUG_EXECUTE_IF("net_real_write_failure",
> + DBUG_SET("+d,net_real_write_failure2"););
> +
> /* assign global limit variable if limit is not given */
> {
> SELECT_LEX *param= lex->unit.global_parameters;
>
>
>
> ------------------------------------------------------------------------
>
>
