Commit c6fd10db authored by unknown's avatar unknown

Bug#30882 Dropping a temporary table inside a stored function may cause a server crash

If a stored function that contains a drop temporary table statement
is invoked by a create temporary table of the same name may cause
a server crash. The problem is that when dropping a table no check
is done to ensure that table is not being used by some outer query
(or outer statement), potentially leaving the outer query with a
reference to a stale (freed) table.

The solution is when dropping a temporary table, always check if
the table is being used by some outer statement as a temporary
table can be dropped inside stored procedures.

The check is performed by looking at the TABLE::query_id value for
temporary tables. To simplify this check and to solve a bug related
to handling of temporary tables in prelocked mode, this patch changes
the way in which this member is used to track the fact that table is
used/unused. Now we ensure that TABLE::query_id is zero for unused
temporary tables (which means that all temporary tables which were
used by a statement should be marked as free for reuse after it's
execution has been completed).


mysql-test/include/handler.inc:
  Add test case for side effect of Bug#30882
mysql-test/r/handler_innodb.result:
  Add test case result for side effect of Bug#30882
mysql-test/r/handler_myisam.result:
  Add test case result for side effect of Bug#30882
mysql-test/r/sp-error.result:
  Add test case result for Bug#30882
mysql-test/t/sp-error.test:
  Add test case for Bug#30882
sql/event_db_repository.cc:
  Update close_thread_tables call, no more default values.
sql/mysql_priv.h:
  Remove implicit default parameters values of the close_thread_tables
  function as no callers are using it.
sql/slave.cc:
  Update close_thread_tables call, no more default values
sql/sp_head.cc:
  Update close_thread_tables call, no more default values
sql/sql_base.cc:
  Changed the approach to distinguishing currently unused temporary tables.
  Now we ensure that such tables always have TABLE::query_id set to 0 and
  use this fact to perform checks during opening and dropping of temporary
  tables. This means that we have to call close_thread_tables() even for
  statements which use only temporary tables. To make this call cheaper,
  we re-factored close_thread_tables() to not take LOCK_open unless there
  are open base tables.
sql/sql_handler.cc:
  Properly close temporary tables associated with a handler.
sql/sql_insert.cc:
  close_temporary_table is now merged into drop_temporary_table.
sql/sql_parse.cc:
  Now the condition doesn't cover all cases because close_thread_tables()
  must be called even for statements that use only temporary tables.
sql/sql_table.cc:
  Use drop_temporary_table which perform checks to verify if
  the table is not being used. Error path problem is due to
  a handler tables issue and is going to be addressed in bug
  31397.
sql/table.h:
  Rename previously unused clear_query_id and document the usage of
  query_id and open_by_handler.
parent 409862b9
......@@ -566,3 +566,35 @@ reap;
connection default;
drop table t2;
disconnect flush;
#
# Bug#30882 Dropping a temporary table inside a stored function may cause a server crash
#
# Test HANDLER statements in conjunction with temporary tables. While the temporary table
# is open by a HANDLER, no other statement can access it.
#
--disable_warnings
drop table if exists t1;
--enable_warnings
create temporary table t1 (a int, b char(1), key a(a), key b(a,b));
insert into t1 values (0,"a"),(1,"b"),(2,"c"),(3,"d"),(4,"e"),
(5,"f"),(6,"g"),(7,"h"),(8,"i"),(9,"j");
select a,b from t1;
handler t1 open as a1;
handler a1 read a first;
handler a1 read a next;
handler a1 read a next;
--error ER_CANT_REOPEN_TABLE
select a,b from t1;
handler a1 read a prev;
handler a1 read a prev;
handler a1 read a=(6) where b="g";
handler a1 close;
select a,b from t1;
handler t1 open as a2;
handler a2 read a first;
handler a2 read a last;
handler a2 read a prev;
handler a2 close;
drop table t1;
......@@ -575,3 +575,65 @@ ERROR 42S02: Table 'test.t1' doesn't exist
handler t1 close;
handler t2 close;
drop table t2;
drop table if exists t1;
create temporary table t1 (a int, b char(1), key a(a), key b(a,b));
insert into t1 values (0,"a"),(1,"b"),(2,"c"),(3,"d"),(4,"e"),
(5,"f"),(6,"g"),(7,"h"),(8,"i"),(9,"j");
select a,b from t1;
a b
0 a
1 b
2 c
3 d
4 e
5 f
6 g
7 h
8 i
9 j
handler t1 open as a1;
handler a1 read a first;
a b
0 a
handler a1 read a next;
a b
1 b
handler a1 read a next;
a b
2 c
select a,b from t1;
ERROR HY000: Can't reopen table: 'a1'
handler a1 read a prev;
a b
1 b
handler a1 read a prev;
a b
0 a
handler a1 read a=(6) where b="g";
a b
6 g
handler a1 close;
select a,b from t1;
a b
0 a
1 b
2 c
3 d
4 e
5 f
6 g
7 h
8 i
9 j
handler t1 open as a2;
handler a2 read a first;
a b
0 a
handler a2 read a last;
a b
9 j
handler a2 read a prev;
a b
8 i
handler a2 close;
drop table t1;
......@@ -575,3 +575,65 @@ ERROR 42S02: Table 'test.t1' doesn't exist
handler t1 close;
handler t2 close;
drop table t2;
drop table if exists t1;
create temporary table t1 (a int, b char(1), key a(a), key b(a,b));
insert into t1 values (0,"a"),(1,"b"),(2,"c"),(3,"d"),(4,"e"),
(5,"f"),(6,"g"),(7,"h"),(8,"i"),(9,"j");
select a,b from t1;
a b
0 a
1 b
2 c
3 d
4 e
5 f
6 g
7 h
8 i
9 j
handler t1 open as a1;
handler a1 read a first;
a b
0 a
handler a1 read a next;
a b
1 b
handler a1 read a next;
a b
2 c
select a,b from t1;
ERROR HY000: Can't reopen table: 'a1'
handler a1 read a prev;
a b
1 b
handler a1 read a prev;
a b
0 a
handler a1 read a=(6) where b="g";
a b
6 g
handler a1 close;
select a,b from t1;
a b
0 a
1 b
2 c
3 d
4 e
5 f
6 g
7 h
8 i
9 j
handler t1 open as a2;
handler a2 read a first;
a b
0 a
handler a2 read a last;
a b
9 j
handler a2 read a prev;
a b
8 i
handler a2 close;
drop table t1;
......@@ -1428,7 +1428,6 @@ create function bug20701() returns varchar(25) binary return "test";
ERROR 42000: This version of MySQL doesn't yet support 'return value collation'
create function bug20701() returns varchar(25) return "test";
drop function bug20701;
End of 5.1 tests
create procedure proc_26503_error_1()
begin
retry:
......@@ -1530,6 +1529,53 @@ return 1;
end|
ERROR HY000: Not allowed to set autocommit from a stored function or trigger
create trigger t1
before insert on t2 for each row set password = password('foo');
delimiter ;|
before insert on t2 for each row set password = password('foo');|
ERROR HY000: Not allowed to set autocommit from a stored function or trigger
drop function if exists f1;
drop function if exists f2;
drop table if exists t1, t2;
create function f1() returns int
begin
drop temporary table t1;
return 1;
end|
create temporary table t1 as select f1();
ERROR HY000: Can't reopen table: 't1'
create function f2() returns int
begin
create temporary table t2 as select f1();
return 1;
end|
create temporary table t1 as select f2();
ERROR HY000: Can't reopen table: 't1'
drop function f1;
drop function f2;
create function f1() returns int
begin
drop temporary table t2,t1;
return 1;
end|
create function f2() returns int
begin
create temporary table t2 as select f1();
return 1;
end|
create temporary table t1 as select f2();
ERROR HY000: Can't reopen table: 't2'
drop function f1;
drop function f2;
create temporary table t2(a int);
select * from t2;
a
create function f2() returns int
begin
drop temporary table t2;
return 1;
end|
select f2();
f2()
1
drop function f2;
drop table t2;
ERROR 42S02: Unknown table 't2'
End of 5.1 tests
......@@ -2078,10 +2078,6 @@ create function bug20701() returns varchar(25) binary return "test";
create function bug20701() returns varchar(25) return "test";
drop function bug20701;
--echo End of 5.1 tests
#
# Bug#26503 (Illegal SQL exception handler code causes the server to crash)
#
......@@ -2237,10 +2233,78 @@ end|
--error ER_SP_CANT_SET_AUTOCOMMIT
create trigger t1
before insert on t2 for each row set password = password('foo');
before insert on t2 for each row set password = password('foo');|
delimiter ;|
#
# Bug#30882 Dropping a temporary table inside a stored function may cause a server crash
#
--disable_warnings
drop function if exists f1;
drop function if exists f2;
drop table if exists t1, t2;
--enable_warnings
delimiter |;
create function f1() returns int
begin
drop temporary table t1;
return 1;
end|
delimiter ;|
--error ER_CANT_REOPEN_TABLE
create temporary table t1 as select f1();
delimiter |;
create function f2() returns int
begin
create temporary table t2 as select f1();
return 1;
end|
delimiter ;|
--error ER_CANT_REOPEN_TABLE
create temporary table t1 as select f2();
drop function f1;
drop function f2;
delimiter |;
create function f1() returns int
begin
drop temporary table t2,t1;
return 1;
end|
create function f2() returns int
begin
create temporary table t2 as select f1();
return 1;
end|
delimiter ;|
--error ER_CANT_REOPEN_TABLE
create temporary table t1 as select f2();
drop function f1;
drop function f2;
create temporary table t2(a int);
select * from t2;
delimiter |;
create function f2() returns int
begin
drop temporary table t2;
return 1;
end|
delimiter ;|
select f2();
drop function f2;
--error ER_BAD_TABLE_ERROR
drop table t2;
--echo End of 5.1 tests
#
# BUG#NNNN: New bug synopsis
#
......
......@@ -549,7 +549,7 @@ Event_db_repository::open_event_table(THD *thd, enum thr_lock_type lock_type,
if (simple_open_n_lock_tables(thd, &tables))
{
close_thread_tables(thd, FALSE, FALSE);
close_thread_tables(thd);
DBUG_RETURN(TRUE);
}
......
......@@ -679,7 +679,7 @@ extern my_decimal decimal_zero;
void free_items(Item *item);
void cleanup_items(Item *item);
class THD;
void close_thread_tables(THD *thd, bool locked=0, bool skip_derived=0);
void close_thread_tables(THD *thd);
bool check_one_table_access(THD *thd, ulong privilege, TABLE_LIST *tables);
bool check_single_table_access(THD *thd, ulong privilege,
TABLE_LIST *tables, bool no_errors);
......@@ -1419,7 +1419,7 @@ TABLE_LIST *unique_table(THD *thd, TABLE_LIST *table, TABLE_LIST *table_list,
bool check_alias);
TABLE *find_temporary_table(THD *thd, const char *db, const char *table_name);
TABLE *find_temporary_table(THD *thd, TABLE_LIST *table_list);
bool close_temporary_table(THD *thd, TABLE_LIST *table_list);
int drop_temporary_table(THD *thd, TABLE_LIST *table_list);
void close_temporary_table(THD *thd, TABLE *table, bool free_share,
bool delete_table);
void close_temporary(TABLE *table, bool free_share, bool delete_table);
......
......@@ -2353,7 +2353,7 @@ log space");
change_rpl_status(RPL_ACTIVE_SLAVE,RPL_IDLE_SLAVE);
DBUG_ASSERT(thd->net.buff != 0);
net_end(&thd->net); // destructor will not free it, because net.vio is 0
close_thread_tables(thd, 0);
close_thread_tables(thd);
pthread_mutex_lock(&LOCK_thread_count);
THD_CHECK_SENTRY(thd);
delete thd;
......
......@@ -1872,7 +1872,7 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
we'll leave it here.
*/
if (!thd->in_sub_stmt)
close_thread_tables(thd, 0, 0);
close_thread_tables(thd);
DBUG_PRINT("info",(" %.*s: eval args done",
(int) m_name.length, m_name.str));
......
......@@ -1055,6 +1055,29 @@ bool close_cached_connection_tables(THD *thd, bool if_wait_for_refresh,
}
/**
Mark all temporary tables which were used by the current statement or
substatement as free for reuse, but only if the query_id can be cleared.
@param thd thread context
@remark For temp tables associated with a open SQL HANDLER the query_id
is not reset until the HANDLER is closed.
*/
static void mark_temp_tables_as_free_for_reuse(THD *thd)
{
for (TABLE *table= thd->temporary_tables ; table ; table= table->next)
{
if ((table->query_id == thd->query_id) && ! table->open_by_handler)
{
table->query_id= 0;
table->file->ha_reset();
}
}
}
/*
Mark all tables in the list which were used by current substatement
as free for reuse.
......@@ -1091,6 +1114,42 @@ static void mark_used_tables_as_free_for_reuse(THD *thd, TABLE *table)
}
/**
Auxiliary function to close all tables in the open_tables list.
@param thd Thread context.
@remark It should not ordinarily be called directly.
*/
static void close_open_tables(THD *thd)
{
bool found_old_table= 0;
safe_mutex_assert_not_owner(&LOCK_open);
VOID(pthread_mutex_lock(&LOCK_open));
DBUG_PRINT("info", ("thd->open_tables: 0x%lx", (long) thd->open_tables));
while (thd->open_tables)
found_old_table|= close_thread_table(thd, &thd->open_tables);
thd->some_tables_deleted= 0;
/* Free tables to hold down open files */
while (open_cache.records > table_cache_size && unused_tables)
VOID(hash_delete(&open_cache,(uchar*) unused_tables)); /* purecov: tested */
check_unused();
if (found_old_table)
{
/* Tell threads waiting for refresh that something has happened */
broadcast_refresh();
}
VOID(pthread_mutex_unlock(&LOCK_open));
}
/*
Close all tables used by the current substatement, or all tables
used by this thread if we are on the upper level.
......@@ -1098,26 +1157,19 @@ static void mark_used_tables_as_free_for_reuse(THD *thd, TABLE *table)
SYNOPSIS
close_thread_tables()
thd Thread handler
lock_in_use Set to 1 (0 = default) if caller has a lock on
LOCK_open
skip_derived Set to 1 (0 = default) if we should not free derived
tables.
stopper When closing tables from thd->open_tables(->next)*,
don't close/remove tables starting from stopper.
IMPLEMENTATION
Unlocks tables and frees derived tables.
Put all normal tables used by thread in free list.
When in prelocked mode it will only close/mark as free for reuse
tables opened by this substatement, it will also check if we are
closing tables after execution of complete query (i.e. we are on
upper level) and will leave prelocked mode if needed.
It will only close/mark as free for reuse tables opened by this
substatement, it will also check if we are closing tables after
execution of complete query (i.e. we are on upper level) and will
leave prelocked mode if needed.
*/
void close_thread_tables(THD *thd, bool lock_in_use, bool skip_derived)
void close_thread_tables(THD *thd)
{
bool found_old_table;
prelocked_mode_type prelocked_mode= thd->prelocked_mode;
DBUG_ENTER("close_thread_tables");
......@@ -1132,7 +1184,7 @@ void close_thread_tables(THD *thd, bool lock_in_use, bool skip_derived)
derived tables with (sub-)statement instead of thread and destroy
them at the end of its execution.
*/
if (thd->derived_tables && !skip_derived)
if (thd->derived_tables)
{
TABLE *table, *next;
/*
......@@ -1147,13 +1199,10 @@ void close_thread_tables(THD *thd, bool lock_in_use, bool skip_derived)
thd->derived_tables= 0;
}
if (prelocked_mode)
{
/*
Mark all temporary tables used by this substatement as free for reuse.
*/
mark_used_tables_as_free_for_reuse(thd, thd->temporary_tables);
}
/*
Mark all temporary tables used by this statement as free for reuse.
*/
mark_temp_tables_as_free_for_reuse(thd);
if (thd->locked_tables || prelocked_mode)
{
......@@ -1217,28 +1266,8 @@ void close_thread_tables(THD *thd, bool lock_in_use, bool skip_derived)
if (!thd->active_transaction())
thd->transaction.xid_state.xid.null();
if (!lock_in_use)
VOID(pthread_mutex_lock(&LOCK_open));
DBUG_PRINT("info", ("thd->open_tables: 0x%lx", (long) thd->open_tables));
found_old_table= 0;
while (thd->open_tables)
found_old_table|= close_thread_table(thd, &thd->open_tables);
thd->some_tables_deleted=0;
/* Free tables to hold down open files */
while (open_cache.records > table_cache_size && unused_tables)
VOID(hash_delete(&open_cache,(uchar*) unused_tables)); /* purecov: tested */
check_unused();
if (found_old_table)
{
/* Tell threads waiting for refresh that something has happened */
broadcast_refresh();
}
if (!lock_in_use)
VOID(pthread_mutex_unlock(&LOCK_open));
/* VOID(pthread_sigmask(SIG_SETMASK,&thd->signals,NULL)); */
if (thd->open_tables)
close_open_tables(thd);
if (prelocked_mode == PRELOCKED)
{
......@@ -1675,6 +1704,7 @@ TABLE *find_temporary_table(THD *thd, TABLE_LIST *table_list)
Try to locate the table in the list of thd->temporary_tables.
If the table is found:
- if the table is being used by some outer statement, fail.
- if the table is in thd->locked_tables, unlock it and
remove it from the list of locked tables. Currently only transactional
temporary tables are present in the locked_tables list.
......@@ -1689,24 +1719,34 @@ TABLE *find_temporary_table(THD *thd, TABLE_LIST *table_list)
thd->temporary_tables list, it's impossible to tell here whether
we're dealing with an internal or a user temporary table.
@retval TRUE the table was not found in the list of temporary tables
of this thread
@retval FALSE the table was found and dropped successfully.
@retval 0 the table was found and dropped successfully.
@retval 1 the table was not found in the list of temporary tables
of this thread
@retval -1 the table is in use by a outer query
*/
bool close_temporary_table(THD *thd, TABLE_LIST *table_list)
int drop_temporary_table(THD *thd, TABLE_LIST *table_list)
{
TABLE *table;
DBUG_ENTER("drop_temporary_table");
if (!(table= find_temporary_table(thd, table_list)))
return 1;
DBUG_RETURN(1);
/* Table might be in use by some outer statement. */
if (table->query_id && table->query_id != thd->query_id)
{
my_error(ER_CANT_REOPEN_TABLE, MYF(0), table->alias);
DBUG_RETURN(-1);
}
/*
If LOCK TABLES list is not empty and contains this table,
unlock the table and remove the table from this list.
*/
mysql_lock_remove(thd, thd->locked_tables, table, FALSE);
close_temporary_table(thd, table, 1, 1);
return 0;
DBUG_RETURN(0);
}
/*
......@@ -2285,8 +2325,7 @@ TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
is always represented by only one TABLE object in THD, and
it can not be cloned. Emit an error for an unsupported behaviour.
*/
if (table->query_id == thd->query_id ||
thd->prelocked_mode && table->query_id)
if (table->query_id)
{
DBUG_PRINT("error",
("query_id: %lu server_id: %u pseudo_thread_id: %lu",
......@@ -2296,7 +2335,6 @@ TABLE *open_table(THD *thd, TABLE_LIST *table_list, MEM_ROOT *mem_root,
DBUG_RETURN(0);
}
table->query_id= thd->query_id;
table->clear_query_id= 1;
thd->thread_specific_used= TRUE;
DBUG_PRINT("info",("Using temporary table"));
goto reset;
......@@ -4306,7 +4344,6 @@ void close_tables_for_reopen(THD *thd, TABLE_LIST **tables)
sp_remove_not_own_routines(thd->lex);
for (TABLE_LIST *tmp= *tables; tmp; tmp= tmp->next_global)
tmp->table= 0;
mark_used_tables_as_free_for_reuse(thd, thd->temporary_tables);
close_thread_tables(thd);
}
......
......@@ -151,6 +151,14 @@ static void mysql_ha_close_table(THD *thd, TABLE_LIST *tables)
}
VOID(pthread_mutex_unlock(&LOCK_open));
}
else if (tables->table)
{
/* Must be a temporary table */
TABLE *table= tables->table;
table->file->ha_index_or_rnd_end();
table->query_id= thd->query_id;
table->open_by_handler= 0;
}
}
/*
......@@ -282,6 +290,12 @@ bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen)
goto err;
}
/*
If it's a temp table, don't reset table->query_id as the table is
being used by this handler. Otherwise, no meaning at all.
*/
tables->table->open_by_handler= 1;
if (! reopen)
send_ok(thd);
DBUG_PRINT("exit",("OK"));
......
......@@ -3399,7 +3399,7 @@ static TABLE *create_table_from_items(THD *thd, HA_CREATE_INFO *create_info,
it preparable for open. But let us do close_temporary_table() here
just in case.
*/
close_temporary_table(thd, create_table);
drop_temporary_table(thd, create_table);
}
}
}
......
......@@ -992,9 +992,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
/*
Multiple queries exits, execute them individually
*/
if (thd->lock || thd->open_tables || thd->derived_tables ||
thd->prelocked_mode)
close_thread_tables(thd);
close_thread_tables(thd);
ulong length= (ulong)(packet_end - next_packet);
log_slow_statement(thd);
......@@ -1331,12 +1329,11 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
break;
}
if (thd->lock || thd->open_tables || thd->derived_tables ||
thd->prelocked_mode)
{
thd->proc_info="closing tables";
close_thread_tables(thd); /* Free tables */
}
thd->proc_info= "closing tables";
/* Free tables */
close_thread_tables(thd);
/*
assume handlers auto-commit (if some doesn't - transaction handling
in MySQL should be redesigned to support it; it's a big change,
......
......@@ -1503,7 +1503,7 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
char path[FN_REFLEN], *alias;
uint path_length;
String wrong_tables;
int error;
int error= 0;
int non_temp_tables_count= 0;
bool some_tables_deleted=0, tmp_table_deleted=0, foreign_key_error=0;
String built_query;
......@@ -1563,10 +1563,27 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
enum legacy_db_type frm_db_type;
mysql_ha_flush(thd, table, MYSQL_HA_CLOSE_FINAL, 1);
if (!close_temporary_table(thd, table))
{
tmp_table_deleted=1;
continue; // removed temporary table
error= drop_temporary_table(thd, table);
switch (error) {
case 0:
// removed temporary table
tmp_table_deleted= 1;
continue;
case -1:
// table already in use
/*
XXX: This branch should never be taken outside of SF, trigger or
prelocked mode.
DBUG_ASSERT(thd->in_sub_stmt);
*/
error= 1;
goto err_with_placeholders;
default:
// temporary table not found
error= 0;
}
/*
......@@ -1593,7 +1610,6 @@ int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
built_query.append("`,");
}
error=0;
table_type= table->db_type;
if (!drop_temporary)
{
......
......@@ -499,6 +499,24 @@ struct st_table {
my_bitmap_map *bitmap_init_value;
MY_BITMAP def_read_set, def_write_set, tmp_set; /* containers */
MY_BITMAP *read_set, *write_set; /* Active column sets */
/*
The ID of the query that opened and is using this table. Has different
meanings depending on the table type.
Temporary tables:
table->query_id is set to thd->query_id for the duration of a statement
and is reset to 0 once it is closed by the same statement. A non-zero
table->query_id means that a statement is using the table even if it's
not the current statement (table is in use by some outer statement).
Non-temporary tables:
Under pre-locked or LOCK TABLES mode: query_id is set to thd->query_id
for the duration of a statement and is reset to 0 once it is closed by
the same statement. A non-zero query_id is used to control which tables
in the list of pre-opened and locked tables are actually being used.
*/
query_id_t query_id;
/*
......@@ -593,8 +611,8 @@ struct st_table {
my_bool locked_by_name;
my_bool fulltext_searched;
my_bool no_cache;
/* To signal that we should reset query_id for tables and cols */
my_bool clear_query_id;
/* To signal that the table is associated with a HANDLER statement */
my_bool open_by_handler;
/*
To indicate that a non-null value of the auto_increment field
was provided by the user or retrieved from the current record.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment