maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Sergei,

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



--
All the best,

Aleksey Midenkov
@midenok