maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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.

> 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.

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.

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