Bug#25411 (trigger code truncated), PART II

Bug 28127 (Some valid identifiers names are not parsed correctly)
Bug 26302 (MySQL server cuts off trailing "*/" from comments in SP/func)

This patch is the second part of a major cleanup, required to fix
Bug 25411 (trigger code truncated).

The root cause of the issue stems from the function skip_rear_comments,
which was a work around to remove "extra" "*/" characters from the query
text, when parsing a query and reusing the text fragments to represent a
view, trigger, function or stored procedure.
The reason for this work around is that "special comments",
like /*!50002 XXX */, were not parsed properly, so that a query like:
  AAA /*!50002 BBB */ CCC
would be seen by the parser as "AAA BBB */ CCC" when the current version
is greater or equal to 5.0.2

The root cause of this stems from how special comments are parsed.
Special comments are really out-of-bound text that appear inside a query,
that affects how the parser behave.
In nature, /*!50002 XXX */ in MySQL is similar to the C concept
of preprocessing :
  #if VERSION >= 50002
  XXX
  #endif

Depending on the current VERSION of the server, either the special comment
should be expanded or it should be ignored, but in all cases the "text" of
the query should be re-written to strip the "/*!50002" and "*/" markers,
which does not belong to the SQL language itself.

Prior to this fix, these markers would leak into :
- the storage format for VIEW,
- the storage format for FUNCTION,
- the storage format for FUNCTION parameters, in mysql.proc (param_list),
- the storage format for PROCEDURE,
- the storage format for PROCEDURE parameters, in mysql.proc (param_list),
- the storage format for TRIGGER,
- the binary log used for replication.

In all cases, not only this cause format corruption, but also provide a vector
for dormant security issues, by allowing to tunnel code that will be activated
after an upgrade.

The proper solution is to deal with special comments strictly during parsing,
when accepting a query from the outside world.
Once a query is parsed and an object is created with a persistant
representation, this object should not arbitrarily mutate after an upgrade.
In short, special comments are a useful but limited feature for MYSQLdump,
when used at an *interface* level to facilitate import/export,
but bloating the server *internal* storage format is *not* the proper way
to deal with configuration management of the user logic.

With this fix:
- the Lex_input_stream class now acts as a comment pre-processor,
and either expands or ignore special comments on the fly.
- MYSQLlex and sql_yacc.yy have been cleaned up to strictly use the
public interface of Lex_input_stream. In particular, how the input stream
accepts or rejects a character is private to Lex_input_stream, and the
internal buffer pointers of that class are strictly private, and should not
be tempered with during parsing.

This caused many changes mostly in sql_lex.cc.

During the code cleanup in case MY_LEX_NUMBER_IDENT,
Bug 28127 (Some valid identifiers names are not parsed correctly)
was found and fixed.

By parsing special comments properly, and removing the function
'skip_rear_comments' [sic],
Bug 26302 (MySQL server cuts off trailing "*/" from comments in SP/func)
has been fixed as well.


sql/event_data_objects.cc:
  Cleanup of the code that extracts the query text
sql/sp.cc:
  Cleanup of the code that extracts the query text
sql/sp_head.cc:
  Cleanup of the code that extracts the query text
sql/sql_trigger.cc:
  Cleanup of the code that extracts the query text
sql/sql_view.cc:
  Cleanup of the code that extracts the query text
mysql-test/r/comments.result:
  Bug#25411 (trigger code truncated)
mysql-test/r/sp.result:
  Bug#25411 (trigger code truncated)
  Bug 26302 (MySQL server cuts off trailing "*/" from comments in SP/func)
mysql-test/r/trigger.result:
  Bug#25411 (trigger code truncated)
mysql-test/r/varbinary.result:
  Bug 28127 (Some valid identifiers names are not parsed correctly)
mysql-test/t/comments.test:
  Bug#25411 (trigger code truncated)
mysql-test/t/sp.test:
  Bug#25411 (trigger code truncated)
  Bug 26302 (MySQL server cuts off trailing "*/" from comments in SP/func)
mysql-test/t/trigger.test:
  Bug#25411 (trigger code truncated)
mysql-test/t/varbinary.test:
  Bug 28127 (Some valid identifiers names are not parsed correctly)
sql/sql_lex.cc:
  Implemented comment pre-processing in Lex_input_stream,
  major cleanup of the lex/yacc code to not use Lex_input_stream private members.
sql/sql_lex.h:
  Implemented comment pre-processing in Lex_input_stream,
  major cleanup of the lex/yacc code to not use Lex_input_stream private members.
sql/sql_yacc.yy:
  post merge fix : view_check_options must be parsed before signaling the end of the query

mysql-test/r/comments.result

@@ -8,7 +8,7 @@ multi line comment */;
 ;
 ERROR 42000: Query was empty
 select 1 /*!32301 +1 */;
-1 /*!32301 +1
+1  +1
 2
 select 1 /*!52301 +1 */;
 1
@@ -26,3 +26,13 @@ select 1 # The rest of the row will be ignored
 1
 1
 /* line with only comment */;
+select 1/*!2*/;
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '2*/' at line 1
+select 1/*!000002*/;
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '2*/' at line 1
+select 1/*!999992*/;
+1
+1
+select 1 + /*!00000 2 */ + 3 /*!99999 noise*/ + 4;
+1 +  2  + 3  + 4
+10

mysql-test/r/sp.result

@@ -6282,4 +6282,130 @@ v1	CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VI
 DROP VIEW v1;
 DROP FUNCTION metered;
 DROP TABLE t1;
-End of 5.0 tests
+drop procedure if exists proc_25411_a;
+drop procedure if exists proc_25411_b;
+drop procedure if exists proc_25411_c;
+create procedure proc_25411_a()
+begin
+/* real comment */
+select 1;
+/*! select 2; */
+select 3;
+/*!00000 select 4; */
+/*!99999 select 5; */
+end
+$$
+create procedure proc_25411_b(
+/* real comment */
+/*! p1 int, */
+/*!00000 p2 int */
+/*!99999 ,p3 int */
+)
+begin
+select p1, p2;
+end
+$$
+create procedure proc_25411_c()
+begin
+select 1/*!,2*//*!00000,3*//*!99999,4*/;
+select 1/*! ,2*//*!00000 ,3*//*!99999 ,4*/;
+select 1/*!,2 *//*!00000,3 *//*!99999,4 */;
+select 1/*! ,2 *//*!00000 ,3 *//*!99999 ,4 */;
+select 1 /*!,2*/ /*!00000,3*/ /*!99999,4*/ ;
+end
+$$
+show create procedure proc_25411_a;
+Procedure	sql_mode	Create Procedure
+proc_25411_a		CREATE DEFINER=`root`@`localhost` PROCEDURE `proc_25411_a`()
+begin
+/* real comment */
+select 1;
+ select 2; 
+select 3;
+ select 4; 
+
+end
+call proc_25411_a();
+1
+1
+2
+2
+3
+3
+4
+4
+show create procedure proc_25411_b;
+Procedure	sql_mode	Create Procedure
+proc_25411_b		CREATE DEFINER=`root`@`localhost` PROCEDURE `proc_25411_b`(
+/* real comment */
+ p1 int, 
+ p2 int 
+
+)
+begin
+select p1, p2;
+end
+select name, param_list, body from mysql.proc where name like "%25411%";
+name	param_list	body
+proc_25411_a		begin
+/* real comment */
+select 1;
+ select 2; 
+select 3;
+ select 4; 
+
+end
+proc_25411_b	
+/* real comment */
+ p1 int, 
+ p2 int 
+
+	begin
+select p1, p2;
+end
+proc_25411_c		begin
+select 1,2,3;
+select 1 ,2 ,3;
+select 1,2 ,3 ;
+select 1 ,2  ,3 ;
+select 1 ,2 ,3  ;
+end
+call proc_25411_b(10, 20);
+p1	p2
+10	20
+show create procedure proc_25411_c;
+Procedure	sql_mode	Create Procedure
+proc_25411_c		CREATE DEFINER=`root`@`localhost` PROCEDURE `proc_25411_c`()
+begin
+select 1,2,3;
+select 1 ,2 ,3;
+select 1,2 ,3 ;
+select 1 ,2  ,3 ;
+select 1 ,2 ,3  ;
+end
+call proc_25411_c();
+1	2	3
+1	2	3
+1	2	3
+1	2	3
+1	2	3
+1	2	3
+1	2	3
+1	2	3
+1	2	3
+1	2	3
+drop procedure proc_25411_a;
+drop procedure proc_25411_b;
+drop procedure proc_25411_c;
+drop procedure if exists proc_26302;
+create procedure proc_26302()
+select 1 /* testing */;
+show create procedure proc_26302;
+Procedure	sql_mode	Create Procedure
+proc_26302		CREATE DEFINER=`root`@`localhost` PROCEDURE `proc_26302`()
+select 1 /* testing */
+select ROUTINE_NAME, ROUTINE_DEFINITION from information_schema.ROUTINES
+where ROUTINE_NAME = "proc_26302";
+ROUTINE_NAME	ROUTINE_DEFINITION
+proc_26302	select 1 /* testing */
+drop procedure proc_26302;

mysql-test/r/trigger.result

@@ -1474,3 +1474,19 @@ DROP TABLE t1,t2;
 SET SESSION LOW_PRIORITY_UPDATES=DEFAULT;
 SET GLOBAL LOW_PRIORITY_UPDATES=DEFAULT;
 End of 5.0 tests
+drop table if exists table_25411_a;
+drop table if exists table_25411_b;
+create table table_25411_a(a int);
+create table table_25411_b(b int);
+create trigger trg_25411a_ai after insert on table_25411_a
+for each row
+insert into table_25411_b select new.*;
+select * from table_25411_a;
+a
+insert into table_25411_a values (1);
+ERROR 42S02: Unknown table 'new'
+select * from table_25411_a;
+a
+1
+drop table table_25411_a;
+drop table table_25411_b;

mysql-test/r/varbinary.result

@@ -79,3 +79,19 @@ select length(a) from t1;
 length(a)
 6
 drop table t1;
+drop table if exists table_28127_a;
+drop table if exists table_28127_b;
+create table table_28127_a(0b02 int);
+show create table table_28127_a;
+Table	Create Table
+table_28127_a	CREATE TABLE `table_28127_a` (
+  `0b02` int(11) DEFAULT NULL
+) ENGINE=MyISAM DEFAULT CHARSET=latin1
+create table table_28127_b(0b2 int);
+show create table table_28127_b;
+Table	Create Table
+table_28127_b	CREATE TABLE `table_28127_b` (
+  `0b2` int(11) DEFAULT NULL
+) ENGINE=MyISAM DEFAULT CHARSET=latin1
+drop table table_28127_a;
+drop table table_28127_b;

mysql-test/t/comments.test

@@ -19,3 +19,18 @@ select 1 # The rest of the row will be ignored
 /* line with only comment */;
 
 # End of 4.1 tests
+
+#
+# Bug#25411 (trigger code truncated)
+#
+
+--error ER_PARSE_ERROR
+select 1/*!2*/;
+
+--error ER_PARSE_ERROR
+select 1/*!000002*/;
+
+select 1/*!999992*/;
+
+select 1 + /*!00000 2 */ + 3 /*!99999 noise*/ + 4;
+

mysql-test/t/sp.test

@@ -7251,4 +7251,83 @@ DROP FUNCTION metered;
 DROP TABLE t1;
 
 
---echo End of 5.0 tests
+#
+# Bug#25411 (trigger code truncated)
+#
+
+--disable_warnings
+drop procedure if exists proc_25411_a;
+drop procedure if exists proc_25411_b;
+drop procedure if exists proc_25411_c;
+--enable_warnings
+
+delimiter $$;
+
+create procedure proc_25411_a()
+begin
+  /* real comment */
+  select 1;
+  /*! select 2; */
+  select 3;
+  /*!00000 select 4; */
+  /*!99999 select 5; */
+end
+$$
+
+create procedure proc_25411_b(
+/* real comment */
+/*! p1 int, */
+/*!00000 p2 int */
+/*!99999 ,p3 int */
+)
+begin
+  select p1, p2;
+end
+$$
+
+create procedure proc_25411_c()
+begin
+  select 1/*!,2*//*!00000,3*//*!99999,4*/;
+  select 1/*! ,2*//*!00000 ,3*//*!99999 ,4*/;
+  select 1/*!,2 *//*!00000,3 *//*!99999,4 */;
+  select 1/*! ,2 *//*!00000 ,3 *//*!99999 ,4 */;
+  select 1 /*!,2*/ /*!00000,3*/ /*!99999,4*/ ;
+end
+$$
+
+delimiter ;$$
+
+show create procedure proc_25411_a;
+call proc_25411_a();
+
+show create procedure proc_25411_b;
+select name, param_list, body from mysql.proc where name like "%25411%";
+call proc_25411_b(10, 20);
+
+show create procedure proc_25411_c;
+call proc_25411_c();
+
+drop procedure proc_25411_a;
+drop procedure proc_25411_b;
+drop procedure proc_25411_c;
+
+
+#
+# Bug#26302 (MySQL server cuts off trailing "*/" from comments in SP/func)
+#
+
+--disable_warnings
+drop procedure if exists proc_26302;
+--enable_warnings
+
+create procedure proc_26302()
+select 1 /* testing */;
+
+show create procedure proc_26302;
+
+select ROUTINE_NAME, ROUTINE_DEFINITION from information_schema.ROUTINES
+where ROUTINE_NAME = "proc_26302";
+
+drop procedure proc_26302;
+
+

mysql-test/t/trigger.test

@@ -1819,3 +1819,30 @@ SET SESSION LOW_PRIORITY_UPDATES=DEFAULT;
 SET GLOBAL LOW_PRIORITY_UPDATES=DEFAULT;
 
 --echo End of 5.0 tests
+
+#
+# Bug#25411 (trigger code truncated)
+#
+
+--disable_warnings
+drop table if exists table_25411_a;
+drop table if exists table_25411_b;
+--enable_warnings
+
+create table table_25411_a(a int);
+create table table_25411_b(b int);
+
+create trigger trg_25411a_ai after insert on table_25411_a
+for each row
+  insert into table_25411_b select new.*;
+
+select * from table_25411_a;
+
+--error ER_BAD_TABLE_ERROR
+insert into table_25411_a values (1);
+
+select * from table_25411_a;
+
+drop table table_25411_a;
+drop table table_25411_b;
+

mysql-test/t/varbinary.test

@@ -85,3 +85,22 @@ alter table t1 modify a varchar(255);
 select length(a) from t1;
 drop table t1;
 
+
+#
+# Bug#28127 (Some valid identifiers names are not parsed correctly)
+#
+
+--disable_warnings
+drop table if exists table_28127_a;
+drop table if exists table_28127_b;
+--enable_warnings
+
+create table table_28127_a(0b02 int);
+show create table table_28127_a;
+
+create table table_28127_b(0b2 int);
+show create table table_28127_b;
+
+drop table table_28127_a;
+drop table table_28127_b;
+

sql/event_data_objects.cc

@@ -148,8 +148,7 @@ Event_parse_data::init_name(THD *thd, sp_name *spn)
     The body is extracted by copying all data between the
     start of the body set by another method and the current pointer in Lex.
 
-    Some questionable removal of characters is done in here, and that part
-    should be refactored when the parser is smarter.
+    See related code in sp_head::init_strings().
 */
 
 void
@@ -160,58 +159,9 @@ Event_parse_data::init_body(THD *thd)
   /* This method is called from within the parser, from sql_yacc.yy */
   DBUG_ASSERT(thd->m_lip != NULL);
 
-  DBUG_PRINT("info", ("body: '%s'  body_begin: 0x%lx end: 0x%lx", body_begin,
-                      (long) body_begin, (long) thd->m_lip->ptr));
-
-  body.length= thd->m_lip->ptr - body_begin;
-  const char *body_end= body_begin + body.length - 1;
-
-  /* Trim nuls or close-comments ('*'+'/') or spaces at the end */
-  while (body_begin < body_end)
-  {
-
-    if ((*body_end == '\0') ||
-        (my_isspace(thd->variables.character_set_client, *body_end)))
-    { /* consume NULs and meaningless whitespace */
-      --body.length;
-      --body_end;
-      continue;
-    }
-
-    /*
-       consume closing comments
-
-       This is arguably wrong, but it's the best we have until the parser is
-       changed to be smarter.   FIXME PARSER
-
-       See also the sp_head code, where something like this is done also.
-
-       One idea is to keep in the lexer structure the count of the number of
-       open-comments we've entered, and scan left-to-right looking for a
-       closing comment IFF the count is greater than zero.
-
-       Another idea is to remove the closing comment-characters wholly in the
-       parser, since that's where it "removes" the opening characters.
-    */
-    if ((*(body_end - 1) == '*') && (*body_end == '/'))
-    {
-      DBUG_PRINT("info", ("consumend one '*" "/' comment in the query '%s'",
-          body_begin));
-      body.length-= 2;
-      body_end-= 2;
-      continue;
-    }
-
-    break;  /* none were found, so we have excised all we can. */
-  }
-
-  /* the first is always whitespace which I cannot skip in the parser */
-  while (my_isspace(thd->variables.character_set_client, *body_begin))
-  {
-    ++body_begin;
-    --body.length;
-  }
+  body.length= thd->m_lip->get_cpp_ptr() - body_begin;
   body.str= thd->strmake(body_begin, body.length);
+  trim_whitespace(thd->charset(), & body);
 
   DBUG_VOID_RETURN;
 }

sql/sp.cc

@@ -588,10 +588,14 @@ sp_create_routine(THD *thd, int type, sp_head *sp)
       log_query.append(STRING_WITH_LEN("CREATE "));
       append_definer(thd, &log_query, &thd->lex->definer->user,
                      &thd->lex->definer->host);
-      log_query.append(thd->lex->stmt_definition_begin,
-                       (char *)sp->m_body_begin -
-                       thd->lex->stmt_definition_begin +
-                       sp->m_body.length);
+
+      LEX_STRING stmt_definition;
+      stmt_definition.str= (char*) thd->lex->stmt_definition_begin;
+      stmt_definition.length= thd->lex->stmt_definition_end
+        - thd->lex->stmt_definition_begin;
+      trim_whitespace(thd->charset(), & stmt_definition);
+
+      log_query.append(stmt_definition.str, stmt_definition.length);
 
       /* Such a statement can always go directly to binlog, no trans cache */
       thd->binlog_query(THD::MYSQL_QUERY_TYPE,

sql/sp_head.cc

@@ -564,18 +564,17 @@ sp_head::init_strings(THD *thd, LEX *lex)
     m_params.str= strmake_root(root, m_param_begin, m_params.length);
   }
 
-  /* If ptr has overrun end_of_query then end_of_query is the end */
-  endp= (lip->ptr > lip->end_of_query ? lip->end_of_query : lip->ptr);
-  /*
-    Trim "garbage" at the end. This is sometimes needed with the
-    "/ * ! VERSION... * /" wrapper in dump files.
-  */
-  endp= skip_rear_comments(thd->charset(), m_body_begin, endp);
+  endp= lip->get_cpp_ptr();
+  lex->stmt_definition_end= endp;
 
   m_body.length= endp - m_body_begin;
   m_body.str= strmake_root(root, m_body_begin, m_body.length);
