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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp