Skip to content

Commit 51b3a85

Browse files
gkodinovgkodinov
committed
Bug #26946491: INSTALL COMMAND FOR CLONE PLUGIN HANGS WHEN HEAVY DMLS ARE IN PROGRESS
* Fixed plugin_add so it does not re-acquire the LOCK_plugin mutex after reporting an error * Fixed the callers of the plugin_add() to expect the above behavior and react accordingly (not release LOCK_plugin when reacting to the eror or re-aqcuire it when ignoring the error). * Fixed plugin_load and plugin_load_list to take the same locks into the same order as mysql_install_plugin does. * Added doxygen comments for some key functons and locks. * Fixed plugin_dl_add to not be keeping the lock while calling report_error().
1 parent 375974f commit 51b3a85

File tree

1 file changed

+117
-36
lines changed

1 file changed

+117
-36
lines changed

‎sql/sql_plugin.cc

Lines changed: 117 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -411,16 +411,24 @@ static int cur_plugin_info_interface_version[MYSQL_MAX_PLUGIN_TYPE_NUM]=
411411

412412
/*
413413
A mutex LOCK_plugin_delete must be acquired before calling plugin_del
414-
function.
414+
function.
415415
*/
416416
mysql_mutex_t LOCK_plugin_delete;
417417

418-
/*
419-
A mutex LOCK_plugin must be acquired before accessing the
420-
following variables/structures.
418+
/**
419+
Serializes access to the global plugin memory list.
420+
421+
LOCK_plugin must be acquired before accessing
422+
plugin_dl_array, plugin_array and plugin_hash.
421423
We are always manipulating ref count, so a rwlock here is unneccessary.
424+
If it must be taken together with the LOCK_system_variables_hash then
425+
LOCK_plugin must be taken before LOCK_system_variables_hash.
422426
*/
423427
mysql_mutex_t LOCK_plugin;
428+
/**
429+
Serializes the INSTALL and UNINSTALL PLUGIN commands.
430+
Must be taken before LOCK_plugin.
431+
*/
424432
mysql_mutex_t LOCK_plugin_install;
425433
static Prealloced_array<st_plugin_dl*, 16> *plugin_dl_array;
426434
static Prealloced_array<st_plugin_int*, 16> *plugin_array;
@@ -561,6 +569,7 @@ static inline void free_plugin_mem(st_plugin_dl *p)
561569
Fills in a ::st_plugin_dl structure.
562570
Initializes the plugin services pointer inside the plugin.
563571
Does not initialize the individual plugins.
572+
Must have LOCK_plugin locked. On error releases LOCK_plugin.
564573
565574
@arg dl The path to the plugin binary to load
566575
@arg report a bitmask that's passed down to report_error()
@@ -590,6 +599,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
590599
system_charset_info, 1) ||
591600
plugin_dir_len + dl->length + 1 >= FN_REFLEN)
592601
{
602+
mysql_mutex_unlock(&LOCK_plugin);
593603
report_error(report, ER_UDF_NO_PATHS);
594604
DBUG_RETURN(NULL);
595605
}
@@ -624,6 +634,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
624634
if (*errmsg == ':') errmsg++;
625635
if (*errmsg == ' ') errmsg++;
626636
}
637+
mysql_mutex_unlock(&LOCK_plugin);
627638
report_error(report, ER_CANT_OPEN_LIBRARY, dlpath, error_number, errmsg);
628639

629640
/*
@@ -644,6 +655,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
644655
if (!(sym= dlsym(plugin_dl.handle, plugin_interface_version_sym)))
645656
{
646657
free_plugin_mem(&plugin_dl);
658+
mysql_mutex_unlock(&LOCK_plugin);
647659
report_error(report, ER_CANT_FIND_DL_ENTRY, plugin_interface_version_sym);
648660
DBUG_RETURN(NULL);
649661
}
@@ -653,6 +665,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
653665
(plugin_dl.version >> 8) > (MYSQL_PLUGIN_INTERFACE_VERSION >> 8))
654666
{
655667
free_plugin_mem(&plugin_dl);
668+
mysql_mutex_unlock(&LOCK_plugin);
656669
report_error(report, ER_CANT_OPEN_LIBRARY, dlpath, 0,
657670
"plugin interface version mismatch");
658671
DBUG_RETURN(NULL);
@@ -672,6 +685,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
672685
my_snprintf(buf, sizeof(buf),
673686
"service '%s' interface version mismatch",
674687
list_of_services[i].name);
688+
mysql_mutex_unlock(&LOCK_plugin);
675689
report_error(report, ER_CANT_OPEN_LIBRARY, dlpath, 0, buf);
676690
DBUG_RETURN(NULL);
677691
}
@@ -683,6 +697,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
683697
if (!(sym= dlsym(plugin_dl.handle, plugin_declarations_sym)))
684698
{
685699
free_plugin_mem(&plugin_dl);
700+
mysql_mutex_unlock(&LOCK_plugin);
686701
report_error(report, ER_CANT_FIND_DL_ENTRY, plugin_declarations_sym);
687702
DBUG_RETURN(NULL);
688703
}
@@ -721,6 +736,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
721736
if (!cur)
722737
{
723738
free_plugin_mem(&plugin_dl);
739+
mysql_mutex_unlock(&LOCK_plugin);
724740
report_error(report, ER_OUTOFMEMORY,
725741
static_cast<int>(plugin_dl.dl.length));
726742
DBUG_RETURN(NULL);
@@ -750,6 +766,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
750766
for ( ; plugin->info ; ++plugin)
751767
if (plugin->flags & PLUGIN_OPT_NO_INSTALL)
752768
{
769+
mysql_mutex_unlock(&LOCK_plugin);
753770
report_error(report, ER_PLUGIN_NO_INSTALL, plugin->name);
754771
free_plugin_mem(&plugin_dl);
755772
DBUG_RETURN(NULL);
@@ -761,6 +778,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
761778
if (! (plugin_dl.dl.str= (char*) my_malloc(key_memory_mysql_plugin_dl,
762779
plugin_dl.dl.length, MYF(0))))
763780
{
781+
mysql_mutex_unlock(&LOCK_plugin);
764782
free_plugin_mem(&plugin_dl);
765783
report_error(report, ER_OUTOFMEMORY,
766784
static_cast<int>(plugin_dl.dl.length));
@@ -773,6 +791,7 @@ static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
773791
/* Add this dll to array */
774792
if (! (tmp= plugin_dl_insert_or_reuse(&plugin_dl)))
775793
{
794+
mysql_mutex_unlock(&LOCK_plugin);
776795
free_plugin_mem(&plugin_dl);
777796
report_error(report, ER_OUTOFMEMORY,
778797
static_cast<int>(sizeof(st_plugin_dl)));
@@ -956,9 +975,12 @@ static st_plugin_int *plugin_insert_or_reuse(st_plugin_int *plugin)
956975
}
957976

958977

959-
/*
960-
NOTE
961-
Requires that a write-lock is held on LOCK_system_variables_hash
978+
/**
979+
Adds a plugin to the global plugin list.
980+
981+
Also installs the plugin variables.
982+
In case of error releases ::LOCK_plugin and reports the error
983+
@note Requires that a write-lock is held on ::LOCK_system_variables_hash
962984
*/
963985
static bool plugin_add(MEM_ROOT *tmp_root,
964986
const LEX_STRING *name, const LEX_STRING *dl,
@@ -974,13 +996,14 @@ static bool plugin_add(MEM_ROOT *tmp_root,
974996
{
975997
mysql_mutex_unlock(&LOCK_plugin);
976998
report_error(report, ER_UDF_EXISTS, name->str);
977-
mysql_mutex_lock(&LOCK_plugin);
978-
DBUG_RETURN(TRUE);
999+
DBUG_RETURN(true);
9791000
}
9801001
/* Clear the whole struct to catch future extensions. */
9811002
memset(&tmp, 0, sizeof(tmp));
982-
if (! (tmp.plugin_dl= plugin_dl_add(dl, report)))
983-
DBUG_RETURN(TRUE);
1003+
if (!(tmp.plugin_dl = plugin_dl_add(dl, report)))
1004+
{
1005+
DBUG_RETURN(true);
1006+
}
9841007
/* Find plugin by name */
9851008
for (plugin= tmp.plugin_dl->plugins; plugin->info; plugin++)
9861009
{
@@ -997,14 +1020,17 @@ static bool plugin_add(MEM_ROOT *tmp_root,
9971020
((*(int*)plugin->info) >> 8) >
9981021
(cur_plugin_info_interface_version[plugin->type] >> 8))
9991022
{
1000-
char buf[256];
1023+
char buf[256], dl_name[FN_REFLEN];
10011024
strxnmov(buf, sizeof(buf) - 1, "API version for ",
10021025
plugin_type_names[plugin->type].str,
10031026
" plugin is too different", NullS);
1027+
/* copy the library name so we can release the mutex */
1028+
strncpy(dl_name, dl->str, sizeof(dl_name) - 1);
1029+
dl_name[sizeof(dl_name) - 1] = 0;
1030+
plugin_dl_del(dl);
10041031
mysql_mutex_unlock(&LOCK_plugin);
1005-
report_error(report, ER_CANT_OPEN_LIBRARY, dl->str, 0, buf);
1006-
mysql_mutex_lock(&LOCK_plugin);
1007-
goto err;
1032+
report_error(report, ER_CANT_OPEN_LIBRARY, dl_name, 0, buf);
1033+
DBUG_RETURN(true);
10081034
}
10091035
tmp.plugin= plugin;
10101036
tmp.name.str= (char *)plugin->name;
@@ -1023,25 +1049,21 @@ static bool plugin_add(MEM_ROOT *tmp_root,
10231049
{
10241050
init_alloc_root(key_memory_plugin_int_mem_root,
10251051
&tmp_plugin_ptr->mem_root, 4096, 4096);
1026-
DBUG_RETURN(FALSE);
1052+
DBUG_RETURN(false);
10271053
}
10281054
tmp_plugin_ptr->state= PLUGIN_IS_FREED;
10291055
}
10301056
mysql_del_sys_var_chain(tmp.system_vars);
10311057
restore_pluginvar_names(tmp.system_vars);
1032-
goto err;
1033-
1034-
/* plugin was disabled */
10351058
plugin_dl_del(dl);
1036-
DBUG_RETURN(FALSE);
1059+
mysql_mutex_unlock(&LOCK_plugin);
1060+
DBUG_RETURN(true);
10371061
}
10381062
}
1063+
plugin_dl_del(dl);
10391064
mysql_mutex_unlock(&LOCK_plugin);
10401065
report_error(report, ER_CANT_FIND_DL_ENTRY, name->str);
1041-
mysql_mutex_lock(&LOCK_plugin);
1042-
err:
1043-
plugin_dl_del(dl);
1044-
DBUG_RETURN(TRUE);
1066+
DBUG_RETURN(true);
10451067
}
10461068

10471069

@@ -1709,8 +1731,17 @@ static bool register_builtin(st_mysql_plugin *plugin,
17091731
}
17101732

17111733

1712-
/*
1713-
called only by plugin_register_dynamic_and_init_all()
1734+
/**
1735+
Reads the plugins from mysql.plugin and loads them
1736+
1737+
Called only by plugin_register_dynamic_and_init_all()
1738+
a.k.a. the bootstrap sequence.
1739+
1740+
@arg tmp_root memory root to use for plugin_add()
1741+
@arg argc number of command line arguments to process
1742+
@arg argv array of command line argument to read values from
1743+
@retval true failure
1744+
@retval false success
17141745
*/
17151746
static void plugin_load(MEM_ROOT *tmp_root, int *argc, char **argv)
17161747
{
@@ -1751,7 +1782,6 @@ static void plugin_load(MEM_ROOT *tmp_root, int *argc, char **argv)
17511782
environment, and it uses mysql_mutex_assert_owner(), so we lock
17521783
the mutex here to satisfy the assert
17531784
*/
1754-
mysql_mutex_lock(&LOCK_plugin);
17551785
while (!(error= read_record_info.read_record(&read_record_info)))
17561786
{
17571787
DBUG_PRINT("info", ("init plugin record"));
@@ -1762,12 +1792,27 @@ static void plugin_load(MEM_ROOT *tmp_root, int *argc, char **argv)
17621792
LEX_STRING name= {(char *)str_name.ptr(), str_name.length()};
17631793
LEX_STRING dl= {(char *)str_dl.ptr(), str_dl.length()};
17641794

1795+
/*
1796+
The whole locking sequence is not strictly speaking needed since this
1797+
is a function that's executed only during server bootstrap, but we do
1798+
it properly for uniformity of the environment for plugin_add.
1799+
Note that it must be done for each iteration since, unlike INSTALL PLUGIN
1800+
the bootstrap process just reports the error and goes on.
1801+
So to ensure the right sequence of lock and unlock we need to take and
1802+
release both the wlock and the mutex.
1803+
*/
1804+
mysql_mutex_lock(&LOCK_plugin);
1805+
mysql_rwlock_wrlock(&LOCK_system_variables_hash);
17651806
if (plugin_add(tmp_root, &name, &dl, argc, argv, REPORT_TO_LOG))
1807+
{
17661808
LogErr(WARNING_LEVEL, ER_PLUGIN_CANT_LOAD,
1767-
str_name.c_ptr(), str_dl.c_ptr());
1809+
str_name.c_ptr(), str_dl.c_ptr());
1810+
}
1811+
else
1812+
mysql_mutex_unlock(&LOCK_plugin);
1813+
mysql_rwlock_unlock(&LOCK_system_variables_hash);
17681814
free_root(tmp_root, MYF(MY_MARK_BLOCKS_FREE));
17691815
}
1770-
mysql_mutex_unlock(&LOCK_plugin);
17711816
if (error > 0)
17721817
{
17731818
char errbuf[MYSQL_ERRMSG_SIZE];
@@ -1783,9 +1828,18 @@ static void plugin_load(MEM_ROOT *tmp_root, int *argc, char **argv)
17831828
}
17841829

17851830

1786-
/*
1787-
called by plugin_register_early_plugins() and
1788-
plugin_register_dynamic_and_init_all()
1831+
/**
1832+
Load a list of plugins
1833+
1834+
Called by plugin_register_early_plugins() and
1835+
plugin_register_dynamic_and_init_all(), a.k.a. the bootstrap sequence.
1836+
1837+
@arg tmp_root memory root to use for plugin_add()
1838+
@arg argc number of command line arguments to process
1839+
@arg argv array of command line argument to read values from
1840+
@arg list list of plugins to load. Ends with a NULL pointer
1841+
@retval true failure
1842+
@retval false success
17891843
*/
17901844
static bool plugin_load_list(MEM_ROOT *tmp_root, int *argc, char **argv,
17911845
const char *list)
@@ -1822,7 +1876,13 @@ static bool plugin_load_list(MEM_ROOT *tmp_root, int *argc, char **argv,
18221876
}
18231877

