Hi!
On Thu, Apr 1, 2021 at 12:08 AM Sergei Golubchik <s...@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
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
