From 428bc5740ba8b8bff05a0104b3eac034cdb9b5f6 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Thu, 3 Oct 2019 14:47:03 +0300 Subject: [PATCH 1/2] MXS-2645 Decrement service client count also when authentication fails The client count was incremented before authentication was complete, and should be decremented if it fails. Otherwise service connection limit can be easily reached. --- .../protocol/MySQL/mariadbclient/mysql_client.cc | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc index b4ffc89d0..ced28263b 100644 --- a/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc +++ b/server/modules/protocol/MySQL/mariadbclient/mysql_client.cc @@ -740,12 +740,20 @@ static void check_packet(DCB* dcb, GWBUF* buf, int bytes) */ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_read) { + // If this function fails and dcb is closed, the service client count must be decremented manually. + // After authentication, the session closing code decrements the count. + auto decrement_and_close = [](DCB* dcb) + { + mxb::atomic::add(&dcb->service->client_count, -1); + dcb_close(dcb); + }; + MXB_AT_DEBUG(check_packet(dcb, read_buffer, nbytes_read)); /** Allocate the shared session structure */ if (dcb->data == NULL && (dcb->data = mysql_session_alloc()) == NULL) { - dcb_close(dcb); + decrement_and_close(dcb); return 1; } @@ -812,7 +820,7 @@ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_re MYSQL_session* ses = (MYSQL_session*)dcb->data; if ((dcb->user = MXS_STRDUP(ses->user)) == NULL) { - dcb_close(dcb); + decrement_and_close(dcb); gwbuf_free(read_buffer); return 0; } @@ -865,7 +873,7 @@ static int gw_read_do_authentication(DCB* dcb, GWBUF* read_buffer, int nbytes_re /** * Close DCB and which will release MYSQL_session */ - dcb_close(dcb); + decrement_and_close(dcb); } /* One way or another, the buffer is now fully processed */ gwbuf_free(read_buffer); From c54e254ea05e61485c3da2e41307bc22bc0c4428 Mon Sep 17 00:00:00 2001 From: Esa Korhonen Date: Thu, 3 Oct 2019 15:45:52 +0300 Subject: [PATCH 2/2] MXS-2645 Add test case to connection_limit test Tests max_connections with repeated failed logins. --- maxscale-system-test/connection_limit.cpp | 106 +++++++++++++++------- 1 file changed, 72 insertions(+), 34 deletions(-) diff --git a/maxscale-system-test/connection_limit.cpp b/maxscale-system-test/connection_limit.cpp index 690d219b5..6e6513c7e 100644 --- a/maxscale-system-test/connection_limit.cpp +++ b/maxscale-system-test/connection_limit.cpp @@ -6,38 +6,93 @@ * - create max num of connections and check tha N+1 connection fails */ - -#include -#include #include "testconnections.h" -using namespace std; +void check_with_wrong_pw(int router, int max_conn, TestConnections& test); +void check_max_conn(int router, int max_conn, TestConnections& test); -void check_max_conn(int router, int max_conn, TestConnections* Test) +int main(int argc, char* argv[]) +{ + TestConnections test(argc, argv); + + // First test with wrong pw to see that count is properly decremented. + test.tprintf("Trying 20 connections with RWSplit with wrong password\n"); + check_with_wrong_pw(0, 20, test); + + if (test.ok()) + { + test.tprintf("Trying 11 connections with RWSplit\n"); + check_max_conn(0, 10, test); + } + + if (test.ok()) + { + test.tprintf("Trying 21 connections with Readconn master\n"); + check_max_conn(1, 20, test); + } + + if (test.ok()) + { + test.tprintf("Trying 26 connections with Readconnn slave\n"); + check_max_conn(2, 25, test); + } + + test.check_maxscale_alive(0); + int rval = test.global_result; + return rval; +} + +void check_with_wrong_pw(int router, int max_conn, TestConnections& test) +{ + // Check MXS-2645, connection count is not decremented properly when authentication fails. + const char wrong_pw[] = "batman"; + bool limit_reached = false; + + for (int i = 0; i < max_conn && !limit_reached; i++) + { + MYSQL* failed_conn = open_conn( + test.maxscales->ports[0][router], test.maxscales->IP[0], + test.maxscales->user_name, wrong_pw, + test.ssl); + auto error = mysql_errno(failed_conn); + if (error == 0) + { + test.expect(false, "Connection succeeded when it should have failed."); + } + else if (error == 1040) + { + test.expect(false, "Connection limit wrongfully reached."); + limit_reached = true; + } + mysql_close(failed_conn); + } +} + +void check_max_conn(int router, int max_conn, TestConnections& test) { MYSQL* conn[max_conn + 1]; int i; for (i = 0; i < max_conn; i++) { - conn[i] = open_conn(Test->maxscales->ports[0][router], - Test->maxscales->IP[0], - Test->maxscales->user_name, - Test->maxscales->password, - Test->ssl); + conn[i] = open_conn(test.maxscales->ports[0][router], + test.maxscales->IP[0], + test.maxscales->user_name, + test.maxscales->password, + test.ssl); if (mysql_errno(conn[i]) != 0) { - Test->add_result(1, "Connection %d failed, error is %s\n", i, mysql_error(conn[i])); + test.add_result(1, "Connection %d failed, error is %s\n", i, mysql_error(conn[i])); } } - conn[max_conn] = open_conn(Test->maxscales->ports[0][router], - Test->maxscales->IP[0], - Test->maxscales->user_name, - Test->maxscales->password, - Test->ssl); + conn[max_conn] = open_conn(test.maxscales->ports[0][router], + test.maxscales->IP[0], + test.maxscales->user_name, + test.maxscales->password, + test.ssl); if (mysql_errno(conn[i]) != 1040) { - Test->add_result(1, + test.add_result(1, "Max_xonnections reached, but error is not 1040, it is %d %s\n", mysql_errno(conn[i]), mysql_error(conn[i])); @@ -47,20 +102,3 @@ void check_max_conn(int router, int max_conn, TestConnections* Test) mysql_close(conn[i]); } } - -int main(int argc, char* argv[]) -{ - TestConnections* Test = new TestConnections(argc, argv); - - Test->tprintf("Trying 11 connections with RWSplit\n"); - check_max_conn(0, 10, Test); - Test->tprintf("Trying 21 connections with Readconn master\n"); - check_max_conn(1, 20, Test); - Test->tprintf("Trying 26 connections with Readconnn slave\n"); - check_max_conn(2, 25, Test); - - Test->check_maxscale_alive(0); - int rval = Test->global_result; - delete Test; - return rval; -}