Skip to content

Commit

Permalink
Fix crash during shutdown in audit plugin code path
Browse files Browse the repository at this point in the history
Summary:
On server shutdown, there exists a race condition that can trigger a segfault if an audit plugin is installed. The root cause because the `THD::audit_class_plugins` array is accessed concurrently without locks.

This is the sequence of actions that can trigger a crash:
1. The shutdown thread calls close_connections on all connected clients.
2. The connection thread will call into the audit plugin because it will receive a network error from `close_connections`, and will log the error into the audit plugin. The thread then adds the plugin to `THD::audit_class_plugins`, but it has not updated `audit_class_mask` yet.
3. The shutdown thread sees that `audit_class_mask` is not set, and also adds the plugin into `THD::audit_class_plugins`, but it has only extended the array, without setting the value of the 2nd element of the array.
4. The connection thread then goes ahead and updates `audit_class_mask` and finishes `acquire_plugins`. It then calls `mysql_audit_notify` and sees the array of size 2, and when it loops over the 2nd element, it segfaults because the value is uninitalized.

The fix is to add a lock per THD that protects these data structures. They should be relatively uncontended since the shutdown thread is the only thread that calls `acquire_plugins` on another THD.

The corresponding code in 5.7/8.0 has been largely refactored, but I suspect this will still be an issue since it doesn't seem like the corresponding data structures are synchronized.

Reviewed By: asandryh

Differential Revision: D6103777

fbshipit-source-id: 17763c6
  • Loading branch information
lth authored and facebook-github-bot committed Oct 20, 2017
1 parent 36cde0f commit 0fc059a
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 4 deletions.
2 changes: 1 addition & 1 deletion sql/mysqld.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12055,7 +12055,7 @@ PSI_mutex_key
key_LOCK_server_started, key_LOCK_status,
key_LOCK_system_variables_hash, key_LOCK_table_share, key_LOCK_thd_data,
key_LOCK_thd_db_read_only_hash,
key_LOCK_db_metadata,
key_LOCK_db_metadata, key_LOCK_thd_audit_data,
key_LOCK_user_conn, key_LOCK_uuid_generator, key_LOG_LOCK_log,
key_master_info_data_lock, key_master_info_run_lock,
key_master_info_sleep_lock, key_master_info_thd_lock,
Expand Down
2 changes: 1 addition & 1 deletion sql/mysqld.h
Original file line number Diff line number Diff line change
Expand Up @@ -1061,7 +1061,7 @@ extern PSI_mutex_key
key_LOCK_server_started, key_LOCK_status,
key_LOCK_table_share, key_LOCK_thd_data,
key_LOCK_thd_db_read_only_hash,
key_LOCK_db_metadata,
key_LOCK_db_metadata, key_LOCK_thd_audit_data,
key_LOCK_user_conn, key_LOCK_uuid_generator, key_LOG_LOCK_log,
key_master_info_data_lock, key_master_info_run_lock,
key_master_info_sleep_lock, key_master_info_thd_lock,
Expand Down
18 changes: 16 additions & 2 deletions sql/sql_audit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,11 @@ static my_bool acquire_plugins(THD *thd, plugin_ref plugin, void *arg)
st_mysql_audit *data= plugin_data(plugin, struct st_mysql_audit *);

set_audit_mask(event_class_mask, event_class);
mysql_mutex_lock(&thd->LOCK_thd_audit_data);

/* Check if this plugin is interested in the event */
if (check_audit_mask(data->class_mask, event_class_mask))
return 0;
goto exit;

/*
Check if this plugin may already be registered. This will fail to
Expand All @@ -165,7 +166,7 @@ static my_bool acquire_plugins(THD *thd, plugin_ref plugin, void *arg)
are an event class of which the audit plugin has interest.
*/
if (!check_audit_mask(data->class_mask, thd->audit_class_mask))
return 0;
goto exit;

/* Check if we need to initialize the array of acquired plugins */
if (unlikely(!thd->audit_class_plugins.buffer))
Expand All @@ -179,6 +180,8 @@ static my_bool acquire_plugins(THD *thd, plugin_ref plugin, void *arg)
plugin= my_plugin_lock(NULL, &plugin);
insert_dynamic(&thd->audit_class_plugins, &plugin);

exit:
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
return 0;
}

Expand Down Expand Up @@ -238,10 +241,14 @@ void mysql_audit_notify(THD *thd, uint event_class, uint event_subtype, ...)

void mysql_audit_release(THD *thd)
{
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
plugin_ref *plugins, *plugins_last;

if (!thd || !(thd->audit_class_plugins.elements))
{
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
return;
}

plugins= (plugin_ref*) thd->audit_class_plugins.buffer;
plugins_last= plugins + thd->audit_class_plugins.elements;
Expand All @@ -264,6 +271,7 @@ void mysql_audit_release(THD *thd)
/* Reset the state of thread values */
reset_dynamic(&thd->audit_class_plugins);
memset(thd->audit_class_mask, 0, sizeof(thd->audit_class_mask));
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
}


Expand All @@ -276,8 +284,10 @@ void mysql_audit_release(THD *thd)

void mysql_audit_init_thd(THD *thd)
{
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
memset(&thd->audit_class_plugins, 0, sizeof(thd->audit_class_plugins));
memset(thd->audit_class_mask, 0, sizeof(thd->audit_class_mask));
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
}


Expand All @@ -294,8 +304,10 @@ void mysql_audit_init_thd(THD *thd)
void mysql_audit_free_thd(THD *thd)
{
mysql_audit_release(thd);
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
DBUG_ASSERT(thd->audit_class_plugins.elements == 0);
delete_dynamic(&thd->audit_class_plugins);
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
}

#ifdef HAVE_PSI_INTERFACE
Expand Down Expand Up @@ -491,6 +503,7 @@ static void event_class_dispatch(THD *thd, unsigned int event_class,
}
else
{
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
plugin_ref *plugins, *plugins_last;

/* Use the cached set of audit plugins */
Expand All @@ -499,6 +512,7 @@ static void event_class_dispatch(THD *thd, unsigned int event_class,

for (; plugins < plugins_last; plugins++)
plugins_dispatch(thd, *plugins, &event_generic);
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
}
}

Expand Down
2 changes: 2 additions & 0 deletions sql/sql_class.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,8 @@ THD::THD(bool enable_plugins)
dbug_sentry=THD_SENTRY_MAGIC;
#endif
#ifndef EMBEDDED_LIBRARY
mysql_mutex_init(key_LOCK_thd_audit_data, &LOCK_thd_audit_data,
MY_MUTEX_INIT_FAST);
mysql_audit_init_thd(this);
net.vio=0;
#endif
Expand Down
1 change: 1 addition & 0 deletions sql/sql_class.h
Original file line number Diff line number Diff line change
Expand Up @@ -2429,6 +2429,7 @@ class THD :public MDL_context_owner,
mysql_mutex_t LOCK_thd_data;
mysql_mutex_t LOCK_thd_db_read_only_hash;
mysql_mutex_t LOCK_db_metadata;
mysql_mutex_t LOCK_thd_audit_data;

/* all prepared statements and cursors of this connection */
Statement_map stmt_map;
Expand Down

0 comments on commit 0fc059a

Please sign in to comment.