maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Michael!

This is a good catch!

But note that LONGLONG_BUFFER_SIZE already reserves +1 for '\0', it's
defined as

/* Max length needed for a buffer to hold a longlong or ulonglong + end \0 */
#define LONGLONG_BUFFER_SIZE 21

The problem is that String::set_int doesn't use LONGLONG_BUFFER_SIZE, it uses
20*cs->mbmaxlen+1

This +1 is supposed to reserve one byte for '\0'. Which is incorrect,
because alloc() does it too, so a better fix would be to remove +1 from
String::set_int().

On Sep 08, Michael Widenius wrote:
> revision-id: 099596296ce (mariadb-10.5.2-272-g099596296ce)
> parent(s): 2de0c74f94c
> author: Michael Widenius <michael.widenius@xxxxxxxxx>
> committer: Michael Widenius <michael.widenius@xxxxxxxxx>
> timestamp: 2020-09-03 15:17:56 +0300
> message:
> 
> Avoid mallocs when using LONGLONG_BUFFER_SIZE
> 
> When using String::alloc() to allocate a string, the String ensures that
> there is space for an extra NULL byte in the buffer and if not, reallocates
> the string. This is a problem with the String::set_int() that calls
> alloc(LONGLONG_BUFFER_SIZE), which forces an extra malloc/free to
> happen.
> 
> Fixed by using LONGLONG_BUFFER_SIZE+1 for StringBuffer's.
> 
> diff --git a/sql/field.cc b/sql/field.cc
> index ab7254e483d..2be4da8d4a8 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -1085,7 +1085,7 @@ Field_longstr::make_packed_sort_key_part(uchar *buff,
>  uchar*
>  Field_longstr::pack_sort_string(uchar *to, const SORT_FIELD_ATTR *sort_field)
>  {
> -  StringBuffer<LONGLONG_BUFFER_SIZE> buf;
> +  StringBuffer<LONGLONG_BUFFER_SIZE+1> buf;
>    val_str(&buf, &buf);
>    return to + sort_field->pack_sort_string(to, &buf, field_charset());
>  }
> diff --git a/sql/item.cc b/sql/item.cc
> index 4a26cb6c140..0d947749e01 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -3612,7 +3612,7 @@ String *Item_int::val_str(String *str)
>  
>  void Item_int::print(String *str, enum_query_type query_type)
>  {
> -  StringBuffer<LONGLONG_BUFFER_SIZE> buf;
> +  StringBuffer<LONGLONG_BUFFER_SIZE+1> buf;
>    // my_charset_bin is good enough for numbers
>    buf.set_int(value, unsigned_flag, &my_charset_bin);
>    str->append(buf);
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx