From 88716c35fba54ca95120ee2f579996cdcf967c67 Mon Sep 17 00:00:00 2001 From: counterpoint Date: Mon, 21 Sep 2015 09:23:22 +0100 Subject: [PATCH] Various changes to block loopholes in different cases and tidy up. --- server/core/dcb.c | 147 ++++++++++++------ server/core/poll.c | 7 +- server/modules/protocol/mysql_backend.c | 16 +- server/modules/routing/readconnroute.c | 6 +- .../routing/readwritesplit/readwritesplit.c | 9 +- 5 files changed, 122 insertions(+), 63 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index 4bbcc80c9..bd7d5677e 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -289,10 +289,10 @@ dcb_final_free(DCB *dcb) { DCB_CALLBACK *cb; - CHK_DCB(dcb); - ss_info_dassert(dcb->state == DCB_STATE_DISCONNECTED || - dcb->state == DCB_STATE_ALLOC, - "dcb not in DCB_STATE_DISCONNECTED not in DCB_STATE_ALLOC state."); + CHK_DCB(dcb); + ss_info_dassert(dcb->state == DCB_STATE_DISCONNECTED || + dcb->state == DCB_STATE_ALLOC, + "dcb not in DCB_STATE_DISCONNECTED not in DCB_STATE_ALLOC state."); if (DCB_POLL_BUSY(dcb)) { @@ -326,53 +326,58 @@ dcb_final_free(DCB *dcb) if (ptr) ptr->next = dcb->next; } - spinlock_release(&dcbspin); + spinlock_release(&dcbspin); - if (dcb->session) { - /*< - * Terminate client session. - */ - { - SESSION *local_session = dcb->session; - dcb->session = NULL; - CHK_SESSION(local_session); - /** - * Set session's client pointer NULL so that other threads - * won't try to call dcb_close for client DCB - * after this call. - */ - if (local_session->client == dcb) - { - spinlock_acquire(&local_session->ses_lock); - local_session->client = NULL; - spinlock_release(&local_session->ses_lock); - } - if (SESSION_STATE_DUMMY != local_session->state) - { - session_free(local_session); - } - } + if (dcb->session) { + /*< + * Terminate client session. + */ + SESSION *local_session = dcb->session; + dcb->session = NULL; + CHK_SESSION(local_session); + /** + * Set session's client pointer NULL so that other threads + * won't try to call dcb_close for client DCB + * after this call. + */ + if (local_session->client == dcb) + { + spinlock_acquire(&local_session->ses_lock); + local_session->client = NULL; + spinlock_release(&local_session->ses_lock); + } + if (SESSION_STATE_DUMMY != local_session->state) + { + session_free(local_session); + } } if (dcb->protocol && (!DCB_IS_CLONE(dcb))) free(dcb->protocol); - if (dcb->protoname) - free(dcb->protoname); + if (dcb->protoname) + free(dcb->protoname); if (dcb->remote) free(dcb->remote); if (dcb->user) free(dcb->user); - /* Clear write and read buffers */ - if (dcb->delayq) { - GWBUF *queue = dcb->delayq; - while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); - } - if (dcb->dcb_readqueue) - { - GWBUF* queue = dcb->dcb_readqueue; - while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); - } + /* Clear write and read buffers */ + if (dcb->delayq) { + GWBUF *queue = dcb->delayq; + while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); + dcb->delayq = NULL; + } + if (dcb->writeq) { + GWBUF *queue = dcb->writeq; + while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); + dcb->writeq = NULL; + } + if (dcb->dcb_readqueue) + { + GWBUF* queue = dcb->dcb_readqueue; + while ((queue = gwbuf_consume(queue, GWBUF_LENGTH(queue))) != NULL); + dcb->dcb_readqueue = NULL; + } spinlock_acquire(&dcb->cb_lock); while ((cb = dcb->callbacks) != NULL) @@ -549,10 +554,36 @@ dcb_process_victim_queue(DCB *listofdcb) dcb = dcb->memdata.next; continue; } + else + { + DCB *nextdcb; + poll_remove_dcb(dcb); + spinlock_acquire(&zombiespin); + bitmask_copy(&dcb->memdata.bitmask, poll_bitmask()); + nextdcb = dcb->memdata.next; + dcb->memdata.next = zombies; + zombies = dcb; + spinlock_release(&zombiespin); + if (dcb->server) + { + atomic_add(&dcb->server->stats.n_current, -1); + } + dcb = nextdcb; + continue; + } } - dcb_close_finish(dcb); } - + /** + * close protocol and router session + */ + if (dcb->func.close != NULL) + { + dcb->func.close(dcb); + } + /** Call possible callback for this DCB in case of close */ + dcb_call_callback(dcb, DCB_REASON_CLOSE); + + if (dcb->fd > 0) { /*< @@ -1826,7 +1857,7 @@ dcb_close(DCB *dcb) pthread_self(), dcb, STRDCBSTATE(dcb->state)))); - raise(SIGABRT); + raise(SIGABRT); } /** @@ -1902,6 +1933,20 @@ dcb_maybe_add_persistent(DCB *dcb) dcb->user))); dcb->dcb_is_zombie = false; dcb->persistentstart = time(NULL); + if (dcb->session) + /*< + * Terminate client session. + */ + { + SESSION *local_session = dcb->session; + session_set_dummy(dcb); + CHK_SESSION(local_session); + if (SESSION_STATE_DUMMY != local_session->state) + { + session_free(local_session); + } + } + dcb->callbacks = NULL; spinlock_acquire(&dcb->server->persistlock); dcb->nextpersistent = dcb->server->persistent; dcb->server->persistent = dcb; @@ -2063,6 +2108,15 @@ dprintOneDCB(DCB *pdcb, DCB *dcb) if (dcb->remote) dcb_printf(pdcb, "\tConnected to: %s\n", dcb->remote); + if (dcb->server) + { + if (dcb->server->name) + dcb_printf(pdcb, "\tServer name/IP: %s\n", + dcb->server->name); + if (dcb->server->port) + dcb_printf(pdcb, "\tPort number: %d\n", + dcb->server->port); + } if (dcb->user) dcb_printf(pdcb, "\tUsername: %s\n", dcb->user); @@ -2858,7 +2912,10 @@ dcb_persistent_clean_count(DCB *dcb, bool cleanall) { nextdcb = disposals->nextpersistent; disposals->persistentstart = 0; - dcb_close_finish(disposals); + if (DCB_STATE_POLLING == dcb->state) + { + poll_remove_dcb(dcb); + } dcb_close(disposals); disposals = nextdcb; } diff --git a/server/core/poll.c b/server/core/poll.c index b3fb82920..2e6745987 100644 --- a/server/core/poll.c +++ b/server/core/poll.c @@ -860,7 +860,12 @@ unsigned long qtime; #endif /* FAKE_CODE */ ss_debug(spinlock_acquire(&dcb->dcb_initlock);) ss_dassert(dcb->state != DCB_STATE_ALLOC); - ss_dassert(dcb->state != DCB_STATE_DISCONNECTED); + /* It isn't obvious that this is impossible */ + /* ss_dassert(dcb->state != DCB_STATE_DISCONNECTED); */ + if (DCB_STATE_DISCONNECTED == dcb->state) + { + return 0; + } ss_debug(spinlock_release(&dcb->dcb_initlock);) LOGIF(LD, (skygw_log_write( diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 94efb669c..d72d6a639 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -167,7 +167,7 @@ static int gw_read_backend_event(DCB *dcb) { int rc = 0; CHK_DCB(dcb); - if (!dcb->session && dcb->persistentstart) + if (dcb->persistentstart) { dcb->dcb_errhandle_called = true; goto return_rc; @@ -407,16 +407,11 @@ static int gw_read_backend_event(DCB *dcb) { else { dcb->dcb_errhandle_called = true; + dcb_close(dcb); + rc = 1; + goto return_rc; } gwbuf_free(errbuf); - LOGIF(LD, (skygw_log_write( - LOGFILE_DEBUG, - "%lu [gw_read_backend_event] " - "after calling handleError. Backend " - "DCB %p, session %p", - pthread_self(), - dcb, - dcb->session))); spinlock_acquire(&session->ses_lock); session->state = SESSION_STATE_STOPPING; @@ -1066,7 +1061,7 @@ gw_backend_hangup(DCB *dcb) session_state_t ses_state; CHK_DCB(dcb); - if (!dcb->session && dcb->persistentstart) + if (dcb->persistentstart) { dcb->dcb_errhandle_called = true; goto retblock; @@ -1124,6 +1119,7 @@ gw_backend_hangup(DCB *dcb) } } gwbuf_free(errbuf); + dcb_close(dcb); goto retblock; } #if defined(SS_DEBUG) diff --git a/server/modules/routing/readconnroute.c b/server/modules/routing/readconnroute.c index e32bdd677..ceeb0a627 100644 --- a/server/modules/routing/readconnroute.c +++ b/server/modules/routing/readconnroute.c @@ -886,7 +886,7 @@ static void handleError( spinlock_release(&session->ses_lock); } - if (router_cli_ses->backend_dcb) { + if (router_cli_ses && router_cli_ses->backend_dcb) { if (backend_dcb != router_cli_ses->backend_dcb) { /* Linkages have gone badly wrong */ @@ -898,9 +898,9 @@ static void handleError( backend_dcb))); } router_cli_ses->backend_dcb = NULL; - dcb_close(backend_dcb); } - + dcb_close(backend_dcb); + /** false because connection is not available anymore */ *succp = false; } diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index d8d8033f0..1a42f1b12 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -990,7 +990,7 @@ static void closeSession( int i; /** * This sets router closed. Nobody is allowed to use router - * whithout checking this first. + * without checking this first. */ router_cli_ses->rses_closed = true; @@ -5306,8 +5306,6 @@ static int router_handle_state_switch( backend_ref_t* bref; int rc = 1; SERVER* srv; - ROUTER_CLIENT_SES* rses; - SESSION* ses; CHK_DCB(dcb); bref = (backend_ref_t *)data; CHK_BACKEND_REF(bref); @@ -5327,7 +5325,10 @@ static int router_handle_state_switch( srv->port, STRSRVSTATUS(srv)))); CHK_SESSION(((SESSION*)dcb->session)); - CHK_CLIENT_RSES(((ROUTER_CLIENT_SES *)dcb->session->router_session)); + if (dcb->session->router_session) + { + CHK_CLIENT_RSES(((ROUTER_CLIENT_SES *)dcb->session->router_session)); + } switch (reason) { case DCB_REASON_NOT_RESPONDING: