1berry/MariaDB-server
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

MDEV-28352 Spider: heap-use-after-free in ha_spider::lock_tables(), heap freed by spider_commit()

Browse Source 
The heap-use-after-free is caused by the following mechanism:

  * In the execution of FLUSH TABLE WITH READ LOCK, the function
    spider_free_trx_conn() is called and the connections held by
    SPIDER_TRX::trx_conn_hash are freed.

  * Then, an instance of ha_spider maintains the freed connections
    because they are also referenced from ha_spider::conns.
    The ha_spider instance is kept in a lock structure until the
    corresponding table is unlocked.

  * Spider accesses ha_spider::conns on the implicit UNLOCK TABLE
    issued by BEGIN.

In the first place, when the connections have been freed, it means
that there are really no remote table locked by Spider.
Thus, there is no need for Spider to access ha_spider::cons on the
implicit UNLOCK TABLE.

We can fix the bug by removing the above mentioned access to
ha_spider::conns. We also modified spider_free_trx_conn() so that it
frees the connections only when no table is locked to reduce the
chance of another heap-use-after-free on ha_spider::conns.
This commit is contained in:
Nayuta Yanagisawa 2022-06-16 20:25:02 +09:00
parent 773f1dad94
commit a26700cca5
3 changed files with 78 additions and 160 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

86
storage/spider/ha_spider.cc
View File

@ -1180,10 +1180,8 @@ int ha_spider::external_lock(
backup_error_status();
DBUG_ENTER("ha_spider::external_lock");
DBUG_PRINT("info",("spider this=%p", this));
DBUG_PRINT("info",("spider lock_type=%x", lock_type));
DBUG_PRINT("info", ("spider sql_command=%d", thd_sql_command(thd)));
/* Beginning of wide_handler setup */
if (wide_handler->stage == SPD_HND_STAGE_EXTERNAL_LOCK)
{
/* Only the stage executor deals with table locks. */
@ -1194,7 +1192,7 @@ int ha_spider::external_lock(
}
else
{
/* Update the stage executor when the stage changes */
/* Update the stage executor when the stage changes. */
wide_handler->stage= SPD_HND_STAGE_EXTERNAL_LOCK;
wide_handler->stage_executor= this;
}
@ -1203,35 +1201,30 @@ int ha_spider::external_lock(
wide_handler->external_lock_type= lock_type;
wide_handler->sql_command = thd_sql_command(thd);
/* We treat BEGIN as if UNLOCK TABLE. */
if (wide_handler->sql_command == SQLCOM_BEGIN)
{
wide_handler->sql_command = SQLCOM_UNLOCK_TABLES;
}
if (lock_type == F_UNLCK &&
wide_handler->sql_command != SQLCOM_UNLOCK_TABLES)
{
DBUG_RETURN(0);
}
trx = spider_get_trx(thd, TRUE, &error_num);
trx= spider_get_trx(thd, TRUE, &error_num);
if (error_num)
{
DBUG_RETURN(error_num);
}
wide_handler->trx = trx;
wide_handler->trx= trx;
/* End of wide_handler setup */
/* Question: Why the following if block is necessary? Why here? */
if (store_error_num)
{
DBUG_RETURN(store_error_num);
}
DBUG_ASSERT(wide_handler->sql_command != SQLCOM_RENAME_TABLE &&
wide_handler->sql_command != SQLCOM_DROP_DB);
/* We treat BEGIN as if UNLOCK TABLE. */
if (wide_handler->sql_command == SQLCOM_BEGIN)
{
wide_handler->sql_command = SQLCOM_UNLOCK_TABLES;
}
const uint sql_command= wide_handler->sql_command;
if (wide_handler->sql_command == SQLCOM_DROP_TABLE ||
wide_handler->sql_command == SQLCOM_ALTER_TABLE)
DBUG_ASSERT(sql_command != SQLCOM_RENAME_TABLE &&
sql_command != SQLCOM_DROP_DB);
if (sql_command == SQLCOM_DROP_TABLE || sql_command == SQLCOM_ALTER_TABLE)
{
if (trx->locked_connections)
{
@ -1242,44 +1235,41 @@ int ha_spider::external_lock(
DBUG_RETURN(0);
}
if (lock_type != F_UNLCK)
if (lock_type == F_UNLCK)
{
if (sql_command != SQLCOM_UNLOCK_TABLES)
{
DBUG_RETURN(0); /* Unlock remote tables only by UNLOCK TABLES. */
}
if (!trx->locked_connections)
{
DBUG_RETURN(0); /* No remote table actually locked by Spider */
}
}
else
{
if (unlikely((error_num= spider_internal_start_trx(this))))
{
DBUG_RETURN(error_num);
}
if (wide_handler->sql_command != SQLCOM_SELECT &&
wide_handler->sql_command != SQLCOM_HA_READ)
if (sql_command != SQLCOM_SELECT && sql_command != SQLCOM_HA_READ)
{
trx->updated_in_this_trx= TRUE;
DBUG_PRINT("info", ("spider trx->updated_in_this_trx=TRUE"));
}
if (!wide_handler->lock_table_type)
{
DBUG_RETURN(0); /* No need to actually lock remote tables. */
}
}
if (wide_handler->lock_table_type > 0 ||
wide_handler->sql_command == SQLCOM_UNLOCK_TABLES)
if (!partition_handler || !partition_handler->handlers)
{
if (wide_handler->sql_command == SQLCOM_UNLOCK_TABLES)
{
/* lock tables does not call reset() */
/* unlock tables does not call store_lock() */
wide_handler->lock_table_type = 0;
}
DBUG_RETURN(lock_tables()); /* Non-partitioned table */
}
/* lock/unlock tables */
if (partition_handler && partition_handler->handlers)
{
for (uint roop_count= 0; roop_count < partition_handler->no_parts;
++roop_count)
{
if (unlikely((error_num =
partition_handler->handlers[roop_count]->lock_tables())))
{
DBUG_RETURN(error_num);
}
}
}
else if (unlikely((error_num= lock_tables())))
for (uint i= 0; i < partition_handler->no_parts; ++i)
{
if (unlikely((error_num= partition_handler->handlers[i]->lock_tables())))
{
DBUG_RETURN(error_num);
}

1
storage/spider/mysql-test/spider/bugfix/disabled.def
View File

@ -1,2 +1 @@
wait_timeout : MDEV-26045
mdev_27239 : failed with ASAN build

151
storage/spider/spd_trx.cc
View File

@ -93,128 +93,51 @@ uchar *spider_trx_ha_get_key(
DBUG_RETURN((uchar*) trx_ha->table_name);
}
int spider_free_trx_conn(
SPIDER_TRX *trx,
bool trx_free
) {
int roop_count;
/*
Try to free the connections held by the given transaction.
*/
int spider_free_trx_conn(SPIDER_TRX *trx, bool trx_free)
{
int loop_count= 0;
SPIDER_CONN *conn;
HASH *conn_hash= &trx->trx_conn_hash;
DBUG_ENTER("spider_free_trx_conn");
roop_count = 0;
if (
trx_free ||
spider_param_conn_recycle_mode(trx->thd) != 2
) {
while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_conn_hash,
roop_count)))
DBUG_ASSERT(!trx_free || !trx->locked_connections);
/* Clear the connection queues in any case. */
while ((conn= (SPIDER_CONN *) my_hash_element(conn_hash, loop_count)))
{
spider_conn_clear_queue_at_commit(conn);
loop_count++;
}
if (trx_free || spider_param_conn_recycle_mode(trx->thd) != 2)
{
/* Free connections only when no connection is locked. */
if (!trx->locked_connections)
{
spider_conn_clear_queue_at_commit(conn);
if (conn->table_lock)
loop_count= 0;
while ((conn= (SPIDER_CONN *) my_hash_element(conn_hash, loop_count)))
{
DBUG_ASSERT(!trx_free);
roop_count++;
} else
spider_free_conn_from_trx(trx, conn, FALSE, trx_free, &roop_count);
spider_free_conn_from_trx(trx, conn, FALSE, trx_free, &loop_count);
}
}
trx->trx_conn_adjustment++;
} else {
while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_conn_hash,
roop_count)))
{
spider_conn_clear_queue_at_commit(conn);
if (conn->table_lock)
{
DBUG_ASSERT(!trx_free);
} else
conn->error_mode = 1;
roop_count++;
}
DBUG_RETURN(0);
}
#if defined(HS_HAS_SQLCOM) && defined(HAVE_HANDLERSOCKET)
roop_count = 0;
if (
trx_free ||
spider_param_hs_r_conn_recycle_mode(trx->thd) != 2
) {
while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_hs_r_conn_hash,
roop_count)))
{
if (conn->table_lock)
{
DBUG_ASSERT(!trx_free);
roop_count++;
} else
spider_free_conn_from_trx(trx, conn, FALSE, trx_free, &roop_count);
}
trx->trx_hs_r_conn_adjustment++;
} else {
while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_hs_r_conn_hash,
roop_count)))
{
if (conn->table_lock)
{
DBUG_ASSERT(!trx_free);
} else
conn->error_mode = 1;
roop_count++;
}
}
roop_count = 0;
if (
trx_free ||
spider_param_hs_w_conn_recycle_mode(trx->thd) != 2
) {
while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_hs_w_conn_hash,
roop_count)))
{
if (conn->table_lock)
{
DBUG_ASSERT(!trx_free);
roop_count++;
} else
spider_free_conn_from_trx(trx, conn, FALSE, trx_free, &roop_count);
}
trx->trx_hs_w_conn_adjustment++;
} else {
while ((conn = (SPIDER_CONN*) my_hash_element(&trx->trx_hs_w_conn_hash,
roop_count)))
{
if (conn->table_lock)
{
DBUG_ASSERT(!trx_free);
} else
conn->error_mode = 1;
roop_count++;
}
}
#endif
#if defined(HS_HAS_SQLCOM) && defined(HAVE_HANDLERSOCKET)
if (trx_free)
loop_count= 0;
while ((conn= (SPIDER_CONN *) my_hash_element(conn_hash, loop_count)))
{
while ((conn = (SPIDER_CONN*) my_hash_element(
&trx->trx_direct_hs_r_conn_hash, 0)))
if (!conn->table_lock)
{
#ifdef HASH_UPDATE_WITH_HASH_VALUE
my_hash_delete_with_hash_value(&trx->trx_direct_hs_r_conn_hash,
conn->conn_key_hash_value, (uchar*) conn);
#else
my_hash_delete(&trx->trx_direct_hs_r_conn_hash, (uchar*) conn);
#endif
spider_free_conn(conn);
}
while ((conn = (SPIDER_CONN*) my_hash_element(
&trx->trx_direct_hs_w_conn_hash, 0)))
{
#ifdef HASH_UPDATE_WITH_HASH_VALUE
my_hash_delete_with_hash_value(&trx->trx_direct_hs_w_conn_hash,
conn->conn_key_hash_value, (uchar*) conn);
#else
my_hash_delete(&trx->trx_direct_hs_w_conn_hash, (uchar*) conn);
#endif
spider_free_conn(conn);
conn->error_mode= 1;
}
loop_count++;
}
#endif
DBUG_RETURN(0);
}
@ -3417,6 +3340,12 @@ int spider_commit(
trx->bulk_access_conn_first = NULL;
#endif
/*
We do (almost) nothing if the following two conditions are both met:
* This is just the end of a statement, not an explicit commit.
* The autocommit is OFF or we are in an explicit transaction.
*/
if (all || (!thd_test_options(thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)))
{
if (trx->trx_start)