Re: [Maria-developers] cf9bafc: MDEV-14815 - Server crash or AddressSanitizer errors or valgrind warnings
Hi, Sergey! On Oct 19, Sergey Vojtovich wrote:
revision-id: cf9bafc8cb0943c914d118525888140ea15e6b55 (mariadb-10.0.36-50-gcf9bafc) parent(s): 8e716138cef2d9be603cdf71701d82bcb72dfd69 author: Sergey Vojtovich committer: Sergey Vojtovich timestamp: 2018-10-18 23:50:50 +0400 message:
MDEV-14815 - Server crash or AddressSanitizer errors or valgrind warnings in thr_lock / has_old_lock upon FLUSH TABLES
Explicit partition access of partitioned MEMORY table under LOCK TABLES may cause subsequent statements to crash the server, deadlock, trigger valgrind warnings or ASAN errors. Freed memory was being used due to incorrect cleanup.
At least MyISAM and InnoDB don't seem to be affected, since their THR_LOCK structures don't survive FLUSH TABLES. MEMORY keeps table shared data (including THR_LOCK) even if there're no open instances.
There's partition_info::lock_partitions bitmap, which holds bits of partitions allowed to be accessed after pruning. This bitmap is updated for each individual statement.
This bitmap was abused in ha_partition::store_lock() such that when we need to unlock a table, locked by LOCK TABLES, only locks for partitions that were accessed by previous statement were released.
Eventually FLUSH TABLES frees THR_LOCK_DATA objects, which are still linked into THR_LOCK lists. When such THR_LOCK gets reused we end up with freed memory access.
Fixed by using ha_partition::m_locked_partitions bitmap similarly to ha_partition::external_lock().
Thanks, good comment.
Some unused code removed.
Generally, it's better to do that in a separate commit. It's very easy to do with git citool, and doesn't requite any advance planning, you just edit whatever you like and then run 'git citool' twice, selecting what lines you want to commit each time.
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index 0488ebf..02de92a 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -3907,9 +3907,14 @@ THR_LOCK_DATA **ha_partition::store_lock(THD *thd, } else { - for (i= bitmap_get_first_set(&(m_part_info->lock_partitions)); + MY_BITMAP *used_partitions= lock_type == TL_UNLOCK || + lock_type == TL_IGNORE ?
why TL_IGNORE too? external_lock only checks for TL_UNLOCK.
+ &m_locked_partitions : + &m_part_info->lock_partitions; + + for (i= bitmap_get_first_set(used_partitions); i < m_tot_parts; - i= bitmap_get_next_set(&m_part_info->lock_partitions, i)) + i= bitmap_get_next_set(used_partitions, i)) { DBUG_PRINT("info", ("store lock %d iteration", i)); to= m_file[i]->store_lock(thd, to, lock_type);
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, On Fri, Oct 19, 2018 at 04:13:16PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On Oct 19, Sergey Vojtovich wrote:
revision-id: cf9bafc8cb0943c914d118525888140ea15e6b55 (mariadb-10.0.36-50-gcf9bafc) parent(s): 8e716138cef2d9be603cdf71701d82bcb72dfd69 author: Sergey Vojtovich committer: Sergey Vojtovich timestamp: 2018-10-18 23:50:50 +0400 message:
MDEV-14815 - Server crash or AddressSanitizer errors or valgrind warnings in thr_lock / has_old_lock upon FLUSH TABLES
Explicit partition access of partitioned MEMORY table under LOCK TABLES may cause subsequent statements to crash the server, deadlock, trigger valgrind warnings or ASAN errors. Freed memory was being used due to incorrect cleanup.
At least MyISAM and InnoDB don't seem to be affected, since their THR_LOCK structures don't survive FLUSH TABLES. MEMORY keeps table shared data (including THR_LOCK) even if there're no open instances.
There's partition_info::lock_partitions bitmap, which holds bits of partitions allowed to be accessed after pruning. This bitmap is updated for each individual statement.
This bitmap was abused in ha_partition::store_lock() such that when we need to unlock a table, locked by LOCK TABLES, only locks for partitions that were accessed by previous statement were released.
Eventually FLUSH TABLES frees THR_LOCK_DATA objects, which are still linked into THR_LOCK lists. When such THR_LOCK gets reused we end up with freed memory access.
Fixed by using ha_partition::m_locked_partitions bitmap similarly to ha_partition::external_lock().
Thanks, good comment.
Some unused code removed.
Generally, it's better to do that in a separate commit.
It's very easy to do with git citool, and doesn't requite any advance planning, you just edit whatever you like and then run 'git citool' twice, selecting what lines you want to commit each time. Agree. I'll move this cleanup to a separate commit. Thanks for citool explanation, very useful.
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index 0488ebf..02de92a 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -3907,9 +3907,14 @@ THR_LOCK_DATA **ha_partition::store_lock(THD *thd, } else { - for (i= bitmap_get_first_set(&(m_part_info->lock_partitions)); + MY_BITMAP *used_partitions= lock_type == TL_UNLOCK || + lock_type == TL_IGNORE ?
why TL_IGNORE too? external_lock only checks for TL_UNLOCK.
external_lock() checks for F_UNLCK, which is different namespace. TL_IGNORE is something very hard to follow. In this particular case it is called exactly by FLUSH TABLES, when it collects list of locks for tables to be re-locked. FWICT we can't catch TL_IGNORE in this else branch otherwise. Thanks, Sergey
Hi, Sergey! On Oct 19, Sergey Vojtovich wrote:
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index 0488ebf..02de92a 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -3907,9 +3907,14 @@ THR_LOCK_DATA **ha_partition::store_lock(THD *thd, } else { - for (i= bitmap_get_first_set(&(m_part_info->lock_partitions)); + MY_BITMAP *used_partitions= lock_type == TL_UNLOCK || + lock_type == TL_IGNORE ?
why TL_IGNORE too? external_lock only checks for TL_UNLOCK. external_lock() checks for F_UNLCK, which is different namespace.
oops. indeed. my mistake, sorry.
TL_IGNORE is something very hard to follow. In this particular case it is called exactly by FLUSH TABLES, when it collects list of locks for tables to be re-locked. FWICT we can't catch TL_IGNORE in this else branch otherwise.
I see, thanks. Ok to push Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich