fix(mysqlx): harden check_connect() poll and getsockopt handling

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.
fix/mysqlx-check-connect-poll
Rene Cannao 1 month ago
parent 923cbfeadc
commit 76aafdac67

@ -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;
}

@ -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 $@

@ -15,6 +15,7 @@
#include <cerrno>
#include <cstdio>
#include <cstring>
#include <fcntl.h>
#include <poll.h>
#include <sys/socket.h>
#include <netinet/in.h>
@ -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();

Loading…
Cancel
Save