maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx