From 5e5ae51b730aa67f9efb87af4f4921309eac51f1 Mon Sep 17 00:00:00 2001 From: Sergei Petrunia Date: Sun, 12 Jan 2020 20:50:12 +0200 Subject: [PATCH 1/4] 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 | 72 +++++++++++++++++++++------------------------- 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, 52 insertions(+), 59 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* sp_head::create() +{ + 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)); + + return sp; +} + + +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 * */ -void * -sp_head::operator new(size_t size) throw() -{ - DBUG_ENTER("sp_head::operator new"); - MEM_ROOT own_root; - sp_head *sp; - - 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); -} - -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)); - - DBUG_VOID_RETURN; -} - - -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 @@ public: 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 @@ public: being opened is probably enough). */ SQL_I_List 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 (); + // users must use sp_head::destroy(sp) + virtual ~sp_head(); - static void - operator delete(void *ptr, size_t size) throw (); - - 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 @@ public: 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 1cd2a369d7a..27da1fc16fd 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); From 4635047ca1dbcd8b536da4be9236ffab8d48f2d9 Mon Sep 17 00:00:00 2001 From: Sergei Petrunia Date: Wed, 15 Jan 2020 16:34:51 +0300 Subject: [PATCH 2/4] MDEV-21341: Fix UBSAN failures, part #5 Item_cond inherits from Item_args but doesn't store its arguments as function arguments, which means it has zero arguments. Don't call memcpy in this case. --- sql/item_func.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sql/item_func.cc b/sql/item_func.cc index 702d8a1a2ee..ffd2b462431 100644 --- a/sql/item_func.cc +++ b/sql/item_func.cc @@ -109,7 +109,8 @@ Item_args::Item_args(THD *thd, const Item_args *other) arg_count= 0; return; } - memcpy(args, other->args, sizeof(Item*) * arg_count); + if (arg_count) + memcpy(args, other->args, sizeof(Item*) * arg_count); } From 23041af720416f00db69e07b15861fc6d61b2b45 Mon Sep 17 00:00:00 2001 From: Sergei Petrunia Date: Wed, 15 Jan 2020 16:35:59 +0300 Subject: [PATCH 3/4] MDEV-21341: Fix UBSAN failures, part 8: fix error in compare_fields_by_table_order Dont assign Item_field variables to point to Item_string objects (even if we don't make any dangerous calls for them). --- sql/sql_select.cc | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 40941294013..2358615affc 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -13669,12 +13669,16 @@ static int compare_fields_by_table_order(Item *field1, { int cmp= 0; bool outer_ref= 0; - Item_field *f1= (Item_field *) (field1->real_item()); - Item_field *f2= (Item_field *) (field2->real_item()); - if (field1->const_item() || f1->const_item()) + Item *field1_real= field1->real_item(); + Item *field2_real= field2->real_item(); + + if (field1->const_item() || field1_real->const_item()) return 1; - if (field2->const_item() || f2->const_item()) + if (field2->const_item() || field2_real->const_item()) return -1; + + Item_field *f1= (Item_field *) field1_real; + Item_field *f2= (Item_field *) field2_real; if (f2->used_tables() & OUTER_REF_TABLE_BIT) { outer_ref= 1; From bde7e0ba6e94d576c4563022f38e8d81b1f6d54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 16 Jan 2020 11:40:21 +0200 Subject: [PATCH 4/4] MDEV-21500 Server hang when using simulated AIO The write-heavy test innodb_zip.wl6501_scale_1 timed out on 10.2 60d7011c5f6ebda057d3e730c6f67519a1fb7f0c for me. Out of os_aio_n_segments=6, 5 are waiting for an event in os_aio_simulated_handler(). One thread is waiting for a write to complete in buf_dblwr_add_to_batch(), but that would never happen, because nothing is waking up the simulated AIO handler threads. This hang appears to have been introduced in MySQL 5.6.12 in mysql/mysql-server@26cfde776cdf5ce61bd5cc494dfc1df28c76977f. --- storage/innobase/buf/buf0dblwr.cc | 4 +++- storage/xtradb/buf/buf0dblwr.cc | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/storage/innobase/buf/buf0dblwr.cc b/storage/innobase/buf/buf0dblwr.cc index 36054dbf9fe..32b4399b41d 100644 --- a/storage/innobase/buf/buf0dblwr.cc +++ b/storage/innobase/buf/buf0dblwr.cc @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 1995, 2017, Oracle and/or its affiliates. All Rights Reserved. -Copyright (c) 2013, 2019, MariaDB Corporation. +Copyright (c) 2013, 2020, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -963,6 +963,7 @@ try_again: ib_int64_t sig_count = os_event_reset(buf_dblwr->b_event); mutex_exit(&buf_dblwr->mutex); + os_aio_simulated_wake_handler_threads(); os_event_wait_low(buf_dblwr->b_event, sig_count); goto try_again; } @@ -1096,6 +1097,7 @@ try_again: checkpoint. */ ib_int64_t sig_count = os_event_reset(buf_dblwr->b_event); mutex_exit(&buf_dblwr->mutex); + os_aio_simulated_wake_handler_threads(); os_event_wait_low(buf_dblwr->b_event, sig_count); goto try_again; diff --git a/storage/xtradb/buf/buf0dblwr.cc b/storage/xtradb/buf/buf0dblwr.cc index c3a169d45ef..5df40b5f4e8 100644 --- a/storage/xtradb/buf/buf0dblwr.cc +++ b/storage/xtradb/buf/buf0dblwr.cc @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 1995, 2017, Oracle and/or its affiliates. All Rights Reserved. -Copyright (c) 2013, 2019, MariaDB Corporation. +Copyright (c) 2013, 2020, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -963,6 +963,7 @@ try_again: ib_int64_t sig_count = os_event_reset(buf_dblwr->b_event); mutex_exit(&buf_dblwr->mutex); + os_aio_simulated_wake_handler_threads(); os_event_wait_low(buf_dblwr->b_event, sig_count); goto try_again; } @@ -1112,6 +1113,7 @@ try_again: checkpoint. */ ib_int64_t sig_count = os_event_reset(buf_dblwr->b_event); mutex_exit(&buf_dblwr->mutex); + os_aio_simulated_wake_handler_threads(); os_event_wait_low(buf_dblwr->b_event, sig_count); goto try_again;