From ea31b1e6ecfe8265b77e06b81f59d0ba3bcb0240 Mon Sep 17 00:00:00 2001 From: Michael Widenius Date: Tue, 19 Aug 2008 01:21:22 +0300 Subject: [PATCH] Fixes for Bug #38016 Maria: trying to access freed memory when committing a transaction Don't write out states if they haven't changed sql/sql_base.cc: Call extra(HA_EXTRA_PREPARE_FOR_DROP) before doing a drop of a table More DBUG sql/sql_table.cc: Call extra(HA_EXTRA_PREPARE_FOR_RENAME) before renaming a table storage/maria/ha_maria.cc: Ensure that file->trn is set when we call extra(HA_EXTRA_PREPARE_FOR_DROP/RENAME) storage/maria/ma_close.c: When doing close, assert if we have pointers in trn->table_list that points to the MARIA_SHARE storage/maria/ma_extra.c: Reset info->state_start in case of drop/rename. This fixes the problem of accessing freed memory in repair Don't write state changed if they haven't changed storage/maria/ma_open.c: Reset share->changed after we have written out a state (speed optimization to not write states when they haven't changed) storage/maria/ma_state.c: Decrement share->in_trans properly in DBUG_BINARY to ensure that the DBUG_ASSERT() in maria_close() works More DBUG --- sql/sql_base.cc | 6 ++++++ sql/sql_table.cc | 4 +++- storage/maria/ha_maria.cc | 18 ++++++++++++++++-- storage/maria/ma_close.c | 4 ++++ storage/maria/ma_extra.c | 4 ++-- storage/maria/ma_open.c | 1 + storage/maria/ma_state.c | 36 ++++++++++++++++++++++++++++++++---- 7 files changed, 64 insertions(+), 9 deletions(-) diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 2c2404ee97d..dbd5c833bd7 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -2184,6 +2184,7 @@ void unlink_open_table(THD *thd, TABLE *find, bool unlock) void drop_open_table(THD *thd, TABLE *table, const char *db_name, const char *table_name) { + DBUG_ENTER("drop_open_table"); if (table->s->tmp_table) close_temporary_table(thd, table, 1, 1); else @@ -2194,10 +2195,12 @@ void drop_open_table(THD *thd, TABLE *table, const char *db_name, unlink_open_table() also tells threads waiting for refresh or close that something has happened. */ + table->file->extra(HA_EXTRA_PREPARE_FOR_DROP); unlink_open_table(thd, table, FALSE); quick_rm_table(table_type, db_name, table_name, 0); VOID(pthread_mutex_unlock(&LOCK_open)); } + DBUG_VOID_RETURN; } @@ -3680,6 +3683,9 @@ TABLE *drop_locked_tables(THD *thd,const char *db, const char *table_name) if (!strcmp(table->s->table_name.str, table_name) && !strcmp(table->s->db.str, db)) { + /* Inform handler that table will be dropped after close */ + table->file->extra(HA_EXTRA_PREPARE_FOR_DROP); + /* If MERGE child, forward lock handling to parent. */ mysql_lock_remove(thd, thd->locked_tables, table->parent ? table->parent : table, TRUE); diff --git a/sql/sql_table.cc b/sql/sql_table.cc index d64b621a7db..b0275e3c860 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -7185,7 +7185,7 @@ err: if (errpos >= 3 && to->file->ha_end_bulk_insert(error > 1) && error <= 0) { to->file->print_error(my_errno,MYF(0)); - error=1; + error= 1; } to->file->extra(HA_EXTRA_NO_IGNORE_DUP_KEY); @@ -7208,6 +7208,8 @@ err: to->file->ha_release_auto_increment(); if (errpos >= 2 && to->file->ha_external_lock(thd,F_UNLCK)) error=1; + if (error < 0 && to->file->extra(HA_EXTRA_PREPARE_FOR_RENAME)) + error= 1; DBUG_RETURN(error > 0 ? -1 : 0); } diff --git a/storage/maria/ha_maria.cc b/storage/maria/ha_maria.cc index bb7e0985b46..3b3e06163a3 100644 --- a/storage/maria/ha_maria.cc +++ b/storage/maria/ha_maria.cc @@ -44,6 +44,7 @@ C_MODE_END #ifdef MARIA_CANNOT_ROLLBACK #define trans_register_ha(A, B, C) do { /* nothing */ } while(0) #endif +#define THD_TRN (*(TRN **)thd_ha_data(thd, maria_hton)) ulong pagecache_division_limit, pagecache_age_threshold; ulonglong pagecache_buffer_size; @@ -2201,6 +2202,21 @@ int ha_maria::extra(enum ha_extra_function operation) { if ((specialflag & SPECIAL_SAFE_MODE) && operation == HA_EXTRA_KEYREAD) return 0; + + /* + We have to set file->trn here because in some cases we call + extern_lock(F_UNLOCK) (which resets file->trn) followed by maria_close() + without calling commit/rollback in between. If file->trn is not set + we can't remove file->share from the transaction list in the extra() call. + */ + + if (!file->trn && + (operation == HA_EXTRA_PREPARE_FOR_DROP || + operation == HA_EXTRA_PREPARE_FOR_RENAME)) + { + THD *thd= table->in_use; + file->trn= THD_TRN; + } return maria_extra(file, operation, 0); } @@ -2240,8 +2256,6 @@ int ha_maria::delete_table(const char *name) return maria_delete_table(name); } -#define THD_TRN (*(TRN **)thd_ha_data(thd, maria_hton)) - int ha_maria::external_lock(THD *thd, int lock_type) { TRN *trn= THD_TRN; diff --git a/storage/maria/ma_close.c b/storage/maria/ma_close.c index c520fdccaa7..5f34b167dfb 100644 --- a/storage/maria/ma_close.c +++ b/storage/maria/ma_close.c @@ -69,6 +69,10 @@ int maria_close(register MARIA_HA *info) if (flag) { /* Last close of file; Flush everything */ + + /* Check that we don't have any dangling pointers from the transaction */ + DBUG_ASSERT(share->in_trans == 0); + if (share->kfile.file >= 0) { if ((*share->once_end)(share)) diff --git a/storage/maria/ma_extra.c b/storage/maria/ma_extra.c index 8c4f44df91c..7fab28edf42 100644 --- a/storage/maria/ma_extra.c +++ b/storage/maria/ma_extra.c @@ -326,7 +326,7 @@ int maria_extra(MARIA_HA *info, enum ha_extra_function function, { _ma_remove_table_from_trnman(share, info->trn); /* Ensure we don't point to the deleted data in trn */ - info->state= &share->state.state; + info->state= info->state_start= &share->state.state; } type= do_flush ? FLUSH_RELEASE : FLUSH_IGNORE_CHANGED; @@ -347,7 +347,7 @@ int maria_extra(MARIA_HA *info, enum ha_extra_function function, if (do_flush) { /* Save the state so that others can find it from disk. */ - if (_ma_state_info_write(share, 1 | 2) || + if ((share->changed && _ma_state_info_write(share, 1 | 2)) || my_sync(share->kfile.file, MYF(0))) error= my_errno; else diff --git a/storage/maria/ma_open.c b/storage/maria/ma_open.c index e35b5eaac86..04ad99a56ca 100644 --- a/storage/maria/ma_open.c +++ b/storage/maria/ma_open.c @@ -1186,6 +1186,7 @@ uint _ma_state_info_write(MARIA_SHARE *share, uint pWrite) res= _ma_state_info_write_sub(share->kfile.file, &share->state, pWrite); if (pWrite & 4) pthread_mutex_unlock(&share->intern_lock); + share->changed= 0; return res; } diff --git a/storage/maria/ma_state.c b/storage/maria/ma_state.c index ec8ea2010ae..b18fad4a9b1 100644 --- a/storage/maria/ma_state.c +++ b/storage/maria/ma_state.c @@ -79,6 +79,9 @@ my_bool _ma_setup_live_state(MARIA_HA *info) pthread_mutex_lock(&share->intern_lock); share->in_trans++; + DBUG_PRINT("info", ("share: 0x%lx in_trans: %d", + (ulong) share, share->in_trans)); + history= share->state_history; /* @@ -359,15 +362,16 @@ my_bool _ma_trnman_end_trans_hook(TRN *trn, my_bool commit, { my_bool error= 0; MARIA_USED_TABLES *tables, *next; + DBUG_ENTER("_ma_trnman_end_trans_hook"); for (tables= (MARIA_USED_TABLES*) trn->used_tables; tables; tables= next) { + MARIA_SHARE *share= tables->share; next= tables->next; if (commit) { - MARIA_SHARE *share= tables->share; MARIA_STATE_HISTORY *history; pthread_mutex_lock(&share->intern_lock); @@ -405,13 +409,27 @@ my_bool _ma_trnman_end_trans_hook(TRN *trn, my_bool commit, /* Remove not visible states */ share->state_history= _ma_remove_not_visible_states(history, 0, 1); } + DBUG_PRINT("info", ("share: 0x%lx in_trans: %d", + (ulong) share, share->in_trans)); share->in_trans--; pthread_mutex_unlock(&share->intern_lock); } + else + { +#ifndef DBUG_OFF + /* + We need to keep share->in_trans correct in the debug library + because of the assert in maria_close() + */ + pthread_mutex_lock(&share->intern_lock); + share->in_trans--; + pthread_mutex_unlock(&share->intern_lock); +#endif + } my_free(tables, MYF(0)); } trn->used_tables= 0; - return error; + DBUG_RETURN(error); } @@ -421,11 +439,18 @@ my_bool _ma_trnman_end_trans_hook(TRN *trn, my_bool commit, @notes This is used when we unlock a table from a group of locked tables just before doing a rename or drop table. + + share->internal_lock must be locked when function is called */ void _ma_remove_table_from_trnman(MARIA_SHARE *share, TRN *trn) { MARIA_USED_TABLES *tables, **prev; + DBUG_ENTER("_ma_remove_table_from_trnman"); + DBUG_PRINT("enter", ("share: 0x%lx in_trans: %d", + (ulong) share, share->in_trans)); + + safe_mutex_assert_owner(&share->intern_lock); for (prev= (MARIA_USED_TABLES**) &trn->used_tables, tables= *prev; tables; @@ -434,11 +459,14 @@ void _ma_remove_table_from_trnman(MARIA_SHARE *share, TRN *trn) if (tables->share == share) { *prev= tables->next; + share->in_trans--; + DBUG_PRINT("info", ("in_trans: %d", share->in_trans)); my_free(tables, MYF(0)); + break; } - else - prev= &tables->next; + prev= &tables->next; } + DBUG_VOID_RETURN; }