Hi Kristofer
Please see below for comments.
This patch will be OK to push after the following 2 items are resolved:
- See my questions related to the client/server protocol
To be discussed with kostja:
- There is (?) a minor change needed with the st_lex allocation:
new st_lex should not use malloc/free, but instead sp_head::reset_lex()
should use the sp_head MEM_ROOT to allocate the st_lex with a placement
operator:
new (thd->mem_root) st_lex
Regards,
Marc.
kpettersson@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of thek. When thek does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-10-30 15:29:30+01:00, thek@adventure.(none) +9 -0
> Bug #31153 calling stored procedure crashes server if available memory is low
>
> When the server was out of memory it crashed because of invalid memory access.
>
> This patch adds detection for failed memory allocations and make the server
> output a proper error message.
>
> mysys/my_new.cc@stripped, 2007-10-30 15:29:27+01:00, thek@adventure.(none) +3 -3
> Due to an inconsistency in gcc, the operator 'new' needs to be declared with an
> empty throw() directive or it will not return a NULL pointer on a failed
> malloc call.
>
> sql/mysqld.cc@stripped, 2007-10-30 15:29:28+01:00, thek@adventure.(none) +1 -1
> Don't try to push_warning if there is no more memory. It will cause a
> recursion until the stack is consumed.
>
> sql/sp_head.cc@stripped, 2007-10-30 15:29:28+01:00, thek@adventure.(none) +44 -7
> Added check for out-of-memory on a 'new' operation.
> Refactored reset_lex method to return a error state code instead of void.
> Initialize the mem-root with init_sql_alloc to get a basic error handler for
> memory allocation problems. This alone won't prevent the server from crashing,
> NULL pointers have to be account for as well.
>
> sql/sp_head.h@stripped, 2007-10-30 15:29:28+01:00, thek@adventure.(none) +3 -3
> Due to an inconsistency in gcc, the operator 'new' needs to be declared with an
> empty throw() directive or it will not return a NULL pointer on a failed
> malloc call.
>
>
The comments are misleading, and gcc is doing exactly what it's supposed to.
Adding a throw() clause has nothing to do with whether the operator new
*actually* returns a NULL or not.
Please change this comment (and similar ones in the patch) with:
Use the throw() clause in operator new, to indicate to the compiler that
memory allocation can fail and return NULL,
so that the compiler should generate code to check for NULL before
invoking C++ constructors, to be crash safe.
> sql/sql_base.cc@stripped, 2007-10-30 15:29:28+01:00, thek@adventure.(none) +1 -2
> Use init_sql_alloc to get basic out-of-memory error handling.
>
> sql/sql_lex.h@stripped, 2007-10-30 15:29:28+01:00, thek@adventure.(none) +6 -0
> Overloaded new and delete operators for the sp_head class.
>
> sql/sql_parse.cc@stripped, 2007-10-30 15:29:28+01:00, thek@adventure.(none) +5 -2
> Removed assertion to be able to test fatal error recovery in debug mode.
> Added code to restore the net object associated with the recycled thd object
> so that it won't remember runtime settings from previous statements.
>
> sql/sql_prepare.cc@stripped, 2007-10-30 15:29:28+01:00, thek@adventure.(none) +1 -1
> Use init_sql_alloc to get basic out-of-memory error handling.
>
> sql/sql_yacc.yy@stripped, 2007-10-30 15:29:28+01:00, thek@adventure.(none) +31 -11
> If reset_lex fails, the parsing must stop to prevent access to uninitialized
> objects. The same goes for any memory allocation attempt.
>
> diff -Nrup a/mysys/my_new.cc b/mysys/my_new.cc
> --- a/mysys/my_new.cc 2006-12-23 20:04:08 +01:00
> +++ b/mysys/my_new.cc 2007-10-30 15:29:27 +01:00
> @@ -22,17 +22,17 @@
>
> #ifdef USE_MYSYS_NEW
>
> -void *operator new (size_t sz)
> +void *operator new (size_t sz) throw ()
> {
> return (void *) malloc (sz ? sz : 1);
> }
>
Ok for consistency, but this code has no effect:
adding a throw() clause in the implementation does not change the
declaration,
which is in the standard library header files,
and does not change the operator new itself,
since the throw clause is not part of the C++ symbol mangling.
>
> -void *operator new[] (size_t sz)
> +void *operator new[] (size_t sz) throw ()
> {
> return (void *) malloc (sz ? sz : 1);
> }
>
Ok for consistency, but this code has no effect (same)
>
> -void operator delete (void *ptr)
> +void operator delete (void *ptr) throw ()
> {
> if (ptr)
> free(ptr);
>
Ok for consistency, but this code has no effect (same)
> diff -Nrup a/sql/mysqld.cc b/sql/mysqld.cc
> --- a/sql/mysqld.cc 2007-08-16 16:25:45 +02:00
> +++ b/sql/mysqld.cc 2007-10-30 15:29:28 +01:00
> @@ -2488,7 +2488,7 @@ static int my_message_sql(uint error, co
>
> thd->query_error= 1; // needed to catch query errors during replication
>
> - if (!thd->no_warnings_for_error)
> + if (!thd->no_warnings_for_error && error != ER_OUTOFMEMORY)
>
Ok, it's inside my_message_sql().
> push_warning(thd, MYSQL_ERROR::WARN_LEVEL_ERROR, error, str);
> /*
> thd->lex->current_select == 0 if lex structure is not inited
> diff -Nrup a/sql/sp_head.cc b/sql/sp_head.cc
> --- a/sql/sp_head.cc 2007-07-31 14:23:23 +02:00
> +++ b/sql/sp_head.cc 2007-10-30 15:29:28 +01:00
> @@ -411,14 +411,21 @@ check_routine_name(LEX_STRING ident)
> */
>
> void *
> -sp_head::operator new(size_t size)
> +sp_head::operator new(size_t size) throw()
> {
> DBUG_ENTER("sp_head::operator new");
> MEM_ROOT own_root;
> sp_head *sp;
>
> - init_alloc_root(&own_root, MEM_ROOT_BLOCK_SIZE, MEM_ROOT_PREALLOC);
> + init_sql_alloc(&own_root, MEM_ROOT_BLOCK_SIZE, MEM_ROOT_PREALLOC);
> sp= (sp_head *) alloc_root(&own_root, size);
> + if (sp == NULL)
> + {
> + THD *thd= current_thd;
> + my_error(ER_OUTOFMEMORY, MYF(0), size);
> + thd->fatal_error();
> + return NULL;
> + }
>
Here, the code should be just:
if (sp == NULL) return NULL;
since reporting the error is supposed to be done inside alloc_root,
due to init_sql_alloc().
> sp->main_mem_root= own_root;
> DBUG_PRINT("info", ("mem_root 0x%lx", (ulong) &sp->mem_root));
> DBUG_RETURN(sp);
> @@ -429,6 +436,10 @@ sp_head::operator delete(void *ptr, size
> {
> DBUG_ENTER("sp_head::operator delete");
> MEM_ROOT own_root;
> +
> + if (ptr == NULL)
> + DBUG_VOID_RETURN;
> +
>
Ok
> sp_head *sp= (sp_head *) ptr;
>
> /* Make a copy of main_mem_root as free_root will free the sp */
> @@ -472,6 +483,14 @@ sp_head::init(LEX *lex)
>
> lex->spcont= m_pcont= new sp_pcontext();
>
> + if (!lex->spcont)
> + {
> + THD *thd= current_thd;
> + my_error(ER_OUTOFMEMORY,MYF(0), sizeof(sp_pcontext));
> + thd->fatal_error();
> + DBUG_VOID_RETURN;
> + }
> +
>
No, reporting the error should be done by the operator new
> /*
> Altough trg_table_fields list is used only in triggers we init for all
> types of stored procedures to simplify reset_lex()/restore_lex() code.
> @@ -973,7 +992,7 @@ sp_head::execute(THD *thd)
> DBUG_RETURN(TRUE);
>
> /* init per-instruction memroot */
> - init_alloc_root(&execute_mem_root, MEM_ROOT_BLOCK_SIZE, 0);
> + init_sql_alloc(&execute_mem_root, MEM_ROOT_BLOCK_SIZE, 0);
>
> DBUG_ASSERT(!(m_flags & IS_INVOKED));
> m_flags|= IS_INVOKED;
> @@ -1795,16 +1814,33 @@ sp_head::execute_procedure(THD *thd, Lis
> }
>
>
> -// Reset lex during parsing, before we parse a sub statement.
> -void
> +/**
> + @brief Reset lex during parsing, before we parse a sub statement.
> +
> + @param thd Thread handler.
> +
> + @return Error state
> + @retval true An error occurred.
> + @retval false Success.
> +*/
> +
> +bool
> sp_head::reset_lex(THD *thd)
> {
> DBUG_ENTER("sp_head::reset_lex");
> LEX *sublex;
> LEX *oldlex= thd->lex;
>
> + sublex= new st_lex;
> + if (sublex == 0)
> + {
> + my_error(ER_OUTOFMEMORY, MYF(0),sizeof(st_lex));
> + thd->fatal_error();
> + DBUG_RETURN(TRUE);
> + }
> +
>
Same here : error reporting inside
> + thd->lex= sublex;
> (void)m_lex.push_front(oldlex);
> - thd->lex= sublex= new st_lex;
>
> /* Reset most stuff. */
> lex_start(thd);
> @@ -1827,7 +1863,7 @@ sp_head::reset_lex(THD *thd)
> sublex->interval_list.empty();
> sublex->type= 0;
>
> - DBUG_VOID_RETURN;
> + DBUG_RETURN(FALSE);
> }
>
> // Restore lex during parsing, after we have parsed a sub statement.
> @@ -3704,6 +3740,7 @@ sp_add_to_query_tables(THD *thd, LEX *le
> if (!(table= (TABLE_LIST *)thd->calloc(sizeof(TABLE_LIST))))
> {
> my_error(ER_OUTOFMEMORY, MYF(0), sizeof(TABLE_LIST));
> + thd->fatal_error();
>
Ok, due to the existing design of thd->calloc().
> return NULL;
> }
> table->db_length= strlen(db);
> diff -Nrup a/sql/sp_head.h b/sql/sp_head.h
> --- a/sql/sp_head.h 2007-07-12 20:26:38 +02:00
> +++ b/sql/sp_head.h 2007-10-30 15:29:28 +01:00
> @@ -191,10 +191,10 @@ public:
> Security_context m_security_ctx;
>
> static void *
> - operator new(size_t size);
> + operator new(size_t size) throw ();
>
Ok
>
> static void
> - operator delete(void *ptr, size_t size);
> + operator delete(void *ptr, size_t size) throw ();
>
Ok
>
> sp_head();
>
> @@ -254,7 +254,7 @@ public:
> }
>
> // Resets lex in 'thd' and keeps a copy of the old one.
> - void
> + bool
> reset_lex(THD *thd);
>
Ok
>
> // Restores lex in 'thd' from our copy, but keeps some status from the
> diff -Nrup a/sql/sql_base.cc b/sql/sql_base.cc
> --- a/sql/sql_base.cc 2007-08-24 13:28:10 +02:00
> +++ b/sql/sql_base.cc 2007-10-30 15:29:28 +01:00
> @@ -80,7 +80,6 @@ bool Prelock_error_handler::safely_trapp
> return ((m_handled_errors > 0) && (m_unhandled_errors == 0));
> }
>
> -
> TABLE *unused_tables; /* Used by mysql_test */
> HASH open_cache; /* Used by mysql_test */
>
> @@ -2643,7 +2642,7 @@ int open_tables(THD *thd, TABLE_LIST **s
> temporary mem_root for new .frm parsing.
> TODO: variables for size
> */
> - init_alloc_root(&new_frm_mem, 8024, 8024);
> + init_sql_alloc(&new_frm_mem, 8024, 8024);
>
Ok
>
> thd->current_tablenr= 0;
> restart:
> diff -Nrup a/sql/sql_lex.h b/sql/sql_lex.h
> --- a/sql/sql_lex.h 2007-08-28 17:51:02 +02:00
> +++ b/sql/sql_lex.h 2007-10-30 15:29:28 +01:00
> @@ -1169,6 +1169,11 @@ typedef struct st_lex : public Query_tab
>
> st_lex();
>
> + static void*
> + operator new(size_t sz) throw() { return (void *) malloc (sz ? sz : 1); }
> +
> + void operator delete (void *ptr) throw() { free(ptr); }
> +
>
This needs to be discussed / revised.
It seems that the current code did use the global operator new
(implemented as malloc in mysys)
to allocate new LEX structures in sp_head::reset_lex(), but that
probably is a bug.
LEX structures should be allocated in the sp_head's mem_root.
> virtual ~st_lex()
> {
> destroy_query_tables_list();
> @@ -1303,3 +1308,4 @@ extern void lex_start(THD *thd);
> extern void lex_end(LEX *lex);
> extern int MYSQLlex(void *arg, void *yythd);
> extern char *skip_rear_comments(CHARSET_INFO *cs, char *begin, char *end);
> +
> diff -Nrup a/sql/sql_parse.cc b/sql/sql_parse.cc
> --- a/sql/sql_parse.cc 2007-09-03 09:22:54 +02:00
> +++ b/sql/sql_parse.cc 2007-10-30 15:29:28 +01:00
> @@ -34,6 +34,7 @@
> #include "sp_cache.h"
> #include "sql_trigger.h"
>
> +
> #ifdef HAVE_OPENSSL
> /*
> Without SSL the handshake consists of one packet. This packet
> @@ -5850,6 +5851,7 @@ void mysql_reset_thd_for_next_command(TH
> DBUG_ASSERT(thd->security_ctx== &thd->main_security_ctx);
> thd->tmp_table_used= 0;
> thd->thread_specific_used= FALSE;
> +
> if (!thd->in_sub_stmt)
> {
> if (opt_bin_log)
> @@ -5862,6 +5864,9 @@ void mysql_reset_thd_for_next_command(TH
> thd->rand_used= 0;
> thd->sent_row_count= thd->examined_row_count= 0;
> }
> +
> + thd->net.no_send_error= FALSE;
> + thd->net.no_send_ok= FALSE;
>
Just a question:
Could you detail why this change is needed, and what it does ?
So far, the patch was about robustness in failed memory allocations,
but here the client/server protocol is impacted ...
> DBUG_VOID_RETURN;
> }
>
> @@ -6098,10 +6103,8 @@ void mysql_parse(THD *thd, const char *i
> }
> else
> {
> - DBUG_ASSERT(thd->net.report_error);
>
Same question, see above
> DBUG_PRINT("info",("Command aborted. Fatal_error: %d",
> thd->is_fatal_error));
> -
> query_cache_abort(&thd->net);
> }
> if (thd->lex->sphead)
> diff -Nrup a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> --- a/sql/sql_prepare.cc 2007-07-12 21:22:14 +02:00
> +++ b/sql/sql_prepare.cc 2007-10-30 15:29:28 +01:00
> @@ -2659,7 +2659,7 @@ Prepared_statement::Prepared_statement(T
> last_errno(0),
> flags((uint) IS_IN_USE)
> {
> - init_alloc_root(&main_mem_root, thd_arg->variables.query_alloc_block_size,
> + init_sql_alloc(&main_mem_root, thd_arg->variables.query_alloc_block_size,
>
Ok
> thd_arg->variables.query_prealloc_size);
> *last_error= '\0';
> }
> diff -Nrup a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> --- a/sql/sql_yacc.yy 2007-09-12 20:39:32 +02:00
> +++ b/sql/sql_yacc.yy 2007-10-30 15:29:28 +01:00
> @@ -1628,6 +1628,8 @@ create_function_tail:
> }
> /* Order is important here: new - reset - init */
> sp= new sp_head();
> + if (sp == NULL)
> + MYSQL_YYABORT;
>
Ok
> sp->reset_thd_mem_root(thd);
> sp->init(lex);
> sp->init_sp_name(thd, lex->spname);
> @@ -1929,7 +1931,8 @@ sp_decl:
> {
> LEX *lex= Lex;
>
> - lex->sphead->reset_lex(YYTHD);
> + if (lex->sphead->reset_lex(YYTHD))
> + MYSQL_YYABORT;
>
Ok
> lex->spcont->declare_var_boundary($2);
> }
> type
> @@ -1937,6 +1940,10 @@ sp_decl:
> {
> LEX *lex= Lex;
> sp_pcontext *pctx= lex->spcont;
> + if (pctx == 0)
> + {
> + MYSQL_YYABORT;
> + }
>
Not sure why this is needed.
If memory allocation failed when sp_pcontext was allocated,
the error should have been trapped then, and the parser should already
have aborted.
> uint num_vars= pctx->context_var_count();
> enum enum_field_types var_type= (enum enum_field_types) $4;
> Item *dflt_value_item= $5;
> @@ -2064,7 +2071,8 @@ sp_decl:
>
> sp_cursor_stmt:
> {
> - Lex->sphead->reset_lex(YYTHD);
> + if(Lex->sphead->reset_lex(YYTHD))
> + MYSQL_YYABORT;
>
Ok
>
> /* We use statement here just be able to get a better
> error message. Using 'select' works too, but will then
> @@ -2230,7 +2238,8 @@ sp_proc_stmt:
> LEX *lex= thd->lex;
> Lex_input_stream *lip= thd->m_lip;
>
> - lex->sphead->reset_lex(thd);
> + if (lex->sphead->reset_lex(thd))
> + MYSQL_YYABORT;
>
Ok
> lex->sphead->m_tmp_query= lip->tok_start;
> }
> statement
> @@ -2275,7 +2284,7 @@ sp_proc_stmt:
> sp->restore_lex(thd);
> }
> | RETURN_SYM
> - { Lex->sphead->reset_lex(YYTHD); }
> + { if(Lex->sphead->reset_lex(YYTHD)) MYSQL_YYABORT; }
>
Ok, but write the statement in 2 lines:
if (...)
MYSQL_YYABORT;
This will be critical for code coverage later, to make sure that error
paths are tested and covered separately.
> expr
> {
> LEX *lex= Lex;
> @@ -2472,7 +2481,7 @@ sp_fetch_list:
> ;
>
> sp_if:
> - { Lex->sphead->reset_lex(YYTHD); }
> + { if (Lex->sphead->reset_lex(YYTHD)) MYSQL_YYABORT; }
>
Ok, write in 2 lines
> expr THEN_SYM
> {
> LEX *lex= Lex;
> @@ -2522,7 +2531,7 @@ simple_case_stmt:
> {
> LEX *lex= Lex;
> case_stmt_action_case(lex);
> - lex->sphead->reset_lex(YYTHD); /* For expr $3 */
> + if (lex->sphead->reset_lex(YYTHD)) MYSQL_YYABORT; /* For expr $3
> */
>
Ok, write in 2 lines
> }
> expr
> {
> @@ -2571,7 +2580,7 @@ searched_when_clause_list:
> simple_when_clause:
> WHEN_SYM
> {
> - Lex->sphead->reset_lex(YYTHD); /* For expr $3 */
> + if (Lex->sphead->reset_lex(YYTHD)) MYSQL_YYABORT; /* For expr $3
> */
>
Ok, write in 2 lines
> }
> expr
> {
> @@ -2592,7 +2601,7 @@ simple_when_clause:
> searched_when_clause:
> WHEN_SYM
> {
> - Lex->sphead->reset_lex(YYTHD); /* For expr $3 */
> + if (Lex->sphead->reset_lex(YYTHD)) MYSQL_YYABORT; /* For expr $3
> */
>
Ok, write in 2 lines
> }
> expr
> {
> @@ -2703,7 +2712,7 @@ sp_unlabeled_control:
> lex->sphead->add_instr(i);
> }
> | WHILE_SYM
> - { Lex->sphead->reset_lex(YYTHD); }
> + { if (Lex->sphead->reset_lex(YYTHD)) MYSQL_YYABORT; }
>
Ok, write in 2 lines
> expr DO_SYM
> {
> LEX *lex= Lex;
> @@ -2729,7 +2738,7 @@ sp_unlabeled_control:
> lex->sphead->do_cont_backpatch();
> }
> | REPEAT_SYM sp_proc_stmts1 UNTIL_SYM
> - { Lex->sphead->reset_lex(YYTHD); }
> + { if (Lex->sphead->reset_lex(YYTHD)) MYSQL_YYABORT; }
>
Ok, write in 2 lines
> expr END REPEAT_SYM
> {
> LEX *lex= Lex;
> @@ -4271,6 +4280,8 @@ select_init2:
> select_part2
> {
> LEX *lex= Lex;
> + if (lex == NULL)
> + MYSQL_YYABORT;
>
Ok
> SELECT_LEX * sel= lex->current_select;
> if (lex->current_select->set_braces(0))
> {
> @@ -8422,7 +8433,14 @@ option_type_value:
>
> QQ: May be we should simply prohibit group assignments in SP?
> */
> - Lex->sphead->reset_lex(thd);
> + if (Lex->sphead->reset_lex(thd))
> + {
> + /*
> + Failed to allocate memory or something else really bad
> + happened.
> + */
> + MYSQL_YYABORT;
> + }
>
Ok, but remove the comment.
> lex= thd->lex;
>
> /* Set new LEX as if we at start of set rule. */
> @@ -9832,6 +9850,8 @@ sp_tail:
>
> /* Order is important here: new - reset - init */
> sp= new sp_head();
> + if (sp == NULL)
> + MYSQL_YYABORT;
>
Ok
> sp->reset_thd_mem_root(YYTHD);
> sp->init(lex);
> sp->m_type= TYPE_ENUM_PROCEDURE;
>
>
