From d531b4ee3a9bcd89a2fa6b49a2207eaf966f53e3 Mon Sep 17 00:00:00 2001 From: Sergei Petrunia Date: Sun, 12 Jan 2020 22:15:55 +0300 Subject: 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 version of the fix, with handling for class sp_package) 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 | 77 ++++++++++++++++++++++++++++++------------------------ sql/sp_head.h | 23 ++++++++++------ sql/sql_lex.cc | 10 +++---- sql/sql_parse.cc | 2 +- sql/sql_prepare.cc | 2 +- sql/sql_show.cc | 6 ++--- sql/sql_trigger.cc | 2 +- 9 files changed, 71 insertions(+), 55 deletions(-) diff --git a/sql/sp.cc b/sql/sp.cc index 5661c6910cb..5e162e5dfec 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -854,7 +854,7 @@ static sp_head *sp_compile(THD *thd, String *defstr, sql_mode_t 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 99e68cd2595..e4ffbdcb155 100644 --- a/sql/sp_cache.cc +++ b/sql/sp_cache.cc @@ -283,7 +283,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 360713fc452..af4316085b7 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -474,47 +474,39 @@ check_routine_name(const LEX_CSTRING *ident) * */ -void * -sp_head::operator new(size_t size) throw() +sp_head *sp_head::create(sp_package *parent, const Sp_handler *handler) { - DBUG_ENTER("sp_head::operator new"); MEM_ROOT own_root; + init_sql_alloc(&own_root, "sp_head", MEM_ROOT_BLOCK_SIZE, MEM_ROOT_PREALLOC, + MYF(0)); sp_head *sp; + if (!(sp= new (&own_root) sp_head(&own_root, parent, handler))) + free_root(&own_root, MYF(0)); - init_sql_alloc(&own_root, "sp_head", - 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 %p", &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 %p moved to %p", - &sp->mem_root, &own_root)); - free_root(&own_root, MYF(0)); - - DBUG_VOID_RETURN; +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_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_package *parent, const Sp_handler *sph) - :Query_arena(&main_mem_root, STMT_INITIALIZED_FOR_SP), +sp_head::sp_head(MEM_ROOT *mem_root_arg, sp_package *parent, + const Sp_handler *sph) + :Query_arena(NULL, STMT_INITIALIZED_FOR_SP), Database_qualified_name(&null_clex_str, &null_clex_str), + main_mem_root(*mem_root_arg), m_parent(parent), m_handler(sph), m_flags(0), @@ -545,6 +537,8 @@ sp_head::sp_head(sp_package *parent, const Sp_handler *sph) m_pcont(new (&main_mem_root) sp_pcontext()), m_cont_level(0) { + mem_root= &main_mem_root; + m_first_instance= this; m_first_free_instance= this; m_last_cached_sp= this; @@ -567,10 +561,25 @@ sp_head::sp_head(sp_package *parent, const Sp_handler *sph) } -sp_package::sp_package(LEX *top_level_lex, +sp_package *sp_package::create(LEX *top_level_lex, const sp_name *name, + const Sp_handler *sph) +{ + MEM_ROOT own_root; + init_sql_alloc(&own_root, "sp_package", MEM_ROOT_BLOCK_SIZE, + MEM_ROOT_PREALLOC, MYF(0)); + sp_package *sp; + if (!(sp= new (&own_root) sp_package(&own_root, top_level_lex, name, sph))) + free_root(&own_root, MYF(0)); + + return sp; +} + + +sp_package::sp_package(MEM_ROOT *mem_root_arg, + LEX *top_level_lex, const sp_name *name, const Sp_handler *sph) - :sp_head(NULL, sph), + :sp_head(mem_root_arg, NULL, sph), m_current_routine(NULL), m_top_level_lex(top_level_lex), m_rcontext(NULL), @@ -588,7 +597,7 @@ sp_package::~sp_package() m_routine_declarations.cleanup(); m_body= null_clex_str; if (m_current_routine) - delete m_current_routine->sphead; + sp_head::destroy(m_current_routine->sphead); delete m_rcontext; } @@ -845,7 +854,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 75c95d6705d..7e00cf7a0d8 100644 --- a/sql/sp_head.h +++ b/sql/sp_head.h @@ -127,7 +127,8 @@ bool check_routine_name(const LEX_CSTRING *ident); class sp_head :private Query_arena, - public Database_qualified_name + public Database_qualified_name, + public Sql_alloc { sp_head(const sp_head &); /**< Prevent use of these */ void operator=(sp_head &); @@ -316,13 +317,14 @@ public: */ SQL_I_List m_trg_table_fields; - static void * - operator new(size_t size) throw (); +protected: + sp_head(MEM_ROOT *mem_root, sp_package *parent, const Sp_handler *handler); + virtual ~sp_head(); - static void - operator delete(void *ptr, size_t size) throw (); +public: + static void destroy(sp_head *sp); + static sp_head *create(sp_package *parent, const Sp_handler *handler); - sp_head(sp_package *parent, const Sp_handler *handler); /// Initialize after we have reset mem_root void @@ -340,7 +342,6 @@ public: void set_stmt_end(THD *thd); - virtual ~sp_head(); bool execute_trigger(THD *thd, @@ -964,10 +965,16 @@ public: bool m_is_instantiated; bool m_is_cloning_routine; - sp_package(LEX *top_level_lex, +private: + sp_package(MEM_ROOT *mem_root, + LEX *top_level_lex, const sp_name *name, const Sp_handler *sph); ~sp_package(); +public: + static sp_package *create(LEX *top_level_lex, const sp_name *name, + const Sp_handler *sph); + bool add_routine_declaration(LEX *lex) { return m_routine_declarations.check_dup_qualified(lex->sphead) || diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index f6df176c6a0..6e6c79c0e6c 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -793,7 +793,7 @@ void lex_end_stage1(LEX *lex) } else { - delete lex->sphead; + sp_head::destroy(lex->sphead); lex->sphead= NULL; } @@ -3049,13 +3049,13 @@ void LEX::cleanup_lex_after_parse_error(THD *thd) DBUG_ASSERT(pkg == pkg->m_top_level_lex->sphead); pkg->restore_thd_mem_root(thd); LEX *top= pkg->m_top_level_lex; - delete pkg; + sp_package::destroy(pkg); thd->lex= top; thd->lex->sphead= NULL; } else { - delete thd->lex->sphead; + sp_head::destroy(thd->lex->sphead); thd->lex->sphead= NULL; } } @@ -6190,7 +6190,7 @@ sp_head *LEX::make_sp_head(THD *thd, const sp_name *name, sp_head *sp; /* Order is important here: new - reset - init */ - if (likely((sp= new sp_head(package, sph)))) + if (likely((sp= sp_head::create(package, sph)))) { sp->reset_thd_mem_root(thd); sp->init(this); @@ -7829,7 +7829,7 @@ sp_package *LEX::create_package_start(THD *thd, return 0; } } - if (unlikely(!(pkg= new sp_package(this, name_arg, sph)))) + if (unlikely(!(pkg= sp_package::create(this, name_arg, sph)))) return NULL; pkg->reset_thd_mem_root(thd); pkg->init(this); diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 861d50e8872..db38a20ea8a 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -5083,7 +5083,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 da6ec106208..d273db7d0d4 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -3848,7 +3848,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 3f18f659f7e..0b35789b869 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -6331,7 +6331,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); } } @@ -6378,7 +6378,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); @@ -6457,7 +6457,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 b8ce5d743b4..26ec04cb89e 100644 --- a/sql/sql_trigger.cc +++ b/sql/sql_trigger.cc @@ -338,7 +338,7 @@ public: Trigger::~Trigger() { - delete body; + sp_head::destroy(body); } -- cgit v1.2.1