MDEV-14454 Binary protocol returns wrong collation ID for SP OUT parameters

Item_param::set_value() did not set Item::collation and
Item_param::str_value_ptr.str_charset properly. So both
metadata and data for OUT parameters were sent in a wrong
way to the client.

This patch removes the old implementation of Item_param::set_value()
and rewrites it using Type_handler::Item_param_set_from_value(),
so now setting IN and OUT parameters share the a lot of code.

1. Item_param::set_str() now:
  - accepts two additional parameters fromcs, tocs
  - sets str_value_ptr, to make sure it's always in sync with str_value,
    even without Item_param::convert_str_value()
  - does collation.set(tocs, DERIVATION_COERCIBLE),
    to make sure that DTCollation is valid even without
    Item_param::convert_str_value()

2. Item_param::set_value(), which is used to set OUT parameters,
   now reuses Type_handler::Item_param_set_from_value().

3. Cleanup: moving Item_param::str_value_ptr to private,
   as it's not needed outside.

4. Cleanup: adding a new virtual method
   Settable_routine_parameter::get_item_param()
   and using it a few new DBUG_ASSERTs, where
   Item_param cannot appear.

After this change:
1. Assigning of IN parameters works as before:
a. Item_param::set_str() is called and sets the value as a binary string
b. The original value is sent to the query used for binary/general logging
c. Item_param::convert_str_value() converts the value from the client
   character set to the connection character set

2. Assigning of OUT parameters works in the new way:
a. Item_param::set_str() and sets the value
   using the source Item's collation, so both Item::collation
   and Item_param::str_value_ptr.str_charset are properly set.
b. Protocol_binary::send_out_parameters() sends the
   value to the client correctly:
   - Protocol::send_result_set_metadata() uses Item::collation.collation
     (which is now properly set), to detect if conversion is needed,
     and sends a correct collation ID.
   - Protocol::send_result_set_row() calls Type_handler::Item_send_str(),
     which uses Item_param::str_value_ptr.str_charset
     (which is now properly set) to actually perform the conversion.

mysql-test/r/ps.result

@@ -3550,7 +3550,7 @@ CREATE TEMPORARY TABLE tmp1 AS SELECT @a AS c1;
 SHOW CREATE TABLE tmp1;
 Table	Create Table
 tmp1	CREATE TEMPORARY TABLE `tmp1` (
-  `c1` longblob DEFAULT NULL
+  `c1` longtext DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 SELECT @a, @a = REPEAT('a', 16);
 @a	@a = REPEAT('a', 16)
@@ -3568,7 +3568,7 @@ CREATE TEMPORARY TABLE tmp1 AS SELECT @a AS c1;
 SHOW CREATE TABLE tmp1;
 Table	Create Table
 tmp1	CREATE TEMPORARY TABLE `tmp1` (
-  `c1` longblob DEFAULT NULL
+  `c1` longtext DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 SELECT @a, @a = REPEAT('b', 16);
 @a	@a = REPEAT('b', 16)
@@ -3586,7 +3586,7 @@ CREATE TEMPORARY TABLE tmp1 AS SELECT @a AS c1;
 SHOW CREATE TABLE tmp1;
 Table	Create Table
 tmp1	CREATE TEMPORARY TABLE `tmp1` (
-  `c1` longblob DEFAULT NULL
+  `c1` longtext DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 SELECT @a, @a = REPEAT('c', 16);
 @a	@a = REPEAT('c', 16)
@@ -3604,7 +3604,7 @@ CREATE TEMPORARY TABLE tmp1 AS SELECT @a AS c1;
 SHOW CREATE TABLE tmp1;
 Table	Create Table
 tmp1	CREATE TEMPORARY TABLE `tmp1` (
-  `c1` longblob DEFAULT NULL
+  `c1` longtext DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 SELECT @a, @a = REPEAT('d', 16);
 @a	@a = REPEAT('d', 16)
@@ -3622,7 +3622,7 @@ CREATE TEMPORARY TABLE tmp1 AS SELECT @a AS c1;
 SHOW CREATE TABLE tmp1;
 Table	Create Table
 tmp1	CREATE TEMPORARY TABLE `tmp1` (
-  `c1` longblob DEFAULT NULL
+  `c1` longtext DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 SELECT @a, @a = REPEAT('e', 16);
 @a	@a = REPEAT('e', 16)
@@ -3640,7 +3640,7 @@ CREATE TEMPORARY TABLE tmp1 AS SELECT @a AS c1;
 SHOW CREATE TABLE tmp1;
 Table	Create Table
 tmp1	CREATE TEMPORARY TABLE `tmp1` (
-  `c1` longblob DEFAULT NULL
+  `c1` longtext DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 SELECT @a, @a = REPEAT('f', 16);
 @a	@a = REPEAT('f', 16)
@@ -3766,7 +3766,7 @@ CREATE TEMPORARY TABLE tmp1 AS SELECT @a AS c1;
 SHOW CREATE TABLE tmp1;
 Table	Create Table
 tmp1	CREATE TEMPORARY TABLE `tmp1` (
-  `c1` longblob DEFAULT NULL
+  `c1` longtext DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 SELECT @a, @a = 'aaa';
 @a	@a = 'aaa'
@@ -3784,7 +3784,7 @@ CREATE TEMPORARY TABLE tmp1 AS SELECT @a AS c1;
 SHOW CREATE TABLE tmp1;
 Table	Create Table
 tmp1	CREATE TEMPORARY TABLE `tmp1` (
-  `c1` longblob DEFAULT NULL
+  `c1` longtext DEFAULT NULL
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 SELECT @a, @a = 'aaa';
 @a	@a = 'aaa'
@@ -5074,3 +5074,17 @@ t1	CREATE TABLE `t1` (
 ) ENGINE=MyISAM DEFAULT CHARSET=latin1
 DROP TABLE t1;
 DROP PROCEDURE p1;
+#
+# MDEV-14454 Binary protocol returns wrong collation ID for SP OUT parameters
+#
+CREATE PROCEDURE p1(OUT v CHAR(32) CHARACTER SET utf8) SET v='aaa';
+PREPARE stmt1 FROM 'CALL p1(?)';
+EXECUTE stmt1 USING @a;
+CREATE TABLE t1 AS SELECT @a AS c1;
+SHOW CREATE TABLE t1;
+Table	Create Table
+t1	CREATE TABLE `t1` (
+  `c1` longtext CHARACTER SET utf8 DEFAULT NULL
+) ENGINE=MyISAM DEFAULT CHARSET=latin1
+DROP TABLE t1;
+DROP PROCEDURE p1;

mysql-test/t/ps.test

@@ -4529,3 +4529,15 @@ CREATE TABLE t1 AS SELECT @a AS a, @b AS b;
 SHOW CREATE TABLE t1;
 DROP TABLE t1;
 DROP PROCEDURE p1;
+
+--echo #
+--echo # MDEV-14454 Binary protocol returns wrong collation ID for SP OUT parameters
+--echo #
+
+CREATE PROCEDURE p1(OUT v CHAR(32) CHARACTER SET utf8) SET v='aaa';
+PREPARE stmt1 FROM 'CALL p1(?)';
+EXECUTE stmt1 USING @a;
+CREATE TABLE t1 AS SELECT @a AS c1;
+SHOW CREATE TABLE t1;
+DROP TABLE t1;
+DROP PROCEDURE p1;

sql/item.cc

@@ -3903,7 +3903,8 @@ void Item_param::set_time(MYSQL_TIME *tm, timestamp_type time_type,
 }
 
 
-bool Item_param::set_str(const char *str, ulong length)
+bool Item_param::set_str(const char *str, ulong length,
+                         CHARSET_INFO *fromcs, CHARSET_INFO *tocs)
 {
   DBUG_ENTER("Item_param::set_str");
   /*
@@ -3911,10 +3912,24 @@ bool Item_param::set_str(const char *str, ulong length)
     been written to the binary log.
   */
   uint dummy_errors;
-  if (str_value.copy(str, length, &my_charset_bin, &my_charset_bin,
-                     &dummy_errors))
+  if (str_value.copy(str, length, fromcs, tocs, &dummy_errors))
     DBUG_RETURN(TRUE);
+  /*
+    Set str_value_ptr to make sure it's in sync with str_value.
+    This is needed in case if we're called from Item_param::set_value(),
+    from the code responsible for setting OUT parameters in
+    sp_head::execute_procedure(). This makes sure that
+    Protocol_binary::send_out_parameters() later gets a valid value
+    from Item_param::val_str().
+    Note, for IN parameters, Item_param::convert_str_value() will be called
+    later, which will convert the value from the client character set to the
+    connection character set, and will reset both str_value and str_value_ptr.
+  */
+  str_value_ptr.set(str_value.ptr(),
+                    str_value.length(),
+                    str_value.charset());
   state= STRING_VALUE;
+  collation.set(tocs, DERIVATION_COERCIBLE);
   max_length= length;
   maybe_null= 0;
   /* max_length and decimals are set after charset conversion */
@@ -4576,66 +4591,14 @@ bool
 Item_param::set_value(THD *thd, sp_rcontext *ctx, Item **it)
 {
   Item *arg= *it;
-
-  if (arg->is_null())
+  struct st_value tmp;
+  if (arg->save_in_value(&tmp) ||
+      arg->type_handler()->Item_param_set_from_value(thd, this, arg, &tmp))
   {
     set_null();
-    return FALSE;
-  }
-
-  null_value= FALSE;
-  unsigned_flag= arg->unsigned_flag;
-
-  switch (arg->result_type()) {
-  case STRING_RESULT:
-  {
-    char str_buffer[STRING_BUFFER_USUAL_SIZE];
-    String sv_buffer(str_buffer, sizeof(str_buffer), &my_charset_bin);
-    String *sv= arg->val_str(&sv_buffer);
-
-    if (!sv)
-      return TRUE;
-
-    set_str(sv->c_ptr_safe(), sv->length());
-    str_value_ptr.set(str_value.ptr(),
-                      str_value.length(),
-                      str_value.charset());
-    collation.set(str_value.charset(), DERIVATION_COERCIBLE);
-    decimals= 0;
-    break;
-  }
-
-  case REAL_RESULT:
-    set_double(arg->val_real());
-    break;
-
-  case INT_RESULT:
-    set_int(arg->val_int(), arg->max_length);
-    break;
-
-  case DECIMAL_RESULT:
-  {
-    my_decimal dv_buf;
-    my_decimal *dv= arg->val_decimal(&dv_buf);
-
-    if (!dv)
-      return TRUE;
-
-    set_decimal(dv, !dv->sign());
-    break;
-  }
-
-  default:
-    /* That can not happen. */
-
-    DBUG_ASSERT(TRUE);  // Abort in debug mode.
-
-    set_null();         // Set to NULL in release mode.
-    return FALSE;
+    return false;
   }
-
-  set_handler_by_result_type(arg->result_type());
-  return FALSE;
+  return null_value= false;
 }
 
 

sql/item.h

@@ -391,6 +391,8 @@ class Settable_routine_parameter
 
   virtual const Send_field *get_out_param_info() const
   { return NULL; }
+
+  virtual Item_param *get_item_param() { return 0; }
 };
 
 
@@ -3173,6 +3175,7 @@ class Item_param :public Item_basic_value,
   */
   enum enum_indicator_type indicator;
 
+private:
   /*
     A buffer for string and long data values. Historically all allocated
     values returned from val_str() were treated as eligible to
@@ -3184,6 +3187,8 @@ class Item_param :public Item_basic_value,
     Can not be declared inside the union as it's not a POD type.
   */
   String str_value_ptr;
+
+public:
   my_decimal decimal_value;
   union
   {
@@ -3225,7 +3230,8 @@ class Item_param :public Item_basic_value,
   void set_double(double i);
   void set_decimal(const char *str, ulong length);
   void set_decimal(const my_decimal *dv, bool unsigned_arg);
-  bool set_str(const char *str, ulong length);
+  bool set_str(const char *str, ulong length,
+               CHARSET_INFO *fromcs, CHARSET_INFO *tocs);
   bool set_longdata(const char *str, ulong length);
   void set_time(MYSQL_TIME *tm, timestamp_type type, uint32 max_length_arg);
   void set_time(const MYSQL_TIME *tm, uint32 max_length_arg, uint decimals_arg);
@@ -3305,6 +3311,8 @@ class Item_param :public Item_basic_value,
 public:
   virtual const Send_field *get_out_param_info() const;
 
+  Item_param *get_item_param() { return this; }
+
   virtual void make_field(THD *thd, Send_field *field);
 
 private:

sql/protocol.cc

@@ -1327,6 +1327,7 @@ bool Protocol_text::send_out_parameters(List<Item_param> *sp_params)
       continue;
     }
 
+    DBUG_ASSERT(sparam->get_item_param() == NULL);
     sparam->set_value(thd, thd->spcont, reinterpret_cast<Item **>(&item_param));
   }
 

sql/sql_get_diagnostics.cc

@@ -109,6 +109,9 @@ Diagnostics_information_item::set_value(THD *thd, Item **value)
 
   DBUG_ASSERT(srp);
 
+  /* GET DIAGNOSTICS is not allowed in prepared statements */
+  DBUG_ASSERT(srp->get_item_param() == NULL);
+
   /* Set variable/parameter value. */
   rv= srp->set_value(thd, thd->spcont, value);
 

sql/sql_prepare.cc

@@ -735,7 +735,13 @@ static void set_param_str_or_null(Item_param *param, uchar **pos, ulong len,
   {
     if (length > len)
       length= len;
-    param->set_str((const char *) *pos, length);
+    /*
+      We use &my_charset_bin here. Conversion and setting real character
+      sets will be done in Item_param::convert_str_value(), after the
+      original value is appended to the query used for logging.
+    */
+    param->set_str((const char *) *pos, length,
+                   &my_charset_bin, &my_charset_bin);
     *pos+= length;
   }
 }

sql/sql_type.cc

@@ -5059,7 +5059,9 @@ bool Type_handler_string_result::
     charset of connection, so we have to set it later.
   */
   param->set_handler(&type_handler_varchar);
-  return param->set_str(val->m_string.ptr(), val->m_string.length());
+  return param->set_str(val->m_string.ptr(), val->m_string.length(),
+                        attr->collation.collation,
+                        attr->collation.collation);
 }
 
 
@@ -5087,7 +5089,8 @@ bool Type_handler_geometry::
   param->value.cs_info.set(thd, &my_charset_bin);
   param->set_handler(&type_handler_geometry);
   param->set_geometry_type(attr->uint_geometry_type());
-  return param->set_str(val->m_string.ptr(), val->m_string.length());
+  return param->set_str(val->m_string.ptr(), val->m_string.length(),
+                        &my_charset_bin, &my_charset_bin);
 }
 #endif
 

tests/mysql_client_fw.c

@@ -1177,6 +1177,15 @@ static my_bool thread_query(const char *query)
 }
 
 
+static int mysql_query_or_error(MYSQL *mysql, const char *query)
+{
+  int rc= mysql_query(mysql, query);
+  if (rc)
+    fprintf(stderr, "ERROR %d: %s", mysql_errno(mysql), mysql_error(mysql));
+  return rc;
+}
+
+
 /*
   Read and parse arguments and MySQL options from my.cnf
 */

tests/mysql_client_test.c

@@ -19858,6 +19858,85 @@ static void test_mdev14013_1()
 }
 
 
+static void test_mdev14454_internal(const char *init,
+                                    unsigned int csid,
+                                    const char *value)
+{
+  MYSQL_STMT *stmt;
+  MYSQL_BIND bind;
+  const char *stmtstr= "CALL P1(?)";
+  char res[20];
+  int rc;
+
+  if ((rc= mysql_query_or_error(mysql, init)) ||
+      (rc= mysql_query_or_error(mysql, "DROP PROCEDURE IF EXISTS p1")) ||
+      (rc= mysql_query_or_error(mysql,
+                               "CREATE PROCEDURE p1"
+                               "("
+                               "  OUT param1 TEXT CHARACTER SET utf8"
+                               ")"
+                               "BEGIN "
+                               "  SET param1 = _latin1'test\xFF'; "
+                               "END")))
+    DIE("Initiation failed");
+
+  stmt= mysql_stmt_init(mysql);
+  rc= mysql_stmt_prepare(stmt, stmtstr, strlen(stmtstr));
+  DIE_UNLESS(rc == 0);
+  DIE_UNLESS(mysql_stmt_param_count(stmt) == 1);
+
+  bind.buffer_type= MYSQL_TYPE_NULL;
+  rc= mysql_stmt_bind_param(stmt, &bind);
+  DIE_UNLESS(rc == 0);
+
+  rc= mysql_stmt_execute(stmt);
+  DIE_UNLESS(rc == 0);
+
+  memset(res, 0, sizeof(res));
+  memset(&bind, 0, sizeof(bind));
+  bind.buffer_type= MYSQL_TYPE_STRING;
+  bind.buffer_length= sizeof(res);
+  bind.buffer= res;
+
+  do {
+    if (mysql->server_status & SERVER_PS_OUT_PARAMS)
+    {
+      MYSQL_FIELD *field;
+      printf("\nOUT param result set:\n");
+      DIE_UNLESS(mysql_stmt_field_count(stmt) == 1);
+      field= &stmt->fields[0];
+      printf("Field: %s\n", field->name);
+      printf("Type: %d\n", field->type);
+      printf("Collation: %d\n", field->charsetnr);
+      printf("Length: %lu\n", field->length);
+      DIE_UNLESS(stmt->fields[0].charsetnr == csid);
+
+      rc= mysql_stmt_bind_result(stmt, &bind);
+      DIE_UNLESS(rc == 0);
+      rc= mysql_stmt_fetch(stmt);
+      DIE_UNLESS(rc == 0);
+      printf("Value: %s\n", res);
+      DIE_UNLESS(strcmp(res, value) == 0);
+    }
+    else if (mysql_stmt_field_count(stmt))
+    {
+      printf("sp result set\n");
+    }
+  } while (mysql_stmt_next_result(stmt) == 0);
+
+  mysql_stmt_close(stmt);
+  DIE_UNLESS(mysql_query_or_error(mysql, "DROP PROCEDURE p1") == 0);
+}
+
+
+static void test_mdev14454()
+{
+  myheader("test_mdev14454");
+  test_mdev14454_internal("SET NAMES latin1", 8, "test\xFF");
+  test_mdev14454_internal("SET NAMES utf8", 33, "test\xC3\xBF");
+}
+
+
 static struct my_tests_st my_tests[]= {
   { "disable_query_logs", disable_query_logs },
   { "test_view_sp_list_fields", test_view_sp_list_fields },
@@ -20139,6 +20218,7 @@ static struct my_tests_st my_tests[]= {
   { "test_mdev12579", test_mdev12579 },
   { "test_mdev14013", test_mdev14013 },
   { "test_mdev14013_1", test_mdev14013_1 },
+  { "test_mdev14454", test_mdev14454 },
   { 0, 0 }
 };