maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

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