18241878
dl= name;
1879+
/*
1880+
The whole locking sequence is not strictly speaking needed since this
1881+
is a function that's executed only during server bootstrap, but we do
1882+
it properly for uniformity of the environment for plugin_add.
1883+
*/
18251884
mysql_mutex_lock(&LOCK_plugin);
1885+
mysql_rwlock_wrlock(&LOCK_system_variables_hash);
18261886
if ((plugin_dl= plugin_dl_add(&dl, REPORT_TO_LOG)))
18271887
{
18281888
for (plugin= plugin_dl->plugins; plugin->info; plugin++)
@@ -1832,19 +1892,37 @@ static bool plugin_load_list(MEM_ROOT *tmp_root, int *argc, char **argv,
18321892

18331893
free_root(tmp_root, MYF(MY_MARK_BLOCKS_FREE));
18341894
if (plugin_add(tmp_root, &name, &dl, argc, argv, REPORT_TO_LOG))
1895+
{
1896+
mysql_rwlock_unlock(&LOCK_system_variables_hash);
18351897
goto error;
1898+
}
18361899
}
18371900
plugin_dl_del(&dl); // reduce ref count
18381901
}
1902+
else
1903+
{
1904+
mysql_rwlock_unlock(&LOCK_system_variables_hash);
1905+
goto error;
1906+
}
18391907
}
18401908
else
18411909
{
18421910
free_root(tmp_root, MYF(MY_MARK_BLOCKS_FREE));
1911+
/*
1912+
The whole locking sequence is not strictly speaking needed since this
1913+
is a function that's executed only during server bootstrap, but we do
1914+
it properly for uniformity of the environment for plugin_add.
1915+
*/
18431916
mysql_mutex_lock(&LOCK_plugin);
1917+
mysql_rwlock_wrlock(&LOCK_system_variables_hash);
18441918
if (plugin_add(tmp_root, &name, &dl, argc, argv, REPORT_TO_LOG))
1919+
{
1920+
mysql_rwlock_unlock(&LOCK_system_variables_hash);
18451921
goto error;
1922+
}
18461923
}
18471924
mysql_mutex_unlock(&LOCK_plugin);
1925+
mysql_rwlock_unlock(&LOCK_system_variables_hash);
18481926
name.length= dl.length= 0;
18491927
dl.str= NULL; name.str= p= buffer;
18501928
str= &name;
@@ -1866,7 +1944,6 @@ static bool plugin_load_list(MEM_ROOT *tmp_root, int *argc, char **argv,
18661944
}
18671945
DBUG_RETURN(FALSE);
18681946
error:
1869-
mysql_mutex_unlock(&LOCK_plugin);
18701947
LogErr(ERROR_LEVEL, ER_PLUGIN_CANT_LOAD, name.str, dl.str);
18711948
DBUG_RETURN(TRUE);
18721949
}
@@ -2172,8 +2249,8 @@ static bool mysql_install_plugin(THD *thd, const LEX_STRING *name,
21722249
&argc, &argv, NULL))
21732250
{
21742251
mysql_rwlock_unlock(&LOCK_system_variables_hash);
2175-
report_error(REPORT_TO_USER, ER_PLUGIN_IS_NOT_LOADED, name->str);
21762252
mysql_mutex_unlock(&LOCK_plugin);
2253+
report_error(REPORT_TO_USER, ER_PLUGIN_IS_NOT_LOADED, name->str);
21772254
goto err;
21782255
}
21792256
default_argv= argv;
@@ -2185,16 +2262,20 @@ static bool mysql_install_plugin(THD *thd, const LEX_STRING *name,
21852262
if (pv && pv->append_read_only_variables(&argc, &argv, TRUE))
21862263
{
21872264
mysql_rwlock_unlock(&LOCK_system_variables_hash);
2188-
report_error(REPORT_TO_USER, ER_PLUGIN_IS_NOT_LOADED, name->str);
21892265
mysql_mutex_unlock(&LOCK_plugin);
2266+
report_error(REPORT_TO_USER, ER_PLUGIN_IS_NOT_LOADED, name->str);
21902267
goto err;
21912268
}
21922269
error= plugin_add(thd->mem_root, name, dl, &argc, argv, REPORT_TO_USER);
21932270
if (default_argv)
21942271
free_defaults(default_argv);
21952272
mysql_rwlock_unlock(&LOCK_system_variables_hash);
21962273

2197-
if (error || !(tmp= plugin_find_internal(name_cstr, MYSQL_ANY_PLUGIN)))
2274+
/* LOCK_plugin already unlocked by plugin_add() if error */
2275+
if (error)
2276+
goto err;
2277+
2278+
if (!(tmp= plugin_find_internal(name_cstr, MYSQL_ANY_PLUGIN)))
21982279
{
21992280
mysql_mutex_unlock(&LOCK_plugin);
22002281
goto err;

0 commit comments

Comments
 (0)