-  m_defstr.length= endp - lip->buf;
-  m_defstr.str= strmake_root(root, lip->buf, m_defstr.length);
+  trim_whitespace(thd->charset(), & m_body);
+
+  m_defstr.length= endp - lip->get_cpp_buf();
+  m_defstr.str= strmake_root(root, lip->get_cpp_buf(), m_defstr.length);
+  trim_whitespace(thd->charset(), & m_defstr);
+
   DBUG_VOID_RETURN;
 }
 
@@ -1827,8 +1826,6 @@ sp_head::reset_lex(THD *thd)
   sublex->trg_table_fields.empty();
   sublex->sp_lex_in_use= FALSE;
 
-  sublex->in_comment= oldlex->in_comment;
-
   /* Reset type info. */
 
   sublex->charset= NULL;

sql/sql_trigger.cc

@@ -563,10 +563,13 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables,
     append_definer(thd, stmt_query, &definer_user, &definer_host);
   }
 
-  stmt_query->append(thd->lex->stmt_definition_begin,
-                     (char *) thd->lex->sphead->m_body_begin -
-                     thd->lex->stmt_definition_begin +
-                     thd->lex->sphead->m_body.length);
+  LEX_STRING stmt_definition;
+  stmt_definition.str= (char*) thd->lex->stmt_definition_begin;
+  stmt_definition.length= thd->lex->stmt_definition_end
+    - thd->lex->stmt_definition_begin;
+  trim_whitespace(thd->charset(), & stmt_definition);
+
+  stmt_query->append(stmt_definition.str, stmt_definition.length);
 
   trg_def->str= stmt_query->c_ptr();
   trg_def->length= stmt_query->length();
@@ -1032,7 +1035,11 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
         if (!(on_table_name= (LEX_STRING*) alloc_root(&table->mem_root,
                                                       sizeof(LEX_STRING))))
           goto err_with_lex_cleanup;
-        *on_table_name= lex.ident;
+
+        on_table_name->str= (char*) lex.raw_trg_on_table_name_begin;
+        on_table_name->length= lex.raw_trg_on_table_name_end
+          - lex.raw_trg_on_table_name_begin;
+
         if (triggers->on_table_names_list.push_back(on_table_name, &table->mem_root))
           goto err_with_lex_cleanup;
 
@@ -1348,7 +1355,12 @@ Table_triggers_list::change_table_name_in_triggers(THD *thd,
 
     /* Construct CREATE TRIGGER statement with new table name. */
     buff.length(0);
+
+    /* WARNING: 'on_table_name' is supposed to point inside 'def' */
+    DBUG_ASSERT(on_table_name->str > def->str);
+    DBUG_ASSERT(on_table_name->str < (def->str + def->length));
     before_on_len= on_table_name->str - def->str;
+
     buff.append(def->str, before_on_len);
     buff.append(STRING_WITH_LEN("ON "));
     append_identifier(thd, &buff, new_table_name->str, new_table_name->length);

sql/sql_view.cc

@@ -690,7 +690,6 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view,
   char md5[MD5_BUFF_LENGTH];
   bool can_be_merged;
   char dir_buff[FN_REFLEN], path_buff[FN_REFLEN];
-  const char *endp;
   LEX_STRING dir, file, path;
   int error= 0;
   DBUG_ENTER("mysql_register_view");
@@ -708,10 +707,12 @@ static int mysql_register_view(THD *thd, TABLE_LIST *view,
   /* fill structure */
   view->query.str= str.c_ptr_safe();
   view->query.length= str.length();
-  view->source.str= thd->query + thd->lex->create_view_select_start;
-  endp= view->source.str;
-  endp= skip_rear_comments(thd->charset(), endp, thd->query + thd->query_length);
-  view->source.length= endp - view->source.str;
+
+  view->source.str= (char*) thd->lex->create_view_select_start;
+  view->source.length= (thd->lex->create_view_select_end
+                        - thd->lex->create_view_select_start);
+  trim_whitespace(thd->charset(), & view->source);
+
   view->file_version= 1;
   view->calc_md5(md5);
   view->md5.str= md5;