From aca8596efa3b9ee81948edbe78c9fb3adc8d26b8 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Mon, 6 Oct 2014 11:46:12 +0300 Subject: [PATCH] mysql_client.c:gw_client_close didn't close client session in cases where session->state == SESSION_STATE_STOPPING. That is a bug and lead to situation where session wasn't closed at all. Also changed 'authorization failed' to 'access denied' mysql_common.c: fixed memory leak in gw_receive_backend_auth, and replaced error code '2800' with '28000'. readconnroute.c:handleError didn't set *succp pointer so uninitialized value was used in caller's context. makefile.inc: added -lm to linker flags mysql_backend.c: added a few comments --- makefile.inc | 2 +- server/modules/protocol/mysql_backend.c | 36 +++++++++++-------- server/modules/protocol/mysql_client.c | 27 ++++++-------- server/modules/protocol/mysql_common.c | 3 +- server/modules/routing/readconnroute.c | 3 +- .../routing/readwritesplit/readwritesplit.c | 16 ++++----- 6 files changed, 43 insertions(+), 44 deletions(-) diff --git a/makefile.inc b/makefile.inc index 279cea6a4..0d63b5192 100644 --- a/makefile.inc +++ b/makefile.inc @@ -25,7 +25,7 @@ endif # -O2 -g -pipe -Wformat -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fstack-protector --param=ssp-buffer-size=4 -fPIC CFLAGS := $(CFLAGS) -Wall -LDLIBS := $(LDLIBS) -pthread +LDLIBS := $(LDLIBS) -pthread -lm LDMYSQL := -lmysqld CPP_LDLIBS := -lstdc++ diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index da1f73f4f..f3ef18e02 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -199,6 +199,7 @@ static int gw_read_backend_event(DCB *dcb) { if (backend_protocol->protocol_auth_state == MYSQL_CONNECTED) { + /** Read cached backend handshake */ if (gw_read_backend_handshake(backend_protocol) != 0) { backend_protocol->protocol_auth_state = MYSQL_AUTH_FAILED; @@ -209,11 +210,13 @@ static int gw_read_backend_event(DCB *dcb) { "state = MYSQL_AUTH_FAILED.", pthread_self(), backend_protocol->owner_dcb->fd))); - } - else + else { - /* handshake decoded, send the auth credentials */ + /** + * Decode password and send the auth credentials + * to backend. + */ if (gw_send_authentication_to_backend( current_session->db, current_session->user, @@ -227,16 +230,16 @@ static int gw_read_backend_event(DCB *dcb) { "gw_send_authentication_to_backend " "fd %d, state = MYSQL_AUTH_FAILED.", pthread_self(), - backend_protocol->owner_dcb->fd))); - } - else + backend_protocol->owner_dcb->fd))); + } + else { backend_protocol->protocol_auth_state = MYSQL_AUTH_RECV; } } } spinlock_release(&dcb->authlock); - } + } /*< backend_protocol->protocol_auth_state == MYSQL_CONNECTED */ /* * Now: * -- check the authentication reply from backend @@ -266,9 +269,10 @@ static int gw_read_backend_event(DCB *dcb) { router_instance = session->service->router_instance; rsession = session->router_session; - if (backend_protocol->protocol_auth_state == MYSQL_AUTH_RECV) { - /*< - * Read backed auth reply + if (backend_protocol->protocol_auth_state == MYSQL_AUTH_RECV) + { + /** + * Read backed's reply to authentication message */ receive_rc = gw_receive_backend_auth(backend_protocol); @@ -283,7 +287,6 @@ static int gw_read_backend_event(DCB *dcb) { "fd %d, state = MYSQL_AUTH_FAILED.", pthread_self(), backend_protocol->owner_dcb->fd))); - LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, @@ -362,8 +365,8 @@ static int gw_read_backend_event(DCB *dcb) { 0, "Authentication with backend failed. " "Session will be closed."); - - router->handleError(router_instance, + + router->handleError(router_instance, rsession, errbuf, dcb, @@ -371,7 +374,6 @@ static int gw_read_backend_event(DCB *dcb) { &succp); ss_dassert(!succp); - LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, "%lu [gw_read_backend_event] " @@ -412,7 +414,7 @@ static int gw_read_backend_event(DCB *dcb) { } } } /* MYSQL_AUTH_RECV || MYSQL_AUTH_FAILED */ - + spinlock_release(&dcb->authlock); } /* MYSQL_AUTH_RECV || MYSQL_AUTH_FAILED */ @@ -1048,6 +1050,10 @@ gw_backend_close(DCB *dcb) mysql_protocol_done(dcb); + /** + * If session->state is set to STOPPING the client and the session must + * be closed too. + */ if (session != NULL && session->state == SESSION_STATE_STOPPING) { client_dcb = session->client; diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index b1559b94b..9372314d8 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -659,7 +659,7 @@ int gw_read_client_event( dcb, 2, 0, - "Authorization failed"); + "Access denied"); dcb_close(dcb); } @@ -801,9 +801,12 @@ int gw_read_client_event( } /** succeed */ - if (rc) { + if (rc) + { rc = 0; /**< here '0' means success */ - } else { + } + else + { GWBUF* errbuf; bool succp; @@ -1360,20 +1363,12 @@ gw_client_close(DCB *dcb) CHK_SESSION(session); spinlock_acquire(&session->ses_lock); - if (session->state == SESSION_STATE_STOPPING) + if (session->state != SESSION_STATE_STOPPING) { - /** - * Session is already getting closed so avoid - * redundant calls - */ - spinlock_release(&session->ses_lock); - return 1; - } - else - { - session->state = SESSION_STATE_STOPPING; - spinlock_release(&session->ses_lock); - } + session->state = SESSION_STATE_STOPPING; + } + spinlock_release(&session->ses_lock); + router = session->service->router; router_instance = session->service->router_instance; rsession = session->router_session; diff --git a/server/modules/protocol/mysql_common.c b/server/modules/protocol/mysql_common.c index 17a04a77b..892824c19 100644 --- a/server/modules/protocol/mysql_common.c +++ b/server/modules/protocol/mysql_common.c @@ -463,6 +463,7 @@ int gw_receive_backend_auth( bufstr))); free(bufstr); + free(err); rc = -1; } else @@ -1458,7 +1459,7 @@ mysql_send_auth_error ( } mysql_errno = 1045; mysql_error_msg = "Access denied!"; - mysql_state = "2800"; + mysql_state = "28000"; field_count = 0xff; gw_mysql_set_byte2(mysql_err, mysql_errno); diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index c00a5a0a4..9ccbb536a 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -825,7 +825,8 @@ handleError( DCB *client = NULL; SESSION *session = backend_dcb->session; client = session->client; - + /** false because connection is not available anymore */ + *succp = false; ss_dassert(client != NULL); } diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index a8ae23940..8d5f51727 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -266,7 +266,7 @@ static bool handle_error_new_connection( ROUTER_CLIENT_SES* rses, DCB* backend_dcb, GWBUF* errmsg); -static bool handle_error_reply_client(SESSION* ses, GWBUF* errmsg); +static void handle_error_reply_client(SESSION* ses, GWBUF* errmsg); static backend_ref_t* get_root_master_bref(ROUTER_CLIENT_SES* rses); @@ -1669,7 +1669,7 @@ static int routeQuery( route_target_t route_target; bool succp = false; int rlag_max = MAX_RLAG_UNDEFINED; - backend_type_t btype; /*< target backend type */ + backend_type_t btype; /*< target backend type */ CHK_CLIENT_RSES(router_cli_ses); @@ -1683,7 +1683,6 @@ static int routeQuery( packet = GWBUF_DATA(querybuf); packet_type = packet[4]; - if (rses_is_closed) { /** @@ -4015,7 +4014,8 @@ static void handleError ( case ERRACT_REPLY_CLIENT: { - *succp = handle_error_reply_client(session, errmsgbuf); + handle_error_reply_client(session, errmsgbuf); + *succp = false; /*< no new backend servers were made available */ break; } @@ -4026,13 +4026,12 @@ static void handleError ( } -static bool handle_error_reply_client( +static void handle_error_reply_client( SESSION* ses, GWBUF* errmsg) { session_state_t sesstate; DCB* client_dcb; - bool succp; spinlock_acquire(&ses->ses_lock); sesstate = ses->state; @@ -4048,10 +4047,7 @@ static bool handle_error_reply_client( { while ((errmsg=gwbuf_consume(errmsg, GWBUF_LENGTH(errmsg))) != NULL) ; - } - succp = false; /** false because new servers aren's selected. */ - - return succp; + } } /**