Hi, Michael! On Mar 31, Michael Widenius wrote:
Yes, LEX_CSTRING *should* always be \0 terminated, but as we sometimes build these up I do not want to depend on that for generic usage. For usage to printf() than we should trust that the developer knows what he is doing.
However Item_string() will only use the exact length and never call printf, so here this is safe.
Okay. So, in effect, LEX_CSTRING is *not necessarily* \0 terminated. Fine.
diff --git a/sql/partition_info.cc b/sql/partition_info.cc <cut>
- DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s", + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s", part_def->part_id, part_def->is_subpart, - part_name)); + length, part_name));
These two changes contradict each other. Either part_name must be \0-terminated, and then you don't need to specify a length in printf. Or it is not \0-terminated and you must specify a length.
part_name is not null terminated and that is why I had to add .* and length. What is the contradiction ?
You've cut too much from my email. "contradiction" referred to the change just above this one, it was:
- @param part_name Partition name to match. + @param part_name Partition name to match. Must be \0 terminated!
so, please, decide what it is. Either the comment is wrong or the code is redundant.
@@ -1254,21 +1262,16 @@ bool String::append_semi_hex(const char *s, uint len, CHARSET_INFO *cs) // Shrink the buffer, but only if it is allocated on the heap. void Binary_string::shrink(size_t arg_length) { - if (!is_alloced()) - return; - if (ALIGN_SIZE(arg_length + 1) < Alloced_length) + if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length) + { + char *new_ptr; + /* my_realloc() can't fail as new buffer is less than the original one */ + if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length,
you don't need an if() because, as you wrote yourself "my_realloc() can't fail" here.
Yes, this should be true, but one never knows with allocation libraries. There is implementation of realloc that makes a copy and deletes the old one. However I am not sure what happens if there is no place to make a copy. Better safe than sorry.
No. I've fixed this quite a while ago. my_realloc() can *never* return NULL when reducing a buffer size. System realloc(), indeed, can, I've researched that topic when making the change. But my_realloc() has a protection against it and it never returns NULL in this case. We have few places in the code that rely on it.
+ { + Ptr[str_length]=0; + return Ptr; + } + (void) realloc(str_length+1); /* This will add end \0 */ return Ptr; } + /* Use c_ptr() instead. This will be deleted soon, kept for compatiblity */ inline char *c_ptr_quick() { - if (Ptr && str_length < Alloced_length) - Ptr[str_length]=0; - return Ptr; + return c_ptr_safe();
1. why not to remove it now? 2. it's strange that you write "use c_ptr() instead" but you actually use c_ptr_safe() instead.
What I meant is that the developer should instead use c_ptr(). I will make the message more clear. I decided to call c_ptr_safe() here as this is the most safe version to use. In practice the new c_ptr() should be safe for 99% of our code.
Let's remove c_ptr_quick() now. Why keep it? Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org