From ff5fe23ce606a186501eba7bd199110348416061 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Tue, 23 Dec 2014 16:10:27 +0200 Subject: [PATCH] dcb.b:dcb_final_free:replaced ((dcb->flags & DCBF_CLONE)==0) with macro !DCB_IS_CLONE(dcb) readwritesplit.h:Removed invalid macros which assumed that ROUTER_CLIENT_SES->rses_backend_ref always pointed to valid and used backend reference and thus included potential risk of NULL-pointer refernce. mysql_backend.c and mysql_client.c:avoid executing CHK_PROTOCOL(p) after original DCB has been released the memory. readwritesplit.c:Replaced RSES_CLEINT_DCB macro with a function which returns client DCB for a given router client session. --- server/core/dcb.c | 4 +-- .../include/mysql_client_server_protocol.h | 2 -- server/modules/include/readwritesplit.h | 5 +-- server/modules/protocol/mysql_backend.c | 4 +-- server/modules/protocol/mysql_client.c | 11 ++----- server/modules/protocol/mysql_common.c | 32 ------------------ .../routing/readwritesplit/readwritesplit.c | 33 ++++++++++++++++++- 7 files changed, 39 insertions(+), 52 deletions(-) diff --git a/server/core/dcb.c b/server/core/dcb.c index dc37add6c..2e06411a8 100644 --- a/server/core/dcb.c +++ b/server/core/dcb.c @@ -400,9 +400,9 @@ DCB_CALLBACK *cb; } } - if (dcb->protocol && ((dcb->flags & DCBF_CLONE) ==0)) + if (dcb->protocol && (!DCB_IS_CLONE(dcb))) free(dcb->protocol); - if (dcb->data && ((dcb->flags & DCBF_CLONE) ==0)) + if (dcb->data && (!DCB_IS_CLONE(dcb))) free(dcb->data); if (dcb->remote) free(dcb->remote); diff --git a/server/modules/include/mysql_client_server_protocol.h b/server/modules/include/mysql_client_server_protocol.h index b891bd7ba..a672de35b 100644 --- a/server/modules/include/mysql_client_server_protocol.h +++ b/server/modules/include/mysql_client_server_protocol.h @@ -303,11 +303,9 @@ typedef struct { #endif /** _MYSQL_PROTOCOL_H */ -void gw_mysql_close(MySQLProtocol **ptr); MySQLProtocol* mysql_protocol_init(DCB* dcb, int fd); void mysql_protocol_done (DCB* dcb); MySQLProtocol *gw_mysql_init(MySQLProtocol *data); -void gw_mysql_close(MySQLProtocol **ptr); int gw_receive_backend_auth(MySQLProtocol *protocol); int gw_decode_mysql_server_handshake(MySQLProtocol *protocol, uint8_t *payload); int gw_read_backend_handshake(MySQLProtocol *protocol); diff --git a/server/modules/include/readwritesplit.h b/server/modules/include/readwritesplit.h index dfd4f522f..19939c9e7 100644 --- a/server/modules/include/readwritesplit.h +++ b/server/modules/include/readwritesplit.h @@ -323,8 +323,5 @@ typedef struct router_instance { #define BACKEND_TYPE(b) (SERVER_IS_MASTER((b)->backend_server) ? BE_MASTER : \ (SERVER_IS_SLAVE((b)->backend_server) ? BE_SLAVE : BE_UNDEFINED)); -#define RSES_SESSION(r) (r->rses_backend_ref->bref_dcb->session) - -#define RSES_CLIENT_DCB(r) (RSES_SESSION(r)->client) - + #endif /*< _RWSPLITROUTER_H */ diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 96d80e066..ea0641b10 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -1495,7 +1495,7 @@ static GWBUF* process_response_data ( /** Get command which was stored in gw_MySQLWrite_backend */ p = DCB_PROTOCOL(dcb, MySQLProtocol); - CHK_PROTOCOL(p); + if (!DCB_IS_CLONE(dcb)) CHK_PROTOCOL(p); /** All buffers processed here are sescmd responses */ gwbuf_set_type(readbuf, GWBUF_TYPE_SESCMD_RESPONSE); @@ -1629,7 +1629,7 @@ static bool sescmd_response_complete( bool succp; p = DCB_PROTOCOL(dcb, MySQLProtocol); - CHK_PROTOCOL(p); + if (!DCB_IS_CLONE(dcb)) CHK_PROTOCOL(p); protocol_get_response_status(p, &npackets_left, &nbytes_left); diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index 7ab43b6ce..c265d2d2a 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1395,20 +1395,13 @@ gw_client_close(DCB *dcb) dcb->state == DCB_STATE_NOPOLLING || dcb->state == DCB_STATE_ZOMBIE) { - CHK_PROTOCOL(protocol); + if (!DCB_IS_CLONE(dcb)) CHK_PROTOCOL(protocol); } #endif LOGIF(LD, (skygw_log_write(LOGFILE_DEBUG, "%lu [gw_client_close]", pthread_self()))); - /** - * Since only protocol pointer is copied from original DCB to clone in - * dcb_clone, only dcb_close for the original DCB closes protocol. - */ - if (!DCB_IS_CLONE(dcb)) - { - mysql_protocol_done(dcb); - } + mysql_protocol_done(dcb); session = dcb->session; /** * session may be NULL if session_alloc failed. diff --git a/server/modules/protocol/mysql_common.c b/server/modules/protocol/mysql_common.c index b24260bef..8e57d8b21 100644 --- a/server/modules/protocol/mysql_common.c +++ b/server/modules/protocol/mysql_common.c @@ -151,38 +151,6 @@ retblock: } - - -/** - * gw_mysql_close - * - * close a connection if opened - * free data scructure for MySQLProtocol - * - * @param ptr The MySQLProtocol ** to close/free - * - */ -void gw_mysql_close(MySQLProtocol **ptr) { - MySQLProtocol *conn = *ptr; - - ss_dassert(*ptr != NULL); - - if (*ptr == NULL) - return; - - - if (conn->fd > 0) { - /* COM_QUIT will not be sent here, but from the caller of this routine! */ - close(conn->fd); - } else { - // no socket here - } - - free(*ptr); - - *ptr = NULL; -} - /** * Read the backend server MySQL handshake * diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index ef368a4fd..1fd7beec9 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -99,6 +99,7 @@ static int router_get_servercount(ROUTER_INSTANCE* router); static int rses_get_max_slavecount(ROUTER_CLIENT_SES* rses, int router_nservers); static int rses_get_max_replication_lag(ROUTER_CLIENT_SES* rses); static backend_ref_t* get_bref_from_dcb(ROUTER_CLIENT_SES* rses, DCB* dcb); +static DCB* rses_get_client_dcb(ROUTER_CLIENT_SES* rses); static route_target_t get_route_target ( skygw_query_type_t qtype, @@ -1770,6 +1771,33 @@ static void check_create_tmp_table( } } +/** + * Get client DCB pointer of the router client session. + * This routine must be protected by Router client session lock. + * + * @param rses Router client session pointer + * + * @return Pointer to client DCB + */ +static DCB* rses_get_client_dcb( + ROUTER_CLIENT_SES* rses) +{ + DCB* dcb = NULL; + int i; + + for (i=0; irses_nbackends; i++) + { + if ((dcb = rses->rses_backend_ref[i].bref_dcb) != NULL && + BREF_IS_IN_USE(&rses->rses_backend_ref[i]) && + dcb->session != NULL && + dcb->session->client != NULL) + { + return dcb->session->client; + } + } + return NULL; +} + /** * The main routing entry, this is called with every packet that is * received and has to be forwarded to the backend database. @@ -1806,6 +1834,9 @@ static int routeQuery( CHK_CLIENT_RSES(router_cli_ses); /** + * GWBUF is called "type undefined" when the incoming data isn't parsed + * and MySQL packets haven't been extracted to separate buffers. + * "Undefined" == "untyped". * Untyped GWBUF means that it can consist of incomplete and/or multiple * MySQL packets. * Read and route found MySQL packets one by one and store potential @@ -1824,7 +1855,7 @@ static int routeQuery( { if (GWBUF_LENGTH(tmpbuf) > 0) { - DCB* dcb = RSES_CLIENT_DCB(router_cli_ses); + DCB* dcb = rses_get_client_dcb(router_cli_ses); dcb->dcb_readqueue = gwbuf_append(dcb->dcb_readqueue, tmpbuf); }