Re: [Maria-developers] c34533186ab: MDEV-18734 ASAN heap-use-after-free upon sorting by blob column from partitioned table
Hi, Aleksey! As before, despite the email subject, it's git diff dfa2d0bc13..e533b5fb307 Don't be confused :) On Jul 21, Aleksey Midenkov wrote:
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index c53732a2b51..70ed14448ea 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -5130,7 +5135,18 @@ bool ha_partition::init_record_priority_queue() i= bitmap_get_next_set(&m_part_info->read_partitions, i)) { DBUG_PRINT("info", ("init rec-buf for part %u", i)); - int2store(ptr, i); + if (table->s->blob_fields) + { + for (uint j= 0; j < table->s->blob_fields; ++j, ++objs) + { + blob_storage[j]= new (objs) Ordered_blob_storage; + if (!blob_storage[j]) + DBUG_RETURN(true);
is this possible?
+ } + *((Ordered_blob_storage ***) ptr)= blob_storage; + blob_storage+= table->s->blob_fields; + } + int2store(ptr + sizeof(String **), i); ptr+= m_priority_queue_rec_len; } m_start_key.key= (const uchar*)ptr; @@ -6291,6 +6333,43 @@ int ha_partition::handle_ordered_index_scan_key_not_found() }
+void ha_partition::swap_blobs(uchar * rec_buf, Ordered_blob_storage ** storage, bool restore) +{ + uint *ptr, *end; + uint blob_n= 0; + table->move_fields(table->field, rec_buf, table->record[0]); + for (ptr= table->s->blob_field, end= ptr + table->s->blob_fields; + ptr != end; ++ptr, ++blob_n) + { + DBUG_ASSERT(*ptr < table->s->fields); + Field_blob *blob= (Field_blob*) table->field[*ptr]; + DBUG_ASSERT(blob->flags & BLOB_FLAG); + DBUG_ASSERT(blob->field_index == *ptr); + if (!bitmap_is_set(table->read_set, *ptr) || blob->is_null()) + continue; + + Ordered_blob_storage &s= *storage[blob_n]; + + if (restore) + { + if (!s.blob.is_empty()) + blob->swap(s.blob, s.set_read_value);
don't you need here something like else blob->reset(); to make blob empty?
+ } + else + { + bool set_read_value; + String *cached= blob->cached(set_read_value); + if (cached) + { + cached->swap(s.blob); + s.set_read_value= set_read_value; + } + } + } + table->move_fields(table->field, table->record[0], rec_buf);
On swap_blobs() you always move all fields from rec_buf to table->record[0]? Why? This looks very suspicious.
+} + + /* Common routine to handle index_next with ordered results
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! Now updated the patch to git diff dfa2d0bc13..9b564832e3 On Wed, Jul 21, 2021 at 11:33 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
As before, despite the email subject, it's
git diff dfa2d0bc13..e533b5fb307
Don't be confused :)
On Jul 21, Aleksey Midenkov wrote:
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index c53732a2b51..70ed14448ea 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -5130,7 +5135,18 @@ bool ha_partition::init_record_priority_queue() i= bitmap_get_next_set(&m_part_info->read_partitions, i)) { DBUG_PRINT("info", ("init rec-buf for part %u", i)); - int2store(ptr, i); + if (table->s->blob_fields) + { + for (uint j= 0; j < table->s->blob_fields; ++j, ++objs) + { + blob_storage[j]= new (objs) Ordered_blob_storage; + if (!blob_storage[j]) + DBUG_RETURN(true);
is this possible?
If we don't use exceptions we should handle OOM this way. If we can't use -fno-exceptions we should make a wrapper by overloading default new() and catching std::badalloc.
+ } + *((Ordered_blob_storage ***) ptr)= blob_storage; + blob_storage+= table->s->blob_fields; + } + int2store(ptr + sizeof(String **), i); ptr+= m_priority_queue_rec_len; } m_start_key.key= (const uchar*)ptr; @@ -6291,6 +6333,43 @@ int ha_partition::handle_ordered_index_scan_key_not_found() }
+void ha_partition::swap_blobs(uchar * rec_buf, Ordered_blob_storage ** storage, bool restore) +{ + uint *ptr, *end; + uint blob_n= 0; + table->move_fields(table->field, rec_buf, table->record[0]); + for (ptr= table->s->blob_field, end= ptr + table->s->blob_fields; + ptr != end; ++ptr, ++blob_n) + { + DBUG_ASSERT(*ptr < table->s->fields); + Field_blob *blob= (Field_blob*) table->field[*ptr]; + DBUG_ASSERT(blob->flags & BLOB_FLAG); + DBUG_ASSERT(blob->field_index == *ptr); + if (!bitmap_is_set(table->read_set, *ptr) || blob->is_null()) + continue; + + Ordered_blob_storage &s= *storage[blob_n]; + + if (restore) + { + if (!s.blob.is_empty()) + blob->swap(s.blob, s.set_read_value);
don't you need here something like
else blob->reset();
to make blob empty?
I believe no. We protect only blob cache (value or read_value). If the cache was empty that doesn't mean the blob was empty. AFAIR blobs allocated by a storage engine should work just fine. Added a comment about that.
+ } + else + { + bool set_read_value; + String *cached= blob->cached(set_read_value); + if (cached) + { + cached->swap(s.blob); + s.set_read_value= set_read_value; + } + } + } + table->move_fields(table->field, table->record[0], rec_buf);
On swap_blobs() you always move all fields from rec_buf to table->record[0]? Why? This looks very suspicious.
Have you seen the first move_fields() call? We work on rec_buf and if it is the same as record[0] move_fields() does nothing.
+} + + /* Common routine to handle index_next with ordered results
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Jul 23, Aleksey Midenkov wrote:
Now updated the patch to
git diff dfa2d0bc13..9b564832e3
Thanks, no new comments about it. Just one question below still
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index c53732a2b51..70ed14448ea 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -5130,7 +5135,18 @@ bool ha_partition::init_record_priority_queue() i= bitmap_get_next_set(&m_part_info->read_partitions, i)) { DBUG_PRINT("info", ("init rec-buf for part %u", i)); - int2store(ptr, i); + if (table->s->blob_fields) + { + for (uint j= 0; j < table->s->blob_fields; ++j, ++objs) + { + blob_storage[j]= new (objs) Ordered_blob_storage; + if (!blob_storage[j]) + DBUG_RETURN(true);
is this possible?
If we don't use exceptions we should handle OOM this way. If we can't use -fno-exceptions we should make a wrapper by overloading default new() and catching std::badalloc.
I mean, it's a placement new, it doesn't allocate anything. How can it return NULL if it simply returns objs? ...
+ else + { + bool set_read_value; + String *cached= blob->cached(set_read_value); + if (cached) + { + cached->swap(s.blob); + s.set_read_value= set_read_value; + } + } + } + table->move_fields(table->field, table->record[0], rec_buf);
On swap_blobs() you always move all fields from rec_buf to table->record[0]? Why? This looks very suspicious.
Have you seen the first move_fields() call? We work on rec_buf and if it is the same as record[0] move_fields() does nothing.
Oops, no. I missed the first one. And it was very suspicious that you always move in one direction. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Mon, Jul 26, 2021 at 7:35 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Jul 23, Aleksey Midenkov wrote:
Now updated the patch to
git diff dfa2d0bc13..9b564832e3
Thanks, no new comments about it. Just one question below still
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index c53732a2b51..70ed14448ea 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -5130,7 +5135,18 @@ bool ha_partition::init_record_priority_queue() i= bitmap_get_next_set(&m_part_info->read_partitions, i)) { DBUG_PRINT("info", ("init rec-buf for part %u", i)); - int2store(ptr, i); + if (table->s->blob_fields) + { + for (uint j= 0; j < table->s->blob_fields; ++j, ++objs) + { + blob_storage[j]= new (objs) Ordered_blob_storage; + if (!blob_storage[j]) + DBUG_RETURN(true);
is this possible?
If we don't use exceptions we should handle OOM this way. If we can't use -fno-exceptions we should make a wrapper by overloading default new() and catching std::badalloc.
I mean, it's a placement new, it doesn't allocate anything. How can it return NULL if it simply returns objs?
Oh, right! Updated that.
...
+ else + { + bool set_read_value; + String *cached= blob->cached(set_read_value); + if (cached) + { + cached->swap(s.blob); + s.set_read_value= set_read_value; + } + } + } + table->move_fields(table->field, table->record[0], rec_buf);
On swap_blobs() you always move all fields from rec_buf to table->record[0]? Why? This looks very suspicious.
Have you seen the first move_fields() call? We work on rec_buf and if it is the same as record[0] move_fields() does nothing.
Oops, no. I missed the first one. And it was very suspicious that you always move in one direction.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik