Bug#27876 (SF with cyrillic variable name fails during execution (regression))

The root cause of this bug is related to the function skip_rear_comments,
in sql_lex.cc

Recent code changes in skip_rear_comments changed the prototype from
"const uchar*" to "const char*", which had an unforseen impact on this test:
  (endp[-1] < ' ')
With unsigned characters, this code filters bytes of value [0x00 - 0x20]
With *signed* characters, this also filters bytes of value [0x80 - 0xFF].

This caused the regression reported, considering cyrillic characters in the
parameter name to be whitespace, and truncated.
Note that the regression is present both in 5.0 and 5.1.

With this fix:
- [0x80 - 0xFF] bytes are no longer considered whitespace.
This alone fixes the regression.

In addition, filtering [0x00 - 0x20] was found bogus and abusive,
so that the code now filters uses my_isspace when looking for whitespace.

Note that this fix is only addressing the regression affecting UTF-8
in general, but does not address a more fundamental problem with
skip_rear_comments: parsing a string *backwards*, starting at end[-1],
is not safe with multi-bytes characters, so that end[-1] can confuse the
last byte of a multi-byte characters with a characters to filter out.

The only known impact of this remaining issue affects objects that have to
meet all the conditions below:

- the object is a FUNCTION / PROCEDURE / TRIGGER / EVENT / VIEW
- the body consist of only *1* instruction, and does *not* contain a
  BEGIN-END block
- the instruction ends, lexically, with <ident> <whitespace>* ';'?
  For example, "select <ident>;" or "return <ident>;"
- The last character of <ident> is a multi-byte character
- the last byte of this character is ';' '*', '/' or whitespace

In this case, the body of the object will be truncated after parsing,
and stored in an invalid format.

This last issue has not been fixed in this patch, since the real fix
will be implemented by Bug 25411 (trigger code truncated), which is caused
by the very same code.
The real problem is that the function skip_rear_comments is only a
work-around, and should be removed entirely: see the proposed patch for
bug 25411 for details.


sql/sp_head.cc:
  In skip_rear_comments,
  Filter out only whitespace, not other (non ascii or control) valid characters
sql/sql_lex.cc:
  In skip_rear_comments,
  Filter out only whitespace, not other (non ascii or control) valid characters
sql/sql_lex.h:
  In skip_rear_comments,
  Filter out only whitespace, not other (non ascii or control) valid characters
sql/sql_view.cc:
  In skip_rear_comments,
  Filter out only whitespace, not other (non ascii or control) valid characters
tests/mysql_client_test.c:
  Bug#27876 (SF with cyrillic variable name fails during execution (regression))

sql/sp_head.cc

@@ -539,7 +539,7 @@ sp_head::init_strings(THD *thd, LEX *lex)
     Trim "garbage" at the end. This is sometimes needed with the
     "/ * ! VERSION... * /" wrapper in dump files.
   */
-  endp= skip_rear_comments((char*) m_body_begin, (char*) endp);
+  endp= skip_rear_comments(thd->charset(), (char*) m_body_begin, (char*) endp);
 
   m_body.length= endp - m_body_begin;
   m_body.str= strmake_root(root, m_body_begin, m_body.length);

sql/sql_lex.cc

@@ -1098,6 +1098,7 @@ Alter_info::Alter_info(const Alter_info &rhs, MEM_ROOT *mem_root)
 
   SYNOPSIS
     skip_rear_comments()
+      cs      character set
       begin   pointer to the beginning of statement
       end     pointer to the end of statement
 
@@ -1108,10 +1109,11 @@ Alter_info::Alter_info(const Alter_info &rhs, MEM_ROOT *mem_root)
     Pointer to the last non-comment symbol of the statement.
 */
 
-char *skip_rear_comments(char *begin, char *end)
+char *skip_rear_comments(CHARSET_INFO *cs, char *begin, char *end)
 {
-  while (begin < end && (end[-1] <= ' ' || end[-1] == '*' ||
-                         end[-1] == '/' || end[-1] == ';'))
+  while (begin < end && (end[-1] == '*' ||
+                         end[-1] == '/' || end[-1] == ';') ||
+                         my_isspace(cs, end[-1]))
     end-= 1;
   return end;
 }

