From 635fcf708f5f6cea585276d076dd51bc1af0706c Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Mon, 29 Dec 2014 20:19:01 +0200 Subject: [PATCH] Fix to bugs #665, and #664. Potentially to #649. http://bugs.skysql.com/show_bug.cgi?id=665 http://bugs.skysql.com/show_bug.cgi?id=664 http://bugs.skysql.com/show_bug.cgi?id=649 dcb.c:dcb_final_free: (665):set dcb->session->client pointer to NULL so that it won't be read anymore and other threads won't try to close it. dcb_final_free:(664):don't free dcb->data, it is either freed in session_alloc if session creation fails or in session_free only. session.c:if session creation fails, free dcb->data and remove links between client DCB and session. mysql_backend.c:(665):gw_backend_close:check that session->client isn't NULL and that client DCB's state is still polling before calling dcb_close for it. mysql_client.c:gw_mysql_do_authentication:if anything fails, and session_alloc won't be called, free dcb->data. mysql_common.c:gw_send_authentication_to_backend:if session is already closing then return with error. --- server/core/dcb.c | 19 ++++++++--- server/core/session.c | 19 ++++++++++- .../include/mysql_client_server_protocol.h | 6 ++++ server/modules/protocol/mysql_backend.c | 34 +++++++++---------- server/modules/protocol/mysql_client.c | 28 ++++++++++++--- server/modules/protocol/mysql_common.c | 10 ++++++ server/modules/routing/readconnroute.c | 2 +- utils/skygw_debug.h | 8 ++++- 8 files changed, 97 insertions(+), 29 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 63af8c4ca..0180ea08d 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -395,15 +395,24 @@ DCB_CALLBACK *cb; if (local_session->client == dcb) { local_session->client = NULL; } + /** + * Set session's client pointer NULL so that other threads + * won't try to call dcb_close for client DCB + * after this call. + */ + if (dcb->session->client == dcb) + { + spinlock_acquire(&dcb->session->ses_lock); + dcb->session->client = NULL; + spinlock_release(&dcb->session->ses_lock); + } dcb->session = NULL; session_free(local_session); } } if (dcb->protocol && (!DCB_IS_CLONE(dcb))) - free(dcb->protocol); - if (dcb->data && (!DCB_IS_CLONE(dcb))) - free(dcb->data); + free(dcb->protocol); if (dcb->remote) free(dcb->remote); if (dcb->user) @@ -428,7 +437,6 @@ DCB_CALLBACK *cb; } spinlock_release(&dcb->cb_lock); - bitmask_free(&dcb->memdata.bitmask); free(dcb); } @@ -903,7 +911,8 @@ int below_water; dcb->state != DCB_STATE_POLLING && dcb->state != DCB_STATE_LISTENING && dcb->state != DCB_STATE_NOPOLLING && - dcb->session->state != SESSION_STATE_STOPPING)) + (dcb->session == NULL || + dcb->session->state != SESSION_STATE_STOPPING))) { LOGIF(LD, (skygw_log_write( LOGFILE_DEBUG, diff --git a/server/core/session.c b/server/core/session.c index 07291073f..279d189e5 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -85,6 +85,11 @@ session_alloc(SERVICE *service, DCB *client_dcb) "session object due error %d, %s.", errno, strerror(errno)))); + if (client_dcb->data) + { + free(client_dcb->data); + client_dcb->data = NULL; + } goto return_session; } #if defined(SS_DEBUG) @@ -149,6 +154,7 @@ session_alloc(SERVICE *service, DCB *client_dcb) * Decrease refcount, set dcb's session pointer NULL * and set session pointer to NULL. */ + session->client = NULL; session_free(session); client_dcb->session = NULL; session = NULL; @@ -189,6 +195,7 @@ session_alloc(SERVICE *service, DCB *client_dcb) * Decrease refcount, set dcb's session pointer NULL * and set session pointer to NULL. */ + session->client = NULL; session_free(session); client_dcb->session = NULL; session = NULL; @@ -207,6 +214,7 @@ session_alloc(SERVICE *service, DCB *client_dcb) if (session->state != SESSION_STATE_READY) { spinlock_release(&session->ses_lock); + session->client = NULL; session_free(session); client_dcb->session = NULL; session = NULL; @@ -336,7 +344,11 @@ int session_unlink_dcb( if (dcb != NULL) { - dcb->session = NULL; + if (session->client == dcb) + { + session->client = NULL; + } + dcb->session = NULL; } spinlock_release(&session->ses_lock); @@ -429,6 +441,11 @@ bool session_free( if (!session->ses_is_child) { session->state = SESSION_STATE_FREE; + + if (session->data) + { + free(session->data); + } free(session); } succp = true; diff --git a/server/modules/include/mysql_client_server_protocol.h b/server/modules/include/mysql_client_server_protocol.h index a672de35b..bef74f45c 100644 --- a/server/modules/include/mysql_client_server_protocol.h +++ b/server/modules/include/mysql_client_server_protocol.h @@ -111,9 +111,15 @@ typedef enum { * */ typedef struct mysql_session { +#if defined(SS_DEBUG) + skygw_chk_t myses_chk_top; +#endif uint8_t client_sha1[MYSQL_SCRAMBLE_LEN]; /*< SHA1(passowrd) */ char user[MYSQL_USER_MAXLEN+1]; /*< username */ char db[MYSQL_DATABASE_MAXLEN+1]; /*< database */ +#if defined(SS_DEBUG) + skygw_chk_t myses_chk_tail; +#endif } MYSQL_session; diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index ea0641b10..f74a2eebe 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -699,16 +699,6 @@ gw_MySQLWrite_backend(DCB *dcb, GWBUF *queue) switch (backend_protocol->protocol_auth_state) { case MYSQL_HANDSHAKE_FAILED: case MYSQL_AUTH_FAILED: - { - size_t len; - char* str; - uint8_t* packet = (uint8_t *)queue->start; - uint8_t* startpoint; - - len = (size_t)MYSQL_GET_PACKET_LEN(packet); - startpoint = &packet[5]; - str = (char *)malloc(len+1); - snprintf(str, len+1, "%s", startpoint); LOGIF(LE, (skygw_log_write_flush( LOGFILE_ERROR, "Error : Unable to write to backend due to " @@ -717,13 +707,11 @@ gw_MySQLWrite_backend(DCB *dcb, GWBUF *queue) while ((queue = gwbuf_consume( queue, GWBUF_LENGTH(queue))) != NULL); - free(str); rc = 0; spinlock_release(&dcb->authlock); goto return_rc; break; - } - + case MYSQL_IDLE: { uint8_t* ptr = GWBUF_DATA(queue); @@ -1166,21 +1154,33 @@ gw_backend_close(DCB *dcb) mysql_protocol_done(dcb); + spinlock_acquire(&session->ses_lock); /** * If session->state is STOPPING, start closing client session. * Otherwise only this backend connection is closed. */ - if (session != NULL && session->state == SESSION_STATE_STOPPING) + if (session != NULL && + session->state == SESSION_STATE_STOPPING && + session->client != NULL) { client_dcb = session->client; - - if (client_dcb != NULL && - client_dcb->state == DCB_STATE_POLLING) + + if (client_dcb->state == DCB_STATE_POLLING) { + spinlock_release(&session->ses_lock); + /** Close client DCB */ dcb_close(client_dcb); } + else + { + spinlock_release(&session->ses_lock); + } } + else + { + spinlock_release(&session->ses_lock); + } return 1; } diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index c265d2d2a..3a050e715 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -377,15 +377,18 @@ MySQLSendHandshake(DCB* dcb) * * Performs the MySQL protocol 4.1 authentication, using data in GWBUF *queue * - * The useful data: user, db, client_sha1 are copied into the MYSQL_session * dcb->session->data + * (MYSQL_session*)client_data including: user, db, client_sha1 are copied into + * the dcb->data and later to dcb->session->data. + * * client_capabilitiesa are copied into the dcb->protocol * * @param dcb Descriptor Control Block of the client * @param queue The GWBUF with data from client * @return 0 If succeed, otherwise non-zero value * + * @note in case of failure, dcb->data is freed before returning. If succeed, + * dcb->data is freed in session.c:session_free. */ - static int gw_mysql_do_authentication(DCB *dcb, GWBUF *queue) { MySQLProtocol *protocol = NULL; /* int compress = -1; */ @@ -405,6 +408,13 @@ static int gw_mysql_do_authentication(DCB *dcb, GWBUF *queue) { protocol = DCB_PROTOCOL(dcb, MySQLProtocol); CHK_PROTOCOL(protocol); client_data = (MYSQL_session *)calloc(1, sizeof(MYSQL_session)); +#if defined(SS_DEBUG) + client_data->myses_chk_top = CHK_NUM_MYSQLSES; + client_data->myses_chk_tail = CHK_NUM_MYSQLSES; +#endif + /** + * Assign authentication structure with client DCB. + */ dcb->data = client_data; stage1_hash = client_data->client_sha1; @@ -425,7 +435,10 @@ static int gw_mysql_do_authentication(DCB *dcb, GWBUF *queue) { */ /* Detect now if there are enough bytes to continue */ - if (client_auth_packet_size < (4 + 4 + 4 + 1 + 23)) { + if (client_auth_packet_size < (4 + 4 + 4 + 1 + 23)) + { + free(dcb->data); + dcb->data = NULL; return 1; } @@ -444,6 +457,8 @@ static int gw_mysql_do_authentication(DCB *dcb, GWBUF *queue) { if (username == NULL) { + free(dcb->data); + dcb->data = NULL; return 1; } @@ -512,6 +527,11 @@ static int gw_mysql_do_authentication(DCB *dcb, GWBUF *queue) { if (auth_ret == 0) { dcb->user = strdup(client_data->user); } + else + { + free(dcb->data); + dcb->data = NULL; + } /* let's free the auth_token now */ if (auth_token) { @@ -618,7 +638,7 @@ int gw_read_client_event( * Now there should be at least one complete mysql packet in read_buffer. */ switch (protocol->protocol_auth_state) { - + case MYSQL_AUTH_SENT: { int auth_val; diff --git a/server/modules/protocol/mysql_common.c b/server/modules/protocol/mysql_common.c index 8e57d8b21..23cb59a2f 100644 --- a/server/modules/protocol/mysql_common.c +++ b/server/modules/protocol/mysql_common.c @@ -541,6 +541,16 @@ int gw_send_authentication_to_backend( uint8_t *curr_passwd = NULL; unsigned int charset; + /** + * If session is stopping return with error. + */ + if (conn->owner_dcb->session == NULL || + (conn->owner_dcb->session->state != SESSION_STATE_READY && + conn->owner_dcb->session->state != SESSION_STATE_ROUTER_READY)) + { + return 1; + } + if (strlen(dbname)) curr_db = dbname; diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index ba4804ae1..963fcfeda 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -804,7 +804,7 @@ clientReply( GWBUF *queue, DCB *backend_dcb) { - DCB *client = NULL; + DCB *client ; client = backend_dcb->session->client; diff --git a/utils/skygw_debug.h b/utils/skygw_debug.h index 20bdac767..da92b84ac 100644 --- a/utils/skygw_debug.h +++ b/utils/skygw_debug.h @@ -124,7 +124,8 @@ typedef enum skygw_chk_t { CHK_NUM_BACKEND, CHK_NUM_BACKEND_REF, CHK_NUM_PREP_STMT, - CHK_NUM_PINFO + CHK_NUM_PINFO, + CHK_NUM_MYSQLSES } skygw_chk_t; # define STRBOOL(b) ((b) ? "true" : "false") @@ -542,6 +543,11 @@ typedef enum skygw_chk_t { "Parsing info struct has invalid check fields"); \ } +#define CHK_MYSQL_SESSION(s) { \ + ss_info_dassert((s)->myses_chk_top == CHK_NUM_MYSQLSES && \ + (s)->myses_chk_tail == CHK_NUM_MYSQLSES, \ + "MYSQL session struct has invalid check fields"); \ +} #if defined(FAKE_CODE)