Hi! On Thu, Apr 1, 2021 at 12:08 AM Sergei Golubchik <serg@mariadb.org> 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