Closed laurynas-biveinis closed 1 year ago
laurynas-biveinis
commented
1 year ago Other tests that show the same issue: rpl_nogtid.rpl_semi_sync_sender_observe_commit_only, rpl_nogtid.rpl_semi_sync, rpl_nogtid.rpl_semi_sync_optimize_for_static_plugin_config, rpl_nogtid.rpl_ssl_semisync, rpl_nogtid.rpl_semi_sync_uninstall_plugin
hermanlee
commented
1 year ago While the fix might resolve the double free, it seems like it opens up the possibility of lost memory. Based on the stack traces, the semi-sync plugin is being de-initialized/de-allocated and freeing up its internal memory. However, later in the same thread, there is a call to pfs to fetch data from a plugin that is being deallocated, so memory is reallocated for the histograms. It's not clear when this allocated memory is freed because de-initialization has already been called.
It seems like this is an upstream bug where the system_vars should be removed/disabled when the status_vars are being disabled in plugin_deinitialize(). Instead, system_vars are removed in plugin_del(). Would the right fix be to move the code around system_vars remove from plugin_del() to plugin_deinitialize()?
hermanlee
commented
1 year ago Maybe the following diff?
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc
index 9aee2367d64..8dd4c0728b7 100644
--- a/sql/sql_plugin.cc
+++ b/sql/sql_plugin.cc
@@ -1113,6 +1113,10 @@ static void plugin_deinitialize(st_plugin_int *plugin, bool ref_check) {
remove_status_vars(plugin->plugin->status_vars);
}
+ mysql_rwlock_wrlock(&LOCK_system_variables_hash);
+ mysql_del_sys_var_chain(plugin->system_vars);
+ mysql_rwlock_unlock(&LOCK_system_variables_hash);
+
if (plugin_type_deinitialize[plugin->plugin->type]) {
if ((*plugin_type_deinitialize[plugin->plugin->type])(plugin)) {
LogErr(ERROR_LEVEL, ER_PLUGIN_FAILED_DEINITIALIZATION, plugin->name.str,While the fix might resolve the double free, it seems like it opens up the possibility of lost memory. Based on the stack traces, the semi-sync plugin is being de-initialized/de-allocated and freeing up its internal memory. However, later in the same thread, there is a call to pfs to fetch data from a plugin that is being deallocated, so memory is reallocated for the histograms. It's not clear when this allocated memory is freed because de-initialization has already been called.
@hermanlee , could you please point to the source location for "later in the same thread, there is a call to pfs to fetch data from a plugin that is being deallocated, so memory is reallocated for the histograms"?
hermanlee
commented
1 year ago This is based on the stack trace provided.
plugin_deinitialize() -> semi_sync_master_plugin_deinit() -> ReplSemiSyncMaster::~ReplSemiSyncMaster() -> free_latency_histogram_sysvars() is where the memory is freed.
The double free original crash occurred in ha_perfschema::rnd_init() -> PFS_status_variable_cache::manifest() -> prepare_latency_histogram_vars() because free_latency_histogram_sysvar() was called during plugin_deinitalize() and the pointer was kept around. Your fix changes the pointer to nulltpr during the free, so that prepare_latency_histogram_vars() no longer crashes on the double free. However, semi_sync_master_plugin_deinit() has already been called, and nothing outside the plugin calls into free_latency_histogram_sysvar() again. Once the perfschema run completes, reap_plugins() then completes and the memory allocated by perf_schema in prepare_latency_histogram_vars() never gets freed.
hermanlee
commented
1 year ago This is based on the stack trace provided.
plugin_deinitialize() -> semi_sync_master_plugin_deinit() -> ReplSemiSyncMaster::~ReplSemiSyncMaster() -> free_latency_histogram_sysvars() is where the memory is freed.
The double free original crash occurred in ha_perfschema::rnd_init() -> PFS_status_variable_cache::manifest() -> prepare_latency_histogram_vars() because free_latency_histogram_sysvar() was called during plugin_deinitalize() and the pointer was kept around. Your fix changes the pointer to nulltpr during the free, so that prepare_latency_histogram_vars() no longer crashes on the double free. However, semi_sync_master_plugin_deinit() has already been called, and nothing outside the plugin calls into free_latency_histogram_sysvar() again. Once the perfschema run completes, reap_plugins() then completes and the memory allocated by perf_schema in prepare_latency_histogram_vars() never gets freed.
laurynas-biveinis
commented
1 year ago @hermanlee, I have reviewed as much as I was able to, and to the best of my understanding your proposed patch is correct. I'd check the whole testsuite under ASan or Valgrind for the unknown unknowns, and then report upstream and try to upstream the patch or at least get their opinion.
Since this is your patch, do you want to work on this or do you want me to take it?
hermanlee
commented
1 year ago Does the patch resolve the ASAN issues you saw? I can file a bug report for this upstream, but generating a test case might be more difficult. Did you discover how the performance schema call was being made on the plugin after it was de-initialized while running on the same thread?
laurynas-biveinis
commented
1 year ago I made an error in my testing. The proposed patch does not fix the issue, and the sequence of actions is different than assumed. The following three things happen: 1) The plugin is uninstalled, freeing the histograms the first time. Note that this step can be considered atomic, i.e. it does not matter if it's free'ed at deinitialization or unload. 2) The plugin is re-installed again. 3) A status variable query happens, freeing the histograms the second time.
I am analyzing this further. I guess we need to re-check whether my original patch results in a memory leak, as in the above scenario there will be one more plugin unload.
laurynas-biveinis
commented
1 year ago I am still not sure what's going on. The crash/ASan error reproduces on macOS, but not on Linux. I cannot tell yet whether Linux is lucky with the bug being present there but not tripping, or whether there is no bug on Linux in the first place. The previous comment describes what is happening on macOS. On Linux, there is no 2nd free.
laurynas-biveinis
commented
1 year ago I got to the bottom of it and the PR is ready for review. Repeated installation, use, and uninstallation of a plugin results in platform-specific behavior with respect to uninitialized variables in the BSS section. For ELF, they are zero-initialized on every load. For mach-O, apparently they were on the first load, but not later (maybe the unload is postponed?).
It is possible to reproduce the issue on Linux by adding RTLD_NODELETE flag to dlopen call in plugin_dl_add, which is documented as "Do not unload the shared object during dlclose(). Consequently, the object's static and global variables are not reinitialized if the object is reloaded with dlopen() at a later time.", and then the fix works too.
POSIX docs for dlopen do not say anything about BSS and repeated loading.
facebook-github-bot
commented
1 year ago @hermanlee has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
hermanlee
commented
1 year ago Awesome, thanks for tracking this down.
Avoid double free on latency histogram data
Before this fix, rpl.rpl_semi_sync_alias test under ASan with
It seems that the double invocation of
free_latency_histogram_sysvarsis correct in this case, thus protect against the double free with resetting the pointers to nullptr.Squash with 19345e371c976884e3533fd05a71113c1a3646a6