Sergei, On Sat, Oct 31, 2020 at 12:54 PM Sergei Golubchik <s...@mariadb.org> wrote: > > Hi, Aleksey! > > On Oct 25, Aleksey Midenkov wrote: > > On Wed, Oct 21, 2020 at 4:19 PM Sergei Golubchik <s...@mariadb.org> wrote: > > > On Oct 20, Aleksey Midenkov wrote: > > > > revision-id: 0cd5377e9ff (mariadb-10.2.31-357-g0cd5377e9ff) > > > > parent(s): dc716da4571 > > > > author: Aleksey Midenkov <mide...@gmail.com> > > > > committer: Aleksey Midenkov <mide...@gmail.com> > > > > timestamp: 2020-08-17 00:52:35 +0300 > > > > message: > > > > > > > > MDEV-18734 ASAN heap-use-after-free in my_strnxfrm_simple_internal upon update on versioned partitioned table > > > > > > No, I think we need to go deeper here. > > > The bug is not only not caused by system versioning, it is also not > > > caused by virtual columns. > > > > > > The problem is that partitioning copies record[0] out and then back > > > to save and restore values. This does not work for blobs, because > > > record[0] may store the pointer to the memory that Field_blob owns > > > and can free or reallocate as needed. > > > > > > Normally it's not the problem because typically after row reads the > > > engine owns the memory, not Field_blob. When a virtual column is set > > > to a calculated value - then Field_blob owns the memory and we have > > > this bug. > > > > Yes, you right... Actually engine owning blobs is not good too because > > engine does not guarantee to keep previous records blobs. > > I think engine owning blobs is safe. Because they belong to different > handlers, and every handler is pretty much guaranteed to keep the blob > for the last read row until the next row is read.
Agreed. > > > > My solution is to make a special mem_root (m_ordered_root) and copy > > the blobs to m_ordered_root when the record is stored into > > m_ordered_rec_buffer. That m_ordered_root is freed when > > m_ordered_rec_buffer is destroyed (m_ordered_rec_buffer can be > > allocated on m_ordered_root too). When the record is restored from > > m_ordered_rec_buffer the table fields are made to own blobs: we can > > destroy m_ordered_root any time and the table->record[0] pointers will > > still be valid. > > Just from your description it doesn't look like a good idea, > MEM_ROOT can be only freed as a whole, so you'll keep allocating memory > for blobs there and it'll keep growing, remembering all blobs you ever > had in the table. Right. I figured that out after I wrote the reply and made an additional patch with the array of mem_roots. > > > But I'll see the patch, perhaps I misunderstood what you're doing. > > If I'm right though, then I'd suggest to look only at the case when > Field_blob owns the memory. And to avoid copying blobs - they are big, > moving them back and forth is going to take time. A fast solution could > be to take the ownership of the blob memory from the Field_blob. And > then give it back. Using either String::swap or a pair of > String::release and String::reset - this won't need to allocate or > memcpy anything. This complicates things a little and makes the patch a bit less universal: we assert that only table->field can own and free the blob buffer otherwise it is safe. But ok, I made one more patch for your consideration: https://github.com/MariaDB/server/commits/2f9bbf392072361175c889cbf826f11c9880d678 > > > For example, every queue element has a `String backup`. It's empty, not > alloced. no buffer, nothing. When a Field_blob is put into a queue, you > backup::swap(Field_blob::value). When it's popped you swap again. This > should work even if the memory is owned by the engine and the > Field_blob::value is empty. Of course, there may be more than one blob, > so it should really be an array of backup strings. > > Regards, > Sergei > VP of MariaDB Server Engineering > 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
