Sergei, On Sat, Oct 31, 2020 at 12:54 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Oct 25, Aleksey Midenkov wrote:
On Wed, Oct 21, 2020 at 4:19 PM Sergei Golubchik <serg@mariadb.org>
wrote:
On Oct 20, Aleksey Midenkov wrote:
revision-id: 0cd5377e9ff (mariadb-10.2.31-357-g0cd5377e9ff) parent(s): dc716da4571 author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@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/2f9bbf392072361175c889cbf826f11c98...
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 security@mariadb.org
-- All the best, Aleksey Midenkov @midenok