sql/sql_lex.h

@@ -1271,4 +1271,4 @@ extern void lex_free(void);
 extern void lex_start(THD *thd);
 extern void lex_end(LEX *lex);
 extern int MYSQLlex(void *arg, void *yythd);
-extern char *skip_rear_comments(char *begin, char *end);
+extern char *skip_rear_comments(CHARSET_INFO *cs, char *begin, char *end);

sql/sql_view.cc

@@ -772,7 +772,8 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view,
   view->query.str= (char*)str.ptr();
   view->query.length= str.length()-1; // we do not need last \0
   view->source.str= thd->query + thd->lex->create_view_select_start;
-  view->source.length= (char *)skip_rear_comments((char *)view->source.str,
+  view->source.length= (char *)skip_rear_comments(thd->charset(),
+                                                  (char *)view->source.str,
                                                   (char *)thd->query +
                                                   thd->query_length) -
                         view->source.str;

tests/mysql_client_test.c

@@ -15560,6 +15560,69 @@ static void test_bug24179()
 }
 
 
+/*
+  Bug#27876 (SF with cyrillic variable name fails during execution (regression))
+*/
+static void test_bug27876()
+{
+  int rc;
+  MYSQL_RES *result;
+
+  char utf8_func[] =
+  {
+    0xd1, 0x84, 0xd1, 0x83, 0xd0, 0xbd, 0xd0, 0xba,
+    0xd1, 0x86, 0xd0, 0xb8, 0xd0, 0xb9, 0xd0, 0xba,
+    0xd0, 0xb0,
+    0x00
+  };
+
+  char utf8_param[] =
+  {
+    0xd0, 0xbf, 0xd0, 0xb0, 0xd1, 0x80, 0xd0, 0xb0,
+    0xd0, 0xbc, 0xd0, 0xb5, 0xd1, 0x82, 0xd1, 0x8a,
+    0xd1, 0x80, 0x5f, 0xd0, 0xb2, 0xd0, 0xb5, 0xd1,
+    0x80, 0xd1, 0x81, 0xd0, 0xb8, 0xd1, 0x8f,
+    0x00
+  };
+
+  char query[500];
+
+  DBUG_ENTER("test_bug27876");
+  myheader("test_bug27876");
+
+  rc= mysql_query(mysql, "set names utf8");
+  myquery(rc);
+
+  rc= mysql_query(mysql, "select version()");
+  myquery(rc);
+  result= mysql_store_result(mysql);
+  mytest(result);
+
+  sprintf(query, "DROP FUNCTION IF EXISTS %s", utf8_func);
+  rc= mysql_query(mysql, query);
+  myquery(rc);
+
+  sprintf(query,
+          "CREATE FUNCTION %s( %s VARCHAR(25))"
+          " RETURNS VARCHAR(25) DETERMINISTIC RETURN %s",
+          utf8_func, utf8_param, utf8_param);
+  rc= mysql_query(mysql, query);
+  myquery(rc);
+  sprintf(query, "SELECT %s(VERSION())", utf8_func);
+  rc= mysql_query(mysql, query);
+  myquery(rc);
+  result= mysql_store_result(mysql);
+  mytest(result);
+
+  sprintf(query, "DROP FUNCTION %s", utf8_func);
+  rc= mysql_query(mysql, query);
+  myquery(rc);
+
+  rc= mysql_query(mysql, "set names default");
+  myquery(rc);
+}
+
+
 /*
   Read and parse arguments and MySQL options from my.cnf
 */
@@ -15840,6 +15903,7 @@ static struct my_tests_st my_tests[]= {
   { "test_bug23383", test_bug23383 },
   { "test_bug21635", test_bug21635 },
   { "test_bug24179", test_bug24179 },
+  { "test_bug27876", test_bug27876 },
   { 0, 0 }
 };