Re: [Maria-developers] MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table
Hi Sergei, On Thu, Mar 21, 2019 at 10:24 PM Sergei Golubchik wrote: > Hi, Aleksey! > > On Mar 20, Aleksey Midenkov wrote: > > Hi Sergei! > > > > It turned out that only vcol blobs are affected. They are allocated > > by update_virtual_fields(), so it was enough to just refresh their value > by > > doing update_virtual_fields() again after record is restored from the > > queue. Please review the fix: > > > > https://github.com/MariaDB/server/pull/1234 > > > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > > index 8700610415cb..aa5a04af1dbc 100644 > > --- a/sql/ha_partition.cc > > +++ b/sql/ha_partition.cc > > @@ -6251,6 +6251,8 @@ void ha_partition::return_top_record(uchar *buf) > > > >part_id= uint2korr(key_buffer); > >memcpy(buf, rec_buffer, m_rec_length); > > + if (table->vfield && buf == table->record[0]) > > +table->update_virtual_fields(this, VCOL_UPDATE_FOR_READ); > >m_last_part= part_id; > >m_top_entry= part_id; > > } > > I suspect there's a problem with that still. > > With this fix you basically free and reallocate the blob for every > record. So for all blobs in the queue blob pointers might be invalid. > > But the queue might need them if it's sorted by the blob value. > Try this test case: > > create or replace table t1 (x int primary key, > b tinytext, v text as (concat(b)) virtual, > index (v(10)) > ) partition by range columns (x) ( > partition p1 values less than (4), > partition p2 values less than (6), > partition pn values less than (maxvalue)); > insert into t1 (x, b) values (1, 'q'), (2, 'z'), (4, 'a'), (5, 'b'), (6, > 'x'), (7,'y'); > select * from t1 where v > 'a'; > drop table t1; > > The above example did not reproduce anything for me. init_queue() is not called. If I add UPDATE from the task case, then init_queue() is called but it is sorted by 'x'. If I modify UPDATE to: update t1 set b= 'bar' where v > 'a' order by v limit 2; Then Bounded_queue is used instead of QUICK_RANGE_SELECT. > here there's an index on the virtual column (to tempt partitioning to > sort the queue by it) and an expression for a virtual column (to make > blob to allocate memory for it, and not just reuse the field's pointer). > > Here partitioning tries to sort the queue by the index, and fails. > If you remove concat from the virtual column definition, everything > works. > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org > -- All the best, Aleksey Midenkov @midenok ___ 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
Re: [Maria-developers] MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table
Hi, Aleksey! On Mar 20, Aleksey Midenkov wrote: > Hi Sergei! > > It turned out that only vcol blobs are affected. They are allocated > by update_virtual_fields(), so it was enough to just refresh their value by > doing update_virtual_fields() again after record is restored from the > queue. Please review the fix: > > https://github.com/MariaDB/server/pull/1234 > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 8700610415cb..aa5a04af1dbc 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -6251,6 +6251,8 @@ void ha_partition::return_top_record(uchar *buf) > >part_id= uint2korr(key_buffer); >memcpy(buf, rec_buffer, m_rec_length); > + if (table->vfield && buf == table->record[0]) > +table->update_virtual_fields(this, VCOL_UPDATE_FOR_READ); >m_last_part= part_id; >m_top_entry= part_id; > } I suspect there's a problem with that still. With this fix you basically free and reallocate the blob for every record. So for all blobs in the queue blob pointers might be invalid. But the queue might need them if it's sorted by the blob value. Try this test case: create or replace table t1 (x int primary key, b tinytext, v text as (concat(b)) virtual, index (v(10)) ) partition by range columns (x) ( partition p1 values less than (4), partition p2 values less than (6), partition pn values less than (maxvalue)); insert into t1 (x, b) values (1, 'q'), (2, 'z'), (4, 'a'), (5, 'b'), (6, 'x'), (7,'y'); select * from t1 where v > 'a'; drop table t1; here there's an index on the virtual column (to tempt partitioning to sort the queue by it) and an expression for a virtual column (to make blob to allocate memory for it, and not just reuse the field's pointer). Here partitioning tries to sort the queue by the index, and fails. If you remove concat from the virtual column definition, everything works. Regards, Sergei Chief Architect MariaDB and secur...@mariadb.org ___ 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
Re: [Maria-developers] MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table
Hi Sergei! It turned out that only vcol blobs are affected. They are allocated by update_virtual_fields(), so it was enough to just refresh their value by doing update_virtual_fields() again after record is restored from the queue. Please review the fix: https://github.com/MariaDB/server/pull/1234 On Tue, Mar 12, 2019 at 1:42 PM Sergei Golubchik wrote: > Hi, Aleksey! > > On Mar 11, Aleksey Midenkov wrote: > > Hi, Sergei! > > > > ha_partition::handle_ordered_index_scan() stores records in > > m_ordered_rec_buffer. Then TABLE::update_virtual_fields() updates blob > > buffer and frees the old one. Then ha_partition::return_top_record() > > returns record from m_ordered_rec_buffer with stale blob pointer. What > > should we do with this? I propose to duplicate blob buffer when record > > gets into m_ordered_rec_buffer. > > You can make Field_blob to forget that it owns the buffer. > And when reading back from the queue, Field_blob will need to take > over the buffer again. > > It's String::release() and String::reset() methods. > > The tricky part here is that Field_blob doesn't always own the buffer, > sometimes the storage engine does. So, when reading from the queue and > restoring the ownership you'll need to take care to do it only for > values that Field_blob used to own before, not for all blobs. > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org > -- All the best, Aleksey Midenkov @midenok ___ 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
Re: [Maria-developers] MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table
Hi, Aleksey! On Mar 11, Aleksey Midenkov wrote: > Hi, Sergei! > > ha_partition::handle_ordered_index_scan() stores records in > m_ordered_rec_buffer. Then TABLE::update_virtual_fields() updates blob > buffer and frees the old one. Then ha_partition::return_top_record() > returns record from m_ordered_rec_buffer with stale blob pointer. What > should we do with this? I propose to duplicate blob buffer when record > gets into m_ordered_rec_buffer. You can make Field_blob to forget that it owns the buffer. And when reading back from the queue, Field_blob will need to take over the buffer again. It's String::release() and String::reset() methods. The tricky part here is that Field_blob doesn't always own the buffer, sometimes the storage engine does. So, when reading from the queue and restoring the ownership you'll need to take care to do it only for values that Field_blob used to own before, not for all blobs. Regards, Sergei Chief Architect MariaDB and secur...@mariadb.org ___ 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
[Maria-developers] MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table
Hi, Sergei! ha_partition::handle_ordered_index_scan() stores records in m_ordered_rec_buffer. Then TABLE::update_virtual_fields() updates blob buffer and frees the old one. Then ha_partition::return_top_record() returns record from m_ordered_rec_buffer with stale blob pointer. What should we do with this? I propose to duplicate blob buffer when record gets into m_ordered_rec_buffer. More details can be found here: https://github.com/tempesta-tech/mariadb/issues/581 -- All the best, Aleksey Midenkov @midenok ___ 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
