maria-developers team mailing list archive

[Michael Widenius <michael.widenius@xxxxxxxxx>]

Hi!

On Thu, Apr 1, 2021 at 12:03 AM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

<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.

Neither. The comment is right. part_name must at this point in time be
null terminated, but
may not have to be that in the future.

The printf string for DBUG_PRINT is correct.  It uses the length,
which avoids a strlen() inside
sprintf() and is also future proof.

The only reason that part_name needs to be null terminated is because
of this code:

  if (!part_def)
  {
    my_error(ER_UNKNOWN_PARTITION, MYF(0), part_name, table->alias.c_ptr());
    DBUG_RETURN(true);
  }
Whenever we fix the error message to use %.*s, then we can lift the
restrictions that
part_name needs to be null terminated.

Regards,
Monty

> > > > @@ -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.

We also have code that relies on that my_malloc() never fails.
Note what this commit did was just an cleanup of the original code,
not trying to do any big changes of logic, except for c_ptr().

Anyway, I can remove the if here to keep you happy.

> > > > +    {
> > > > +      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?

Not enough time to do it now. I rather work on fixing review comments.

Regards,
Monty