maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Thu, Apr 1, 2021 at 12:08 AM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Monty!

<cut>

> Hmm, I don't understand. The difference between
>
>   String a("Hello", 5);
>
> and
>
>   char hello[5];
>   String a(buf, 5);
>
> is that in the first case the argument is const char*. And in that case
> Alloced_length=0 and in that case you can expect the string to be \0
> terminated.

There is no guarantee at all that when one has allocated length == 0 that
the string is 0 terminated.

I tried your suggestion and it did not work, we got
a lot of calls to malloc() that never gets freed.

Here is an example:

diff --git a/sql/sql_string.h b/sql/sql_string.h
index aa773ba396f..80c672c0137 100644
--- a/sql/sql_string.h
+++ b/sql/sql_string.h
@@ -609,7 +609,7 @@ class Binary_string: public Sql_alloc
       This is to handle the case of String("Hello",5) and
       String("hello",5) efficently.
     */
-    if (unlikely(!alloced && !Ptr[str_length]))
+    if (unlikely(!Alloced_length && !Ptr[str_length]))
       return Ptr;
     if (str_length < Alloced_length)

mtr main.grant
main.grant                               [ pass ]    943
***Warnings generated in error logs during shutdown after running
tests: main.grant
Warning: Memory not freed: 256

I traced it to this code, that is also in 10.5:
  String(const char *str, size_t len, CHARSET_INFO *cs)
   :Charset(cs),
    Binary_string((char *) str, len)
  { }
Removing the const caused Alloced_length to be set, which caused
the extra allocations.

After fixing this I was able to run the full test suite without issues.
Will try again with valgrind today.

> In your example that we do "a lot", the first argument is const char*,
> so String will have Alloced_length==0 and my suggestion will work
> exactly as planned, it'll use a \0 terminated fast path.

Yes, we have a lot, but we have also calls with not const that we have
to consider.

The current assumptions when using String and wanting a \0 terminated
string are:
- If you let String allocate the memory, c_ptr() will always be safe and fast.
- If you use your own buffer that you fill in and you assign it to a
String and you
  expect to use it for functions that expects \0 terminated strings, you should
  add the extra \0 yourself if you want c_ptr() to be fast and safe.
If not, then
  use c_ptr_safe().

The thing to consider is which option is better: to use 'alloced' or
Alloced_length.
alloced is a weaker test and will cover more cases and will this cause fewer
memory allocations. Alloced_length is probably a bit safer. In practice it looks
like with current code both works (I have in the past tested alloced with
valgrind and there has been no issues.

Regards,
Monty