BUG#38425487 mysql_command_factory->connect() failure when running in explicit mode leaks session due to premature cleanup

Issue ===== When mysql_command_services runs in explicit mode (thread initialized via cmd_thread_srv->init()), a failed connect() (e.g., bad username) leaks the server session owned by the command-services backend. The error log shows: [ERROR] [MY-010470] [Server] Closed forcefully 1 session left opened by plugin server_service Root cause ========== Two problems overlapped: 1) On failure, connect_helper() calls end_server() + mysql_close_free(), which continues into mysql_extension_free(). However, in early failure paths the session service pointer (mcs_extn->session_svc) could be set too late, so teardown had no valid session to close. 2) The consumer service release and session handling lives in the component's close(), which is not guaranteed to run on early connect() failures. The failure path already invoked mysql_close_free() at that point. Fix === - Centralize server-backed cleanup in mysql_extension_free() to ensure that it always closes the backend session and resets vars and frees ext->mcs_extn accordingly. Also, to release the consumer services acquired earlier. - Assign mcs_extn->session_svc earlier in cssm_begin_connect() so that all early failures still have a valid session handle for cleanup. - Make the component close() a thin wrapper that releases consumer services and calls mysql_close() only. Change-Id: I36bbf60280a065cc2481a3ae829ef299b08e1592

Author: miguelaraujo

components/test/test_mysql_command_services.cc

@@ -30,6 +30,7 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */
 #include <mysql/service_srv_session_info.h>
 #include <cstdio>
 #include <cstring>
+#include <thread>
 
 REQUIRES_SERVICE_PLACEHOLDER_AS(mysql_thd_security_context, thd_security_ctx);
 REQUIRES_SERVICE_PLACEHOLDER_AS(mysql_account_database_security_context_lookup,
@@ -38,6 +39,7 @@ REQUIRES_SERVICE_PLACEHOLDER_AS(mysql_security_context_options,
  security_ctx_options);
 REQUIRES_SERVICE_PLACEHOLDER_AS(udf_registration, udf_srv);
 REQUIRES_SERVICE_PLACEHOLDER_AS(mysql_command_factory, cmd_factory_srv);
+REQUIRES_SERVICE_PLACEHOLDER_AS(mysql_command_thread, cmd_thread_srv);
 REQUIRES_SERVICE_PLACEHOLDER_AS(mysql_command_options, cmd_options_srv);
 REQUIRES_SERVICE_PLACEHOLDER_AS(mysql_command_query, cmd_query_srv);
 REQUIRES_SERVICE_PLACEHOLDER_AS(mysql_command_query_result,
@@ -57,6 +59,7 @@ REQUIRES_SERVICE_AS(udf_registration, udf_srv),
  account_db_security_ctx_lookup),
  REQUIRES_SERVICE_AS(mysql_security_context_options, security_ctx_options),
  REQUIRES_SERVICE_AS(mysql_command_factory, cmd_factory_srv),
+ REQUIRES_SERVICE_AS(mysql_command_thread, cmd_thread_srv),
  REQUIRES_SERVICE_AS(mysql_command_options, cmd_options_srv),
  REQUIRES_SERVICE_AS(mysql_command_query, cmd_query_srv),
  REQUIRES_SERVICE_AS(mysql_command_query_result, cmd_query_result_srv),
@@ -389,6 +392,57 @@ static long long test_mysql_command_services_error_code_udf(
  return static_cast<long long>(err_no);
 }
 
+// Run in thread + failed connect + cleanup
+static long long test_mysql_command_services_explicit_connect_fail_cleanup_udf(
+ UDF_INIT *, UDF_ARGS *, unsigned char *is_null, unsigned char *error) {
+ *is_null = 0;
+ *error = 1;
+
+ constexpr const char *bad_user = "no_such_user";
+ constexpr const char *host = "localhost";
+
+ long long ret_err = -1;
+
+ // Spawn a thread, since the cmd thread must run off the server's main session
+ // thread
+ std::thread worker([&] {
+ MYSQL_H mysql_h = nullptr;
+
+ if (cmd_thread_srv->init() != 0) return;
+
+ do {
+ if (cmd_factory_srv->init(&mysql_h) != 0 || mysql_h == nullptr) break;
+
+ // Set the options
+ if (cmd_options_srv->set(mysql_h, MYSQL_NO_LOCK_REGISTRY,
+ reinterpret_cast<void *>(1)) != 0 ||
+ cmd_options_srv->set(mysql_h, MYSQL_COMMAND_USER_NAME, bad_user) !=
+ 0 ||
+ cmd_options_srv->set(mysql_h, MYSQL_COMMAND_HOST_NAME, host) != 0) {
+ cmd_factory_srv->close(mysql_h);
+ mysql_h = nullptr;
+ break; // Failed setting the options, exit
+ }
+
+ // Expect failure: wrong user
+ ret_err = (cmd_factory_srv->connect(mysql_h) != 0) ? 1 : 0;
+
+ // Always close the handle even on failure
+ cmd_factory_srv->close(mysql_h);
+ mysql_h = nullptr;
+ } while (false);
+
+ cmd_thread_srv->end();
+ });
+
+ worker.join();
+
+ *error = (ret_err == -1)
+ ? 1
+ : 0; // only mark UDF-level error if we never tried connect
+ return ret_err;
+}
+
 static mysql_service_status_t init() {
  Udf_func_string udf1 = test_mysql_command_services_udf;
  if (udf_srv->udf_register("test_mysql_command_services_udf", STRING_RESULT,
@@ -416,6 +470,18 @@ static mysql_service_status_t init() {
  return 1;
  }
 
+ Udf_func_longlong udf4 =
+ test_mysql_command_services_explicit_connect_fail_cleanup_udf;
+ if (udf_srv->udf_register(
+ "test_mysql_command_services_explicit_connect_fail_cleanup_udf",
+ INT_RESULT, reinterpret_cast<Udf_func_any>(udf4), nullptr, nullptr)) {
+ fprintf(
+ stderr,
+ "Can't register "
+ "test_mysql_command_services_explicit_connect_fail_cleanup_udf UDF\n");
+ return 1;
+ }
+
  return 0;
 }
 
@@ -433,6 +499,13 @@ static mysql_service_status_t deinit() {
  fprintf(stderr,
  "Can't unregister the test_mysql_command_services_error_code_udf "
  "UDF\n");
+ if (udf_srv->udf_unregister(
+ "test_mysql_command_services_explicit_connect_fail_cleanup_udf",
+ &was_present))
+ fprintf(
+ stderr,
+ "Can't unregister "
+ "test_mysql_command_services_explicit_connect_fail_cleanup_udf UDF\n");
  return 0; /* success */
 }
 

mysql-test/suite/test_services/r/test_mysql_command_services_component.result

@@ -114,4 +114,8 @@ SELECT test_mysql_command_services_error_code_udf("SELECT * FROM test.no_such_ta
 test_mysql_command_services_error_code_udf("SELECT * FROM test.no_such_table")
 1146
 SET @@GLOBAL.DEBUG = '-d, mysql_command_services_component_test_errno';
+# Test : failing connect() when running in an initialized thread; cleanup (cmd_factory->close() + cmd_thread->end()) must not leak the session
+SELECT test_mysql_command_services_explicit_connect_fail_cleanup_udf();
+test_mysql_command_services_explicit_connect_fail_cleanup_udf()
+1
 UNINSTALL COMPONENT "file://component_test_mysql_command_services";

mysql-test/suite/test_services/t/test_mysql_command_services_component.test

@@ -89,4 +89,9 @@ SET @@GLOBAL.DEBUG = '+d, mysql_command_services_component_test_errno';
 SELECT test_mysql_command_services_error_code_udf("SELECT * FROM test.no_such_table");
 SET @@GLOBAL.DEBUG = '-d, mysql_command_services_component_test_errno';
 
+--echo # Test : failing connect() when running in an initialized thread; cleanup (cmd_factory->close() + cmd_thread->end()) must not leak the session
+SELECT test_mysql_command_services_explicit_connect_fail_cleanup_udf();
+
+# Error-log must not contain: [ERROR] [MY-010470] [Server] Closed forcefully 1 session left opened by plugin server_service
+
 UNINSTALL COMPONENT "file://component_test_mysql_command_services";

sql-common/client.cc

@@ -139,6 +139,7 @@
 #include "sql/client_settings.h"
 #include "sql/server_component/mysql_command_services_imp.h"
 /* mysql_command_service_extn */
+#include "sql/mysqld.h" // srv_registry
 #else
 #include "libmysql/client_settings.h"
 #endif
@@ -3374,6 +3375,138 @@ void mysql_extension_bind_free(MYSQL_EXTENSION *ext) {
  memset(&ext->bind_info, 0, sizeof(ext->bind_info));
 }
 
+#ifdef MYSQL_SERVER
+/**
+ Release services.
+
+ @param[in] consumer_refs A valid mysql_command_consumer_refs object.
+ @param[in] mcs_ext A valid mysql_command_service_extn object.
+ @param[in] srv_registry Registry service pointer.
+*/
+static void release_services(mysql_command_consumer_refs *consumer_refs,
+ mysql_command_service_extn *mcs_ext,
+ mysql_service_registry_t *srv_registry) {
+ if (consumer_refs) {
+ if (consumer_refs->factory_srv) {
+ /* This service call is used to free the memory, the allocation
+ was happened through factory_srv->start() service api. */
+ consumer_refs->factory_srv->end(
+ reinterpret_cast<SRV_CTX_H>(mcs_ext->consumer_srv_data));
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_factory_v1) *>(
+ consumer_refs->factory_srv)));
+ }
+ if (consumer_refs->metadata_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_metadata_v1) *>(
+ consumer_refs->metadata_srv)));
+ if (consumer_refs->row_factory_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(
+ mysql_text_consumer_row_factory_v1) *>(
+ consumer_refs->row_factory_srv)));
+ if (consumer_refs->error_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_error_v1) *>(
+ consumer_refs->error_srv)));
+ if (consumer_refs->get_null_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(mysql_text_consumer_get_null_v1) *>(
+ consumer_refs->get_null_srv)));
+ if (consumer_refs->get_integer_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(
+ mysql_text_consumer_get_integer_v1) *>(
+ consumer_refs->get_integer_srv)));
+ if (consumer_refs->get_longlong_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(
+ mysql_text_consumer_get_longlong_v1) *>(
+ consumer_refs->get_longlong_srv)));
+ if (consumer_refs->get_decimal_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(
+ mysql_text_consumer_get_decimal_v1) *>(
+ consumer_refs->get_decimal_srv)));
+ if (consumer_refs->get_double_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(
+ mysql_text_consumer_get_double_v1) *>(
+ consumer_refs->get_double_srv)));
+ if (consumer_refs->get_date_time_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(
+ mysql_text_consumer_get_date_time_v1) *>(
+ consumer_refs->get_date_time_srv)));
+ if (consumer_refs->get_string_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(
+ mysql_text_consumer_get_string_v1) *>(
+ consumer_refs->get_string_srv)));
+ if (consumer_refs->client_capabilities_srv)
+ srv_registry->release(reinterpret_cast<my_h_service>(
+ const_cast<SERVICE_TYPE_NO_CONST(
+ mysql_text_consumer_client_capabilities_v1) *>(
+ consumer_refs->client_capabilities_srv)));
+ }
+}
+
+/**
+ Free the command service extension.
+
+ This function releases consumer services and closes/detaches the backend
+ session as appropriate, then frees 'ext->mcs_extn'.
+ Safe to call even if some fields are null/partially initialized. No-op if
+ 'ext' or 'ext->mcs_extn' is null.
+
+ @param[in,out] ext The MYSQL_EXTENSION owning the command service state.
+ On return, any consumer services are released, cleans up
+ the backend session (detached sessions are detached/
+ closed, THD-associated sessions are left to their owner),
+ and 'ext->mcs_extn' is freed and set to nullptr. The 'ext'
+ object itself is not freed.
+*/
+static void mysql_command_service_extn_free(MYSQL_EXTENSION *ext) {
+ if (!ext || !ext->mcs_extn) return;
+
+ auto *mcs_ext = reinterpret_cast<mysql_command_service_extn *>(ext->mcs_extn);
+
+ // Release consumer services acquired earlier
+ if (mcs_ext->command_consumer_services) {
+ auto *consumer_refs = reinterpret_cast<mysql_command_consumer_refs *>(
+ mcs_ext->command_consumer_services);
+ bool no_lock_registry = mcs_ext->no_lock_registry;
+
+ if (consumer_refs) {
+ no_lock_registry
+ ? release_services(consumer_refs, mcs_ext, srv_registry_no_lock)
+ : release_services(consumer_refs, mcs_ext, srv_registry);
+ delete consumer_refs;
+ consumer_refs = nullptr;
+
+ // avoid dangling ptr
+ mcs_ext->command_consumer_services = nullptr;
+ }
+ }
+
+ // Close session
+ if (mcs_ext->session_svc) {
+ if (!mcs_ext->is_thd_associated) {
+ // Detached session: detach then close
+ srv_session_detach(mcs_ext->session_svc);
+ srv_session_close(mcs_ext->session_svc);
+ }
+ // THD-associated session: do not detach/close here (owner will close it)
+ mcs_ext->session_svc = nullptr;
+ }
+
+ // Reset var, free ext storage, and set ptr to nullptr
+ mcs_ext->is_thd_associated = false;
+ my_free(mcs_ext);
+ ext->mcs_extn = nullptr;
+}
+#endif
+
 void mysql_extension_free(MYSQL_EXTENSION *ext) {
  if (!ext) return;
  if (ext->trace_data) my_free(ext->trace_data);
@@ -3400,7 +3533,7 @@ void mysql_extension_free(MYSQL_EXTENSION *ext) {
  ext->mysql_async_context = nullptr;
  }
 #ifdef MYSQL_SERVER
- if (ext->mcs_extn) my_free(ext->mcs_extn);
+ mysql_command_service_extn_free(ext);
 #endif
  // free state change related resources.
  free_state_change_info(ext);

sql/server_component/mysql_command_backend.cc

@@ -224,12 +224,12 @@ mysql_state_machine_status cssm_begin_connect(mysql_async_connect *ctx) {
  thd = mysql_session->get_thd();
  mcs_extn->is_thd_associated = false;
  Security_context_handle sc;
+ mcs_extn->session_svc = mysql_session;
  if (mysql_security_context_imp::get(thd, &sc)) return STATE_MACHINE_FAILED;
  if (mysql_security_context_imp::lookup(sc, user, host, nullptr, db))
  return STATE_MACHINE_FAILED;
  mcs_extn->mcs_thd = thd;
  mysql->thd = thd;
- mcs_extn->session_svc = mysql_session;
  } else {
  mysql->thd = reinterpret_cast<void *>(mcs_extn->mcs_thd);
  }