-
Notifications
You must be signed in to change notification settings - Fork 518
PS-11143-[8.4] sql/auth: quiesce caching_sha2_password before teardown #6028
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 8.4
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| # | ||
| # Verify auth callback drains before plugin deinit with threadpool | ||
| # | ||
| # restart:--thread-handling=pool-of-threads --thread-pool-size=2 --thread-pool-max-threads=4 | ||
| CREATE USER tp_user@127.0.0.1 IDENTIFIED WITH caching_sha2_password BY 'tp_pwd'; | ||
| SET GLOBAL debug = "+d,auth_plugin_before_callback_sync"; | ||
| 1 | ||
| 1 | ||
| SET DEBUG_SYNC='now WAIT_FOR auth_plugin_before_callback_entered TIMEOUT 30'; | ||
| # restart | ||
| DROP USER IF EXISTS tp_user@127.0.0.1; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| --source include/not_windows.inc | ||
| --source include/have_debug.inc | ||
| --source include/have_debug_sync.inc | ||
|
|
||
| --echo # | ||
| --echo # Verify auth callback drains before plugin deinit with threadpool | ||
| --echo # | ||
|
|
||
| --let $restart_parameters=restart:--thread-handling=pool-of-threads --thread-pool-size=2 --thread-pool-max-threads=4 | ||
| --source include/restart_mysqld.inc | ||
| --source include/have_pool_of_threads.inc | ||
|
|
||
| CREATE USER tp_user@127.0.0.1 IDENTIFIED WITH caching_sha2_password BY 'tp_pwd'; | ||
|
|
||
| SET GLOBAL debug = "+d,auth_plugin_before_callback_sync"; | ||
|
|
||
| --let $mysqld_pid_file=`SELECT @@GLOBAL.pid_file` | ||
|
|
||
| --let $output_file= $MYSQLTEST_VARDIR/tmp/bug_ps11143_mysql_output | ||
| --let $pid_file= $MYSQLTEST_VARDIR/tmp/bug_ps11143_mysql_pid | ||
| --let $sql_file= $MYSQLTEST_VARDIR/tmp/bug_ps11143_query.sql | ||
| --write_file $sql_file | ||
| SELECT 1; | ||
| EOF | ||
| --let $command= $MYSQL | ||
| --let $command_opt= --protocol=tcp --host=127.0.0.1 --port=$MASTER_MYPORT --user=tp_user --password=tp_pwd < $sql_file | ||
| --let $redirect_stderr= 1 | ||
| --source include/start_proc_in_background.inc | ||
|
|
||
| SET DEBUG_SYNC='now WAIT_FOR auth_plugin_before_callback_entered TIMEOUT 30'; | ||
|
|
||
| --source include/expect_crash.inc | ||
| exec kill -TERM `cat $mysqld_pid_file`; | ||
|
|
||
| --source include/wait_until_disconnected.inc | ||
| --source include/wait_proc_to_finish.inc | ||
|
|
||
| --let $restart_parameters= | ||
| --source include/start_mysqld.inc | ||
|
|
||
| DROP USER IF EXISTS tp_user@127.0.0.1; | ||
|
|
||
| --remove_file $pid_file | ||
| --remove_file $output_file | ||
| --remove_file $sql_file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /* Copyright (c) 2026, Percona LLC and/or its affiliates. | ||
|
|
||
| This program is free software; you can redistribute it and/or modify | ||
| it under the terms of the GNU General Public License, version 2.0, | ||
| as published by the Free Software Foundation. | ||
|
|
||
| This program is designed to work with certain software (including | ||
| but not limited to OpenSSL) that is licensed under separate terms, | ||
| as designated in a particular file or component or in included license | ||
| documentation. The authors of MySQL hereby grant you an additional | ||
| permission to link the program and your derivative works with the | ||
| separately licensed software that they have either included with | ||
| the program or referenced in the documentation. | ||
|
|
||
| This program is distributed in the hope that it will be useful, | ||
| but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| GNU General Public License, version 2.0, for more details. | ||
|
|
||
| You should have received a copy of the GNU General Public License | ||
| along with this program; if not, write to the Free Software | ||
| Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ | ||
|
|
||
| #ifndef AUTH_PLUGIN_SHUTDOWN_INCLUDED | ||
| #define AUTH_PLUGIN_SHUTDOWN_INCLUDED | ||
|
|
||
| bool begin_auth_plugin_operation(); | ||
| void end_auth_plugin_operation(); | ||
| void start_auth_plugin_shutdown_and_wait(); | ||
|
|
||
| class Auth_plugin_operation_guard { | ||
| public: | ||
| Auth_plugin_operation_guard() | ||
| : m_has_operation(begin_auth_plugin_operation()) {} | ||
|
|
||
| ~Auth_plugin_operation_guard() { | ||
| if (m_has_operation) end_auth_plugin_operation(); | ||
| } | ||
|
|
||
| bool has_operation() const { return m_has_operation; } | ||
| explicit operator bool() const { return m_has_operation; } | ||
|
|
||
| private: | ||
| bool m_has_operation; | ||
| }; | ||
|
|
||
| #endif // AUTH_PLUGIN_SHUTDOWN_INCLUDED |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,8 @@ | |
| #include <atomic> | ||
| #include <boost/algorithm/string/classification.hpp> | ||
| #include <boost/algorithm/string/split.hpp> | ||
| #include <condition_variable> | ||
| #include <mutex> | ||
| #include <string> /* std::string */ | ||
| #include <utility> | ||
| #include <vector> /* std::vector */ | ||
|
|
@@ -77,6 +79,7 @@ | |
| #include "sql/auth/auth_acls.h" | ||
| #include "sql/auth/auth_common.h" | ||
| #include "sql/auth/auth_internal.h" // optimize_plugin_compare_by_pointer | ||
| #include "sql/auth/auth_plugin_shutdown.h" | ||
| #include "sql/auth/partial_revokes.h" | ||
| #include "sql/auth/sql_auth_cache.h" // acl_cache | ||
| #include "sql/auth/sql_security_ctx.h" | ||
|
|
@@ -1286,6 +1289,52 @@ int security_level(void) { | |
| external_roles_t g_external_roles; | ||
| Cached_authentication_plugins *g_cached_authentication_plugins = nullptr; | ||
|
|
||
| namespace { | ||
|
|
||
| class Auth_plugin_shutdown_state { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this is defined here? It is rather tightly related to Auth_plugin_operation_guard.
|
||
| public: | ||
| bool begin_operation() { | ||
| std::lock_guard<std::mutex> lock(m_mutex); | ||
| if (m_shutting_down) return false; | ||
| ++m_active_operations; | ||
| return true; | ||
| } | ||
|
|
||
| void end_operation() { | ||
| std::lock_guard<std::mutex> lock(m_mutex); | ||
| assert(m_active_operations > 0); | ||
| if (--m_active_operations == 0) m_cv.notify_all(); | ||
| } | ||
|
|
||
| void start_shutdown_and_wait() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering what happens when:
If the threads are stopped first (seems to be more logical to me), how it is done, does the main wait until all connections are "gently" closed? I suppose at least there is some timeout. Then (I guess) sth like pthread_cancel() may be used and if so, probably m_active_operations never reach 0 and the server hangs. For that scenario I recommend replacing wait() with wait_for(). If the order is reverse, the hank is even more obvious. |
||
| std::unique_lock<std::mutex> lock(m_mutex); | ||
| m_shutting_down = true; | ||
| m_cv.wait(lock, [&] { return m_active_operations == 0; }); | ||
| } | ||
|
|
||
| private: | ||
| std::mutex m_mutex; | ||
| std::condition_variable m_cv; | ||
| size_t m_active_operations = 0; | ||
| bool m_shutting_down = false; | ||
| }; | ||
|
|
||
| Auth_plugin_shutdown_state g_auth_plugin_shutdown_state; | ||
|
|
||
| } // namespace | ||
|
|
||
| bool begin_auth_plugin_operation() { | ||
| return g_auth_plugin_shutdown_state.begin_operation(); | ||
| } | ||
|
|
||
| void end_auth_plugin_operation() { | ||
| g_auth_plugin_shutdown_state.end_operation(); | ||
| } | ||
|
|
||
| void start_auth_plugin_shutdown_and_wait() { | ||
| g_auth_plugin_shutdown_state.start_shutdown_and_wait(); | ||
| } | ||
|
|
||
| bool disconnect_on_expired_password = true; | ||
|
|
||
| extern bool initialized; | ||
|
|
@@ -3565,7 +3614,18 @@ static int do_auth_once(THD *thd, const LEX_CSTRING &auth_plugin_name, | |
|
|
||
| if (plugin) { | ||
| st_mysql_auth *auth = (st_mysql_auth *)plugin_decl(plugin)->info; | ||
| res = auth->authenticate_user(mpvio, &mpvio->auth_info); | ||
| Auth_plugin_operation_guard op_guard; | ||
| if (op_guard) { | ||
| DBUG_EXECUTE_IF("auth_plugin_before_callback_sync", { | ||
| const char act[] = "now SIGNAL auth_plugin_before_callback_entered"; | ||
| assert(!debug_sync_set_action(thd, STRING_WITH_LEN(act))); | ||
| my_sleep(2000000); | ||
| };); | ||
| res = auth->authenticate_user(mpvio, &mpvio->auth_info); | ||
| } else { | ||
| my_error(ER_SERVER_SHUTDOWN, MYF(0)); | ||
| res = CR_ERROR; | ||
| } | ||
|
|
||
| if (unlock_plugin) plugin_unlock(thd, plugin); | ||
| } else { | ||
|
|
@@ -3698,7 +3758,13 @@ static int do_multi_factor_auth(THD *thd, MPVIO_EXT *mpvio) { | |
| .auth_string_length; | ||
| mpvio->status = MPVIO_EXT::START_MFA; | ||
| st_mysql_auth *auth = (st_mysql_auth *)plugin_decl(plugin)->info; | ||
| res = auth->authenticate_user(mpvio, &mpvio->auth_info); | ||
| Auth_plugin_operation_guard op_guard; | ||
| if (op_guard) { | ||
| res = auth->authenticate_user(mpvio, &mpvio->auth_info); | ||
| } else { | ||
| my_error(ER_SERVER_SHUTDOWN, MYF(0)); | ||
| res = CR_ERROR; | ||
| } | ||
| if (res == CR_OK_AUTH_IN_SANDBOX_MODE) { | ||
| /* | ||
| Server allows user account to connect in case registration is | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove formatting not relevant to the fix. Does this file have to be modified at all?