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