revision-id: 9271843e31adef1f8c50e1d9622d07dee3386758 (mariadb-10.1.43-36-g9271843e31a) parent(s): 984b3c15449e0b5c7b3d66047a3c490c7be40faf author: Sergei Petrunia committer: Sergei Petrunia timestamp: 2020-01-12 20:50:12 +0200 message: MDEV-21341: Fix UBSAN failures: Issue Six (Variant #2 of the patch, which keeps the sp_head object inside the MEM_ROOT that sp_head object owns) (10.3 requires extra work due to sp_package, will commit a separate patch for it) sp_head::operator new() and operator delete() were dereferencing sp_head* pointers to memory that didn't hold a valid sp_head object (it was not created/already destroyed). This caused UBSan to crash when looking up type information. Fixed by providing static sp_head::create() and sp_head::destroy() methods. --- sql/sp.cc | 2 +- sql/sp_cache.cc | 2 +- sql/sp_head.cc | 62 ++++++++++++++++++++++++------------------------------ sql/sp_head.h | 17 ++++++++------- sql/sql_lex.cc | 4 ++-- sql/sql_parse.cc | 2 +- sql/sql_prepare.cc | 2 +- sql/sql_show.cc | 6 +++--- sql/sql_trigger.cc | 2 +- sql/sql_yacc.yy | 2 +- 10 files changed, 47 insertions(+), 54 deletions(-) diff --git a/sql/sp.cc b/sql/sp.cc index 966ea0280b4..1d340644ba1 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -754,7 +754,7 @@ static sp_head *sp_compile(THD *thd, String *defstr, ulonglong sql_mode, if (parse_sql(thd, & parser_state, creation_ctx) || thd->lex == NULL) { sp= thd->lex->sphead; - delete sp; + sp_head::destroy(sp); sp= 0; } else diff --git a/sql/sp_cache.cc b/sql/sp_cache.cc index f99c0bd0b6e..bc91634eb32 100644 --- a/sql/sp_cache.cc +++ b/sql/sp_cache.cc @@ -284,7 +284,7 @@ uchar *hash_get_key_for_sp_head(const uchar *ptr, size_t *plen, void hash_free_sp_head(void *p) { sp_head *sp= (sp_head *)p; - delete sp; + sp_head::destroy(sp); } diff --git a/sql/sp_head.cc b/sql/sp_head.cc index 5c5688be4a3..f940040b480 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -550,51 +550,41 @@ check_routine_name(LEX_STRING *ident) } -/* - * - * sp_head - * - */ - -void * -sp_head::operator new(size_t size) throw() +sp_head* sp_head::create() { - DBUG_ENTER("sp_head::operator new"); MEM_ROOT own_root; + init_sql_alloc(&own_root, MEM_ROOT_BLOCK_SIZE, MEM_ROOT_PREALLOC, MYF(0)); sp_head *sp; + if (!(sp= new (&own_root) sp_head(&own_root))) + free_root(&own_root, MYF(0)); - init_sql_alloc(&own_root, MEM_ROOT_BLOCK_SIZE, MEM_ROOT_PREALLOC, MYF(0)); - sp= (sp_head *) alloc_root(&own_root, size); - if (sp == NULL) - DBUG_RETURN(NULL); - sp->main_mem_root= own_root; - DBUG_PRINT("info", ("mem_root 0x%lx", (ulong) &sp->mem_root)); - DBUG_RETURN(sp); + return sp; } -void -sp_head::operator delete(void *ptr, size_t size) throw() -{ - DBUG_ENTER("sp_head::operator delete"); - MEM_ROOT own_root; - if (ptr == NULL) - DBUG_VOID_RETURN; - - sp_head *sp= (sp_head *) ptr; - - /* Make a copy of main_mem_root as free_root will free the sp */ - own_root= sp->main_mem_root; - DBUG_PRINT("info", ("mem_root 0x%lx moved to 0x%lx", - (ulong) &sp->mem_root, (ulong) &own_root)); - free_root(&own_root, MYF(0)); +void sp_head::destroy(sp_head *sp) +{ + if (sp) + { + /* Make a copy of main_mem_root as free_root will free the sp */ + MEM_ROOT own_root= sp->main_mem_root; + delete sp; - DBUG_VOID_RETURN; + DBUG_PRINT("info", ("mem_root 0x%lx moved to 0x%lx", + (ulong) &sp->mem_root, (ulong) &own_root)); + free_root(&own_root, MYF(0)); + } } +/* + * + * sp_head + * + */ -sp_head::sp_head() - :Query_arena(&main_mem_root, STMT_INITIALIZED_FOR_SP), +sp_head::sp_head(MEM_ROOT *mem_root_arg) + :Query_arena(NULL, STMT_INITIALIZED_FOR_SP), + main_mem_root(*mem_root_arg), // todo: std::move operator. m_flags(0), m_sp_cache_version(0), m_creation_ctx(0), @@ -603,6 +593,8 @@ sp_head::sp_head() m_next_cached_sp(0), m_cont_level(0) { + mem_root= &main_mem_root; + m_first_instance= this; m_first_free_instance= this; m_last_cached_sp= this; @@ -848,7 +840,7 @@ sp_head::~sp_head() my_hash_free(&m_sptabs); my_hash_free(&m_sroutines); - delete m_next_cached_sp; + sp_head::destroy(m_next_cached_sp); DBUG_VOID_RETURN; } diff --git a/sql/sp_head.h b/sql/sp_head.h index 2b3e568fb9a..47cb0985b05 100644 --- a/sql/sp_head.h +++ b/sql/sp_head.h @@ -142,7 +142,7 @@ class sp_name : public Sql_alloc bool check_routine_name(LEX_STRING *ident); -class sp_head :private Query_arena +class sp_head :private Query_arena, public Sql_alloc { sp_head(const sp_head &); /**< Prevent use of these */ void operator=(sp_head &); @@ -301,14 +301,16 @@ class sp_head :private Query_arena being opened is probably enough). */ SQL_I_List<Item_trigger_field> m_trg_table_fields; +private: + // users must use sp= sp_head::create() + sp_head(MEM_ROOT *mem_root_arg); - static void * - operator new(size_t size) throw (); - - static void - operator delete(void *ptr, size_t size) throw (); + // users must use sp_head::destroy(sp) + virtual ~sp_head(); - sp_head(); +public: + static sp_head* create(); + static void destroy(sp_head *sp); /// Initialize after we have reset mem_root void @@ -326,7 +328,6 @@ class sp_head :private Query_arena void set_stmt_end(THD *thd); - virtual ~sp_head(); bool execute_trigger(THD *thd, diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index a36a19357eb..a0d9cbea211 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -785,7 +785,7 @@ void lex_end_stage1(LEX *lex) } else { - delete lex->sphead; + sp_head::destroy(lex->sphead); lex->sphead= NULL; } @@ -2781,7 +2781,7 @@ void LEX::cleanup_lex_after_parse_error(THD *thd) if (thd->lex->sphead) { thd->lex->sphead->restore_thd_mem_root(thd); - delete thd->lex->sphead; + sp_head::destroy(thd->lex->sphead); thd->lex->sphead= NULL; } } diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 218d0dbd357..e5626ccbd7c 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -4347,7 +4347,7 @@ mysql_execute_command(THD *thd) /* Don't do it, if we are inside a SP */ if (!thd->spcont) { - delete lex->sphead; + sp_head::destroy(lex->sphead); lex->sphead= NULL; } /* lex->unit.cleanup() is called outside, no need to call it here */ diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 2c6aeda794a..f5adaeaa956 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -3607,7 +3607,7 @@ Prepared_statement::~Prepared_statement() free_items(); if (lex) { - delete lex->sphead; + sp_head::destroy(lex->sphead); delete lex->result; delete (st_lex_local *) lex; } diff --git a/sql/sql_show.cc b/sql/sql_show.cc index fbdb76e9e71..4f217159e5c 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -5863,7 +5863,7 @@ bool store_schema_params(THD *thd, TABLE *table, TABLE *proc_table, { free_table_share(&share); if (free_sp_head) - delete sp; + sp_head::destroy(sp); DBUG_RETURN(1); } } @@ -5919,7 +5919,7 @@ bool store_schema_params(THD *thd, TABLE *table, TABLE *proc_table, } } if (free_sp_head) - delete sp; + sp_head::destroy(sp); } free_table_share(&share); DBUG_RETURN(error); @@ -6012,7 +6012,7 @@ bool store_schema_proc(THD *thd, TABLE *table, TABLE *proc_table, store_column_type(table, field, cs, 5); free_table_share(&share); if (free_sp_head) - delete sp; + sp_head::destroy(sp); } } diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc index 4ecd8139921..c4d348ce400 100644 --- a/sql/sql_trigger.cc +++ b/sql/sql_trigger.cc @@ -1063,7 +1063,7 @@ Table_triggers_list::~Table_triggers_list() { for (int i= 0; i < (int)TRG_EVENT_MAX; i++) for (int j= 0; j < (int)TRG_ACTION_MAX; j++) - delete bodies[i][j]; + sp_head::destroy(bodies[i][j]); /* Free blobs used in insert */ if (record0_field) diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 71e0a18b1a3..2a46bb2a027 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -224,7 +224,7 @@ static sp_head *make_sp_head(THD *thd, sp_name *name, sp_head *sp; /* Order is important here: new - reset - init */ - if ((sp= new sp_head())) + if ((sp= sp_head::create())) { sp->reset_thd_mem_root(thd); sp->init(lex);