← Back to team overview

maria-developers team mailing list archive

Re: 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@xxxxxxxxxxx


Follow ups