Skip to content

Commit 0fc059a

Browse files
lthfacebook-github-bot
authored andcommitted
Fix crash during shutdown in audit plugin code path
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
1 parent 36cde0f commit 0fc059a

File tree

5 files changed

+21
-4
lines changed

5 files changed

+21
-4
lines changed

sql/mysqld.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -12055,7 +12055,7 @@ PSI_mutex_key
1205512055
key_LOCK_server_started, key_LOCK_status,
1205612056
key_LOCK_system_variables_hash, key_LOCK_table_share, key_LOCK_thd_data,
1205712057
key_LOCK_thd_db_read_only_hash,
12058-
key_LOCK_db_metadata,
12058+
key_LOCK_db_metadata, key_LOCK_thd_audit_data,
1205912059
key_LOCK_user_conn, key_LOCK_uuid_generator, key_LOG_LOCK_log,
1206012060
key_master_info_data_lock, key_master_info_run_lock,
1206112061
key_master_info_sleep_lock, key_master_info_thd_lock,

sql/mysqld.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -1061,7 +1061,7 @@ extern PSI_mutex_key
10611061
key_LOCK_server_started, key_LOCK_status,
10621062
key_LOCK_table_share, key_LOCK_thd_data,
10631063
key_LOCK_thd_db_read_only_hash,
1064-
key_LOCK_db_metadata,
1064+
key_LOCK_db_metadata, key_LOCK_thd_audit_data,
10651065
key_LOCK_user_conn, key_LOCK_uuid_generator, key_LOG_LOCK_log,
10661066
key_master_info_data_lock, key_master_info_run_lock,
10671067
key_master_info_sleep_lock, key_master_info_thd_lock,

sql/sql_audit.cc

+16-2
Original file line numberDiff line numberDiff line change
@@ -153,10 +153,11 @@ static my_bool acquire_plugins(THD *thd, plugin_ref plugin, void *arg)
153153
st_mysql_audit *data= plugin_data(plugin, struct st_mysql_audit *);
154154

155155
set_audit_mask(event_class_mask, event_class);
156+
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
156157

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

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

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

183+
exit:
184+
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
182185
return 0;
183186
}
184187

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

239242
void mysql_audit_release(THD *thd)
240243
{
244+
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
241245
plugin_ref *plugins, *plugins_last;
242246

243247
if (!thd || !(thd->audit_class_plugins.elements))
248+
{
249+
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
244250
return;
251+
}
245252

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

269277

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

277285
void mysql_audit_init_thd(THD *thd)
278286
{
287+
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
279288
memset(&thd->audit_class_plugins, 0, sizeof(thd->audit_class_plugins));
280289
memset(thd->audit_class_mask, 0, sizeof(thd->audit_class_mask));
290+
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
281291
}
282292

283293

@@ -294,8 +304,10 @@ void mysql_audit_init_thd(THD *thd)
294304
void mysql_audit_free_thd(THD *thd)
295305
{
296306
mysql_audit_release(thd);
307+
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
297308
DBUG_ASSERT(thd->audit_class_plugins.elements == 0);
298309
delete_dynamic(&thd->audit_class_plugins);
310+
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
299311
}
300312

301313
#ifdef HAVE_PSI_INTERFACE
@@ -491,6 +503,7 @@ static void event_class_dispatch(THD *thd, unsigned int event_class,
491503
}
492504
else
493505
{
506+
mysql_mutex_lock(&thd->LOCK_thd_audit_data);
494507
plugin_ref *plugins, *plugins_last;
495508

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

500513
for (; plugins < plugins_last; plugins++)
501514
plugins_dispatch(thd, *plugins, &event_generic);
515+
mysql_mutex_unlock(&thd->LOCK_thd_audit_data);
502516
}
503517
}
504518

sql/sql_class.cc

+2
Original file line numberDiff line numberDiff line change
@@ -1050,6 +1050,8 @@ THD::THD(bool enable_plugins)
10501050
dbug_sentry=THD_SENTRY_MAGIC;
10511051
#endif
10521052
#ifndef EMBEDDED_LIBRARY
1053+
mysql_mutex_init(key_LOCK_thd_audit_data, &LOCK_thd_audit_data,
1054+
MY_MUTEX_INIT_FAST);
10531055
mysql_audit_init_thd(this);
10541056
net.vio=0;
10551057
#endif

sql/sql_class.h

+1
Original file line numberDiff line numberDiff line change
@@ -2429,6 +2429,7 @@ class THD :public MDL_context_owner,
24292429
mysql_mutex_t LOCK_thd_data;
24302430
mysql_mutex_t LOCK_thd_db_read_only_hash;
24312431
mysql_mutex_t LOCK_db_metadata;
2432+
mysql_mutex_t LOCK_thd_audit_data;
24322433

24332434
/* all prepared statements and cursors of this connection */
24342435
Statement_map stmt_map;

0 commit comments

Comments
 (0)