From 76aafdac6734c35f52c8fc59d397d7ea0e73ad58 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Sat, 18 Apr 2026 08:56:59 +0000 Subject: [PATCH] fix(mysqlx): harden check_connect() poll and getsockopt handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the coderabbitai outside-diff finding originally surfaced on PR #5641: the previous check_connect() conflated poll() errors with timeouts, never retried on EINTR, did not check getsockopt()'s return, and accepted POLLNVAL / POLLERR / POLLHUP as though they were normal "not ready" states. The practical failure mode was slow and misleading: a fatal poll error or a bad descriptor was masked as "not ready yet" until the outer connect_timeout_ms_ (default 10s) elapsed, at which point the session reported a generic "connect failed" reason rather than the actual cause. A concurrent signal (EINTR) would extend that masked window further. Meanwhile, a failing getsockopt() with an uninspected return would leave `err` at its initializer value of 0 and optimistically transition the state machine to AUTHENTICATING, so the very next read or write on the invalid fd would fail in a less diagnostic path. What changed: - poll() is now retried on EINTR. - poll() returning -1 sets ERROR_STATE and returns -1 immediately. - POLLNVAL / POLLERR / POLLHUP in revents are treated as hard errors even if POLLOUT is also set, since the underlying socket is already in a terminal async condition. - getsockopt() return is checked; a nonzero return is now a hard error rather than a silent state advance. Testing: three new assertions in mysqlx_robustness_unit-t.cpp exercise the bad-fd error path, the happy-path connect, and the not-ready timeout path. Plan count 33 → 42. Out of scope: other poll() sites in the mysqlx plugin (the unit test helpers at the top of mysqlx_robustness_unit-t.cpp have their own poll() loops but they aren't on the backend-connect critical path); the connect_timeout_ms_ check itself, which is unchanged. Pre-existing ripple: same Makefile link-gap fix as earlier PRs — adds mysqlx_config_store.cpp to the mysqlx_robustness_unit-t link line so MysqlxConfigStore::resolve_identity() resolves when the binary links. --- plugins/mysqlx/src/mysqlx_connection.cpp | 20 ++++- test/tap/tests/unit/Makefile | 4 +- .../tests/unit/mysqlx_robustness_unit-t.cpp | 75 ++++++++++++++++++- 3 files changed, 93 insertions(+), 6 deletions(-) diff --git a/plugins/mysqlx/src/mysqlx_connection.cpp b/plugins/mysqlx/src/mysqlx_connection.cpp index e5cd69d3f..24f93edf5 100644 --- a/plugins/mysqlx/src/mysqlx_connection.cpp +++ b/plugins/mysqlx/src/mysqlx_connection.cpp @@ -72,11 +72,25 @@ int MysqlxConnection::check_connect() { pfd.fd = fd_; pfd.events = POLLOUT; pfd.revents = 0; - int pr = poll(&pfd, 1, 0); - if (pr <= 0) return 1; // not ready yet + int pr; + do { + pr = poll(&pfd, 1, 0); + } while (pr < 0 && errno == EINTR); + if (pr < 0) { + state_ = ERROR_STATE; + return -1; + } + if (pr == 0) return 1; // not ready yet + if (pfd.revents & (POLLNVAL | POLLERR | POLLHUP)) { + state_ = ERROR_STATE; + return -1; + } int err = 0; socklen_t len = sizeof(err); - getsockopt(fd_, SOL_SOCKET, SO_ERROR, &err, &len); + if (getsockopt(fd_, SOL_SOCKET, SO_ERROR, &err, &len) != 0) { + state_ = ERROR_STATE; + return -1; + } if (err == 0) { state_ = AUTHENTICATING; return 0; } state_ = ERROR_STATE; return -1; } diff --git a/test/tap/tests/unit/Makefile b/test/tap/tests/unit/Makefile index 2b0dab331..51c96e3b2 100644 --- a/test/tap/tests/unit/Makefile +++ b/test/tap/tests/unit/Makefile @@ -524,8 +524,8 @@ mysqlx_credential_verify_unit-t: mysqlx_credential_verify_unit-t.cpp $(PROXYSQL_ $(IDIRS) $(LDIRS) $(OPT) $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) \ $(MYLIBS) -lprotobuf -lssl -lcrypto $(ALLOW_MULTI_DEF) -o $@ -mysqlx_robustness_unit-t: mysqlx_robustness_unit-t.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_session.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_data_stream.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_connection.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_thread.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_protocol.cpp $(MYSQLX_PROTO_OBJS) $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) - $(CXX) $< $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_session.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_data_stream.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_connection.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_thread.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_protocol.cpp $(MYSQLX_PROTO_OBJS) $(TEST_HELPERS_OBJ) \ +mysqlx_robustness_unit-t: mysqlx_robustness_unit-t.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_session.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_data_stream.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_connection.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_thread.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_protocol.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_config_store.cpp $(MYSQLX_PROTO_OBJS) $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR) + $(CXX) $< $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_session.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_data_stream.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_connection.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_thread.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_protocol.cpp $(PROXYSQL_PATH)/plugins/mysqlx/src/mysqlx_config_store.cpp $(MYSQLX_PROTO_OBJS) $(TEST_HELPERS_OBJ) \ -I$(PROXYSQL_PATH)/plugins/mysqlx/include -I$(MYSQLX_PROTO_DIR) \ $(IDIRS) $(LDIRS) $(OPT) $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) \ $(MYLIBS) -lprotobuf -lssl -lcrypto $(ALLOW_MULTI_DEF) -lpthread -o $@ diff --git a/test/tap/tests/unit/mysqlx_robustness_unit-t.cpp b/test/tap/tests/unit/mysqlx_robustness_unit-t.cpp index ddee5c21e..d39f81fb9 100644 --- a/test/tap/tests/unit/mysqlx_robustness_unit-t.cpp +++ b/test/tap/tests/unit/mysqlx_robustness_unit-t.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -525,6 +526,75 @@ static void test_client_disconnect_detected() { close(fds[0]); } +static void test_check_connect_bad_fd() { + // Bug fix (PR #5641 review): check_connect() must report a hard error + // when poll() sees POLLNVAL on a closed/invalid fd, instead of waiting + // out the connect timeout. Create an fd, close it so it's invalid, then + // hand it to the connection and call check_connect(). + int s = socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0); + ok(s >= 0, "socket() created for bad-fd test"); + close(s); + + MysqlxConnection conn; + conn.set_fd(s); + conn.set_state(MysqlxConnection::CONNECTING); + conn.set_connect_timeout(10000); + + int rc = conn.check_connect(); + ok(rc == -1, "check_connect() returns -1 on invalid fd"); + ok(conn.get_state() == MysqlxConnection::ERROR_STATE, + "check_connect() transitions to ERROR_STATE on invalid fd"); + + conn.set_fd(-1); // prevent dtor from double-closing +} + +static void test_check_connect_success_path() { + // Positive path: an already-connected socketpair endpoint reports POLLOUT + // ready immediately, SO_ERROR is 0, so check_connect() must transition to + // AUTHENTICATING and return 0. + int fds[2]; + int sp = socketpair(AF_UNIX, SOCK_STREAM, 0, fds); + ok(sp == 0, "socketpair() created for success-path test"); + + // Make non-blocking to mimic start_connect() fd flags. + int flags = fcntl(fds[0], F_GETFL, 0); + fcntl(fds[0], F_SETFL, flags | O_NONBLOCK); + + MysqlxConnection conn; + conn.set_fd(fds[0]); + conn.set_state(MysqlxConnection::CONNECTING); + conn.set_connect_timeout(10000); + + int rc = conn.check_connect(); + ok(rc == 0, "check_connect() returns 0 when socket is writable and SO_ERROR==0"); + ok(conn.get_state() == MysqlxConnection::AUTHENTICATING, + "check_connect() transitions to AUTHENTICATING on successful connect"); + + conn.set_fd(-1); // detach before manual close + close(fds[0]); + close(fds[1]); +} + +static void test_check_connect_not_ready() { + // Socket that has NOT connected yet: poll() returns 0 (no POLLOUT), + // so check_connect() must return 1 and leave state unchanged. + int s = socket(AF_INET, SOCK_STREAM | SOCK_NONBLOCK, 0); + ok(s >= 0, "socket() created for not-ready test"); + + MysqlxConnection conn; + conn.set_fd(s); + conn.set_state(MysqlxConnection::CONNECTING); + conn.set_connect_timeout(10000); + + int rc = conn.check_connect(); + ok(rc == 1, "check_connect() returns 1 (not ready) on unconnected socket"); + ok(conn.get_state() == MysqlxConnection::CONNECTING, + "check_connect() leaves state as CONNECTING when not ready"); + + conn.set_fd(-1); + close(s); +} + static void test_forward_empty_frame() { int client_fds[2], backend_fds[2]; socketpair(AF_UNIX, SOCK_STREAM, 0, client_fds); @@ -567,7 +637,7 @@ static void test_forward_empty_frame() { } int main() { - plan(33); + plan(42); test_server_response_terminal_frame(); test_server_response_non_terminal_keeps_waiting(); @@ -581,6 +651,9 @@ int main() { test_return_backend_no_thread(); test_connection_limit_config(); test_client_disconnect_detected(); + test_check_connect_bad_fd(); + test_check_connect_success_path(); + test_check_connect_not_ready(); test_forward_empty_frame(); return exit_status();