From 5122777829c33b3334fd41d63fe505124e7ca03e Mon Sep 17 00:00:00 2001 From: counterpoint Date: Wed, 24 Feb 2016 10:00:45 +0000 Subject: [PATCH] Try to fix problem if balancing free client DCB and free session so that auth data is always available and client DCB is not freed until session is ready to be freed. Also fix problem in auth logic. --- server/core/dcb.c | 43 ++++++++++------------- server/core/session.c | 45 +++++++++++++++---------- server/include/dcb.h | 1 + server/modules/protocol/mysql_backend.c | 2 +- server/modules/protocol/mysql_client.c | 4 +-- 5 files changed, 50 insertions(+), 45 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 05ab7de6a..e6006da36 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -126,8 +126,6 @@ static int gw_write(DCB *dcb, bool *stop_writing); static int gw_write_SSL(DCB *dcb, bool *stop_writing); static void dcb_log_errors_SSL (DCB *dcb, const char *called_by, int ret); -static void mysql_auth_free_client_data(DCB *dcb); - size_t dcb_get_session_id( DCB *dcb) { @@ -320,8 +318,6 @@ dcb_clone(DCB *orig) static void dcb_final_free(DCB *dcb) { - DCB_CALLBACK *cb; - CHK_DCB(dcb); ss_info_dassert(dcb->state == DCB_STATE_DISCONNECTED || dcb->state == DCB_STATE_ALLOC, @@ -384,10 +380,24 @@ dcb_final_free(DCB *dcb) if (SESSION_STATE_DUMMY != local_session->state) { session_free(local_session); + return; } } + dcb_free_all_memory(dcb); +} - mysql_auth_free_client_data(dcb); +/** + * Free the memory belonging to a DCB + * + * NB The DCB is fully detached from all links except perhaps the session + * dcb_client link. + * + * @param dcb The DCB to free + */ +void +dcb_free_all_memory(DCB *dcb) +{ + DCB_CALLBACK *cb_dcb; if (dcb->protocol && (!DCB_IS_CLONE(dcb))) { @@ -424,10 +434,10 @@ dcb_final_free(DCB *dcb) } spinlock_acquire(&dcb->cb_lock); - while ((cb = dcb->callbacks) != NULL) + while ((cb_dcb = dcb->callbacks) != NULL) { - dcb->callbacks = cb->next; - free(cb); + dcb->callbacks = cb_dcb->next; + free(cb_dcb); } spinlock_release(&dcb->cb_lock); if (dcb->ssl) @@ -3025,20 +3035,3 @@ dcb_role_name(DCB *dcb) return name; } -/** - * @brief Free the client data pointed to by the passed DCB. - * - * Currently all that is required is to free the storage pointed to by - * dcb->data. But this is intended to be implemented as part of the - * authentication API at which time this code will be moved into the - * MySQL authenticator. If the data structure were to become more complex - * the mechanism would still work and be the responsibility of the authenticator. - * The DCB should not know authenticator implementation details. - * - * @param dcb Request handler DCB connected to the client - */ -static void -mysql_auth_free_client_data(DCB *dcb) -{ - free(dcb->data); -} \ No newline at end of file diff --git a/server/core/session.c b/server/core/session.c index 69ba9c9c0..e1f927b04 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -65,6 +65,8 @@ static SPINLOCK timeout_lock = SPINLOCK_INIT; static int session_setup_filters(SESSION *session); static void session_simple_free(SESSION *session, DCB *dcb); +static void mysql_auth_free_client_data(DCB *dcb); + /** * Allocate a new session for a new client of the specified service. * @@ -91,23 +93,6 @@ session_alloc(SERVICE *service, DCB *client_dcb) "session object due error %d, %s.", errno, strerror_r(errno, errbuf, sizeof(errbuf))); - /* Does this possibly need a lock? */ - /* - * This is really not the right way to do this. The data in a DCB is - * router specific and should be freed by a function in the relevant - * router. This would be better achieved by placing a function reference - * in the DCB and having dcb_final_free call it to dispose of the data - * at the final destruction of the DCB. However, this piece of code is - * only run following a calloc failure, so the system is probably on - * the point of crashing anyway. - * - */ - if (client_dcb->data && !DCB_IS_CLONE(client_dcb)) - { - void * clientdata = client_dcb->data; - client_dcb->data = NULL; - free(clientdata); - } return NULL; } #if defined(SS_DEBUG) @@ -437,6 +422,14 @@ session_free(SESSION *session) spinlock_release(&session_spin); atomic_add(&session->service->stats.n_current, -1); + /*** + * + */ + if (session->client_dcb) + { + mysql_auth_free_client_data(session->client_dcb); + dcb_free_all_memory(session->client_dcb); + } /** * If session is not child of some other session, free router_session. * Otherwise let the parent free it. @@ -1108,3 +1101,21 @@ sessionGetList(SESSIONLISTFILTER filter) return set; } + +/** + * @brief Free the client data pointed to by the passed DCB. + * + * Currently all that is required is to free the storage pointed to by + * dcb->data. But this is intended to be implemented as part of the + * authentication API at which time this code will be moved into the + * MySQL authenticator. If the data structure were to become more complex + * the mechanism would still work and be the responsibility of the authenticator. + * The DCB should not know authenticator implementation details. + * + * @param dcb Request handler DCB connected to the client + */ +static void +mysql_auth_free_client_data(DCB *dcb) +{ + free(dcb->data); +} \ No newline at end of file diff --git a/server/include/dcb.h b/server/include/dcb.h index d069d277b..6a636b3df 100644 --- a/server/include/dcb.h +++ b/server/include/dcb.h @@ -294,6 +294,7 @@ DCB *dcb_get_zombies(void); int dcb_write(DCB *, GWBUF *); DCB *dcb_alloc(dcb_role_t); void dcb_free(DCB *); +void dcb_free_all_memory(DCB *dcb); DCB *dcb_connect(struct server *, struct session *, const char *); DCB *dcb_clone(DCB *); int dcb_read(DCB *, GWBUF **, int); diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 7a7b86bb4..8f4e9bf97 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -172,7 +172,7 @@ static int gw_read_backend_event(DCB *dcb) goto return_rc; } - if (dcb->session == NULL) + if (dcb->dcb_is_zombie || dcb->session == NULL) { goto return_rc; } diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index b0dbd4aef..f68e01f6c 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -537,12 +537,12 @@ int gw_read_client_event(DCB* dcb) auth_val = MYSQL_AUTH_NO_SESSION; } } - if (MYSQL_AUTH_SUCCEEDED != auth_val) + if (MYSQL_AUTH_SUCCEEDED != auth_val && MYSQL_AUTH_SSL_INCOMPLETE != auth_val) { protocol->protocol_auth_state = MYSQL_AUTH_FAILED; mysql_client_auth_error_handling(dcb, auth_val); /** - * Close DCB and which will release MYSQL_session + * Close DCB and which will release MYSQL_session */ dcb_close(dcb); }