maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,


On 01/26/2016 06:06 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> Looks good. Great comments.
> See my suggestions below.

Thanks! See replies below:

> On Jan 26, Alexander Barkov wrote:
> > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > index b14c3af..606e71d 100644
> > --- a/sql/sql_string.cc
> > +++ b/sql/sql_string.cc
> > @@ -789,21 +789,117 @@ int stringcmp(const String *s,const String *t)
> >   }

>> ...

> > +
> > +  if (from == to)
> > +  {
> > +    /*
> > +      Possible string types:
> > +      #a  not possible (constants should not be passed as "to")
> > +      #b  possible     (a fresh variable with no associated char buffer)
> > +      #c  possible     (a variable with a char buffer,
> > +                        in case it's smaller than fixed_length)
> > +      #d  not possible (handled earlier)
> > +      #e  not possible (constants should not be passed as "to")
> > +
> > +      If a string of types #a or #e appears here, that means the caller made
> > +      something wrong. Otherwise, it's safe to reallocate and return "to".
> > +    */
> > +    (void) to->realloc(from_length);
> > +    return to;
>
> 1. use 'from->realloc()' and 'return from' here
>     (a bit easier to read, because between 'to->realloc()' and 'return to'
>     one normally needs to copy 'from' to 'to'. Or one needs to read back to
>     remember that here from == to)

Agreed. I fixed to this:

    (void) from->realloc(from_length);
    return from;

> 2. please add asserts to make sure it's not #a or #e
>     (remember not to use assert(expr1 && expr2); use two asserts in such
>     a case)

Note, it seems it's not possible to distinguish between #a and #b,
and #b is valid here. So there's no a way to assert "not #a".

But "not #e" could be asserted. Something like this should do:

 DBUG_ASSERT(!from->alloced || from->Alloced_length > 0); // Not #e


> 3. sometimes you check the realloc's return value and sometimes you
>     don't. shall we be consistent here?

Yeah, I also noticed that in the original code,
and just preserved this behavior.

Perhaps we should return NULL on realloc() failures and fix all callers
to handle these cases.


> >     }
> >     if (to->realloc(from_length))
> >       return from;				// Actually an error
> >     if ((to->str_length=MY_MIN(from->str_length,from_length)))
> >       memcpy(to->Ptr,from->Ptr,to->str_length);
> >     to->str_charset=from->str_charset;
> > -  return to;
> > +  return to; // "from" was of types #a, #b, #e, or small #c.
> >   }
>
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx