← Back to team overview

maria-developers team mailing list archive

Re: MDEV-9332 Bug after upgrade to 10.1.10


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)))
-  return to;
+  return to; // "from" was of types #a, #b, #e, or small #c.

Chief Architect MariaDB
and security@xxxxxxxxxxx