From 08b99c121e937c45b5b74ab2e7ea4882c2b75f59 Mon Sep 17 00:00:00 2001 From: VilhoRaatikka Date: Tue, 5 Aug 2014 18:14:06 +0300 Subject: [PATCH] Bug #468, http://bugs.skysql.com/show_bug.cgi?id=468, Query classifier accessed freed thread context. If parsing fails thd doesn't need to be freed because it holds correct information about command type. session.c:session_setup_filters : fixed memory leak hintparser.c: added token_free for HINT_TOKENs and fixed a few memory leaks. mysql_client_server_protocol.h: added mysql_protocol_done which frees memory blocks pointed to by protocol members. Those can't be freed in dcb.c because dcb.c doesn't know about protocol's members. mysql_backend.c:gw_backend_close: fixed memory leak mysql_client.c: gw_client_close: fixed memory leak mysql_common.c: added implementation of mysql_protocol_done :protocol_archive_srv_command: tried to fix memory leak. Some memory is still leaking according to valgrind. Removed use of uninitialized local variable len. readwritesplit.c: Fix to bug #469, http://bugs.skysql.com/show_bug.cgi?id=469, rwsplit counts every connection twice in master - counnection counts leak execute_sescmd_in_backend: fixed a memory leak - visible only in DEBUG=Y build. readwritesplit/test/makefile: added target for hints tests --- query_classifier/query_classifier.cc | 14 +++---- server/core/session.c | 1 + server/modules/filter/hint/hintparser.c | 26 +++++++++++-- .../include/mysql_client_server_protocol.h | 1 + server/modules/protocol/mysql_backend.c | 2 + server/modules/protocol/mysql_client.c | 1 + server/modules/protocol/mysql_common.c | 31 ++++++++++++++-- .../routing/readwritesplit/readwritesplit.c | 12 +++--- .../routing/readwritesplit/test/makefile | 4 +- .../readwritesplit/test/test_hints/Makefile | 37 +++++++++++++++++++ .../readwritesplit/test/test_hints/hints.txt | 9 +++++ .../test/test_hints/rwsplit_hints.sh | 36 ++++++++++++++++++ .../test/test_hints/testrwsplit_hints.log | 15 ++++++++ utils/skygw_debug.h | 3 +- 14 files changed, 170 insertions(+), 22 deletions(-) create mode 100644 server/modules/routing/readwritesplit/test/test_hints/Makefile create mode 100644 server/modules/routing/readwritesplit/test/test_hints/hints.txt create mode 100755 server/modules/routing/readwritesplit/test/test_hints/rwsplit_hints.sh create mode 100644 server/modules/routing/readwritesplit/test/test_hints/testrwsplit_hints.log diff --git a/query_classifier/query_classifier.cc b/query_classifier/query_classifier.cc index 7215e2376..889940e00 100644 --- a/query_classifier/query_classifier.cc +++ b/query_classifier/query_classifier.cc @@ -149,17 +149,17 @@ skygw_query_type_t skygw_query_classifier_get_type( thd = get_or_create_thd_for_parsing(mysql, query_str); if (thd == NULL) - { - skygw_query_classifier_free(mysql); - } - /** Create parse_tree inside thd */ - failp = create_parse_tree(thd); - - if (failp) { skygw_query_classifier_free(mysql); *p_mysql = NULL; + goto return_qtype; } + /** + * Create parse_tree inside thd. + * thd and even lex are readable even if parser failed so let it + * continue despite failure. + */ + failp = create_parse_tree(thd); qtype = resolve_query_type(thd); if (p_mysql == NULL) diff --git a/server/core/session.c b/server/core/session.c index 9f1a55615..405a8dfc0 100644 --- a/server/core/session.c +++ b/server/core/session.c @@ -667,6 +667,7 @@ int i; session->filters[i].session = head->session; session->filters[i].instance = head->instance; session->head = *head; + free(head); } for (i = 0; i < service->n_filters; i++) diff --git a/server/modules/filter/hint/hintparser.c b/server/modules/filter/hint/hintparser.c index 4856e3336..075dc7bb1 100644 --- a/server/modules/filter/hint/hintparser.c +++ b/server/modules/filter/hint/hintparser.c @@ -77,9 +77,20 @@ static HINT *lookup_named_hint(HINT_SESSION *, char *); static void create_named_hint(HINT_SESSION *, char *, HINT *); static void hint_push(HINT_SESSION *, HINT *); static const char* token_get_keyword (TOKEN_VALUE token); +static void token_free(HINT_TOKEN* token); typedef enum { HM_EXECUTE, HM_START, HM_PREPARE } HINT_MODE; +void token_free(HINT_TOKEN* token) +{ + if (token->value != NULL) + { + free(token->value); + } + free(token); +} + + static const char* token_get_keyword ( TOKEN_VALUE token) { @@ -224,11 +235,13 @@ HINT_MODE mode = HM_EXECUTE; "with keyword '%s'", token_get_keyword(tok->token), token_get_keyword(TOK_MAXSCALE)))); - free(tok); + token_free(tok); goto retblock; } - + token_free(tok); + state = HS_INIT; + while ((tok = hint_next_token(&buf, &ptr)) != NULL && tok->token != TOK_EOL) { @@ -333,8 +346,13 @@ HINT_MODE mode = HM_EXECUTE; } break; } - free(tok); - } + token_free(tok); + } /*< while */ + + if (tok->token == TOK_EOL) + { + token_free(tok); + } switch (mode) { diff --git a/server/modules/include/mysql_client_server_protocol.h b/server/modules/include/mysql_client_server_protocol.h index f7d6030c1..9e2147d05 100644 --- a/server/modules/include/mysql_client_server_protocol.h +++ b/server/modules/include/mysql_client_server_protocol.h @@ -299,6 +299,7 @@ typedef struct { 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); diff --git a/server/modules/protocol/mysql_backend.c b/server/modules/protocol/mysql_backend.c index 6c101204e..b22d4aa52 100644 --- a/server/modules/protocol/mysql_backend.c +++ b/server/modules/protocol/mysql_backend.c @@ -993,6 +993,8 @@ gw_backend_close(DCB *dcb) /** Send COM_QUIT to the backend being closed */ mysql_send_com_quit(dcb, 0, quitbuf); + + mysql_protocol_done(dcb); if (session != NULL && session->state == SESSION_STATE_STOPPING) { diff --git a/server/modules/protocol/mysql_client.c b/server/modules/protocol/mysql_client.c index db61084eb..6cc9027de 100644 --- a/server/modules/protocol/mysql_client.c +++ b/server/modules/protocol/mysql_client.c @@ -1325,6 +1325,7 @@ gw_client_close(DCB *dcb) CHK_PROTOCOL(protocol); } #endif + mysql_protocol_done(dcb); session = dcb->session; /** diff --git a/server/modules/protocol/mysql_common.c b/server/modules/protocol/mysql_common.c index 4ce34452b..70050a13c 100644 --- a/server/modules/protocol/mysql_common.c +++ b/server/modules/protocol/mysql_common.c @@ -96,6 +96,31 @@ return_p: } +/** + * mysql_protocol_done + * + * free protocol allocations. + * + * @param dcb owner DCB + * + */ +void mysql_protocol_done ( + DCB* dcb) +{ + server_command_t* scmd = ((MySQLProtocol *)dcb->protocol)->protocol_cmd_history; + server_command_t* scmd2; + + while (scmd != NULL) + { + scmd2 = scmd->scom_next; + free(scmd); + scmd = scmd2; + } +} + + + + /** * gw_mysql_close * @@ -1601,7 +1626,7 @@ void protocol_archive_srv_command( { server_command_t* s1; server_command_t** s2; - int len; + int len = 0; spinlock_acquire(&p->protocol_lock); @@ -1617,9 +1642,7 @@ void protocol_archive_srv_command( s2 = &p->protocol_cmd_history; if (*s2 != NULL) - { - len = 0; - + { while ((*s2)->scom_next != NULL) { *s2 = (*s2)->scom_next; diff --git a/server/modules/routing/readwritesplit/readwritesplit.c b/server/modules/routing/readwritesplit/readwritesplit.c index a6a67d9dc..8d989f391 100644 --- a/server/modules/routing/readwritesplit/readwritesplit.c +++ b/server/modules/routing/readwritesplit/readwritesplit.c @@ -1272,6 +1272,8 @@ static int routeQuery( break; case MYSQL_COM_STMT_EXECUTE: + /** Parsing is not needed for this type of packet */ +#if defined(NOT_USED) plainsqlbuf = gwbuf_clone_transform(querybuf, GWBUF_TYPE_PLAINSQL); len = GWBUF_LENGTH(plainsqlbuf); @@ -1280,7 +1282,8 @@ static int routeQuery( memcpy(querystr, startpos, len); memset(&querystr[len], 0, 1); qtype = skygw_query_classifier_get_type(querystr, 0, &mysql); - qtype |= QUERY_TYPE_EXEC_STMT; +#endif + qtype = QUERY_TYPE_EXEC_STMT; break; case MYSQL_COM_SHUTDOWN: /**< 8 where should shutdown be routed ? */ @@ -2146,7 +2149,7 @@ static bool select_connect_backend_servers( backend_ref[i].bref_state = 0; bref_set_state(&backend_ref[i], BREF_IN_USE); - /** + /** * Increase backend connection counter. * Server's stats are _increased_ in * dcb.c:dcb_alloc ! @@ -2184,7 +2187,7 @@ static bool select_connect_backend_servers( session, b->backend_server->protocol); - if (backend_ref[i].bref_dcb != NULL) + if (backend_ref[i].bref_dcb != NULL) { master_connected = true; /** @@ -2201,8 +2204,6 @@ static bool select_connect_backend_servers( bref_set_state(&backend_ref[i], BREF_IN_USE); /** Increase backend connection counters */ - atomic_add(&b->backend_server->stats.n_current, 1); - atomic_add(&b->backend_server->stats.n_connections, 1); atomic_add(&b->backend_conn_count, 1); } else @@ -2832,6 +2833,7 @@ static bool execute_sescmd_in_backend( pthread_self(), dcb->fd, STRPACKETTYPE(cmd)))); + gwbuf_free(tmpbuf); } #endif /*< SS_DEBUG */ switch (scur->scmd_cur_cmd->my_sescmd_packet_type) { diff --git a/server/modules/routing/readwritesplit/test/makefile b/server/modules/routing/readwritesplit/test/makefile index 87d22363d..b4f9aec6b 100644 --- a/server/modules/routing/readwritesplit/test/makefile +++ b/server/modules/routing/readwritesplit/test/makefile @@ -14,6 +14,7 @@ RET := -1 cleantests: - $(DEL) *.o - $(DEL) *~ + -$(MAKE) -C test_hints cleantests testall: @@ -32,4 +33,5 @@ runtests: @echo "-------------------------------" >> $(TESTLOG) ./rwsplit.sh $(TESTLOG) $(THOST) $(TPORT_RW) $(TMASTER_ID) $(TUSER) $(TPWD) @echo "" >> $(TESTLOG) - @cat $(TESTLOG) >> $(TEST_MAXSCALE_LOG) \ No newline at end of file + @cat $(TESTLOG) >> $(TEST_MAXSCALE_LOG) + -$(MAKE) -C test_hints runtests diff --git a/server/modules/routing/readwritesplit/test/test_hints/Makefile b/server/modules/routing/readwritesplit/test/test_hints/Makefile new file mode 100644 index 000000000..08462da4a --- /dev/null +++ b/server/modules/routing/readwritesplit/test/test_hints/Makefile @@ -0,0 +1,37 @@ +# cleantests - clean local and subdirectories' tests +# buildtests - build all local and subdirectories' tests +# runtests - run all local tests +# testall - clean, build and run local and subdirectories' tests + +include ../../../../../../build_gateway.inc +include $(ROOT_PATH)/makefile.inc +include $(ROOT_PATH)/test.inc + +ARGS=6 + +CC=cc +TESTLOG := $(shell pwd)/testrwsplit_hints.log +RET := -1 + +cleantests: + - $(DEL) *.o + - $(DEL) *~ + + +testall: + -$(MAKE) cleantests + -$(MAKE) DEBUG=Y buildtests + -$(MAKE) runtests + +buildtests: + + +runtests: + @echo "" > $(TESTLOG) + @echo "-------------------------------" >> $(TESTLOG) + @echo $(shell date) >> $(TESTLOG) + @echo "Test Read/Write split router - hint routing" >> $(TESTLOG) + @echo "-------------------------------" >> $(TESTLOG) + ./rwsplit_hints.sh $(TESTLOG) $(THOST) $(TPORT_RW) $(TMASTER_ID) $(TUSER) $(TPWD) + @echo "" >> $(TESTLOG) + @cat $(TESTLOG) >> $(TEST_MAXSCALE_LOG) diff --git a/server/modules/routing/readwritesplit/test/test_hints/hints.txt b/server/modules/routing/readwritesplit/test/test_hints/hints.txt new file mode 100644 index 000000000..34196ca36 --- /dev/null +++ b/server/modules/routing/readwritesplit/test/test_hints/hints.txt @@ -0,0 +1,9 @@ +-- maxscale route to master:3000 +-- maxscale route to server server1:3000 +-- maxscale route to server server2:3001 +-- maxscale route to server server3:3002 +-- maxscale route to server server4:3003 +-- maxscale max_slave_replication_lag = 100: +-- maxscale max_slave_replication_lag= 100: +-- maxscale max_slave_replication_lag =100: +-- maxscale max_slave_replication_lag=100: diff --git a/server/modules/routing/readwritesplit/test/test_hints/rwsplit_hints.sh b/server/modules/routing/readwritesplit/test/test_hints/rwsplit_hints.sh new file mode 100755 index 000000000..50b4c6026 --- /dev/null +++ b/server/modules/routing/readwritesplit/test/test_hints/rwsplit_hints.sh @@ -0,0 +1,36 @@ +#!/bin/bash +NARGS=6 +TLOG=$1 +THOST=$2 +TPORT=$3 +TMASTER_ID=$4 +TUSER=$5 +TPWD=$6 + +if [ $# != $NARGS ] ; +then +echo"" +echo "Wrong number of arguments, gave "$#" but "$NARGS" is required" +echo "" +echo "Usage :" +echo " rwsplit.sh " +echo "" +exit 1 +fi + +TESTINPUT=hints.txt +QUERY="select @@server_id;" +RUNCMD=mysql\ --host=$THOST\ -P$TPORT\ -u$TUSER\ -p$TPWD\ --unbuffered=true\ --disable-reconnect\ --silent\ + +while read -r LINE +do +TINPUT=`echo "$LINE"|awk '{split($0,a,":");print a[1]}'` +TRETVAL=`echo "$LINE"|awk '{split($0,a,":");print a[2]}'` +a=`$RUNCMD -e"$QUERY$TINPUT"` +if [ "$a" != "$TRETVAL" ]; then + echo "$TINPUT FAILED, return value $a when $TRETVAL was expected">>$TLOG; +else + echo "$TINPUT PASSED">>$TLOG ; +fi + +done < $TESTINPUT diff --git a/server/modules/routing/readwritesplit/test/test_hints/testrwsplit_hints.log b/server/modules/routing/readwritesplit/test/test_hints/testrwsplit_hints.log new file mode 100644 index 000000000..021b52b72 --- /dev/null +++ b/server/modules/routing/readwritesplit/test/test_hints/testrwsplit_hints.log @@ -0,0 +1,15 @@ + +------------------------------- +Tue Aug 5 13:31:41 EEST 2014 +Test Read/Write split router - hint routing +------------------------------- +-- maxscale route to master FAILED, return value 3003 when 3000 was expected +-- maxscale route to server server1 FAILED, return value 3003 when 3000 was expected +-- maxscale route to server server2 FAILED, return value 3003 when 3001 was expected +-- maxscale route to server server3 FAILED, return value 3003 when 3002 was expected +-- maxscale route to server server4 PASSED +-- maxscale max_slave_replication_lag = 100 FAILED, return value 3003 when was expected +-- maxscale max_slave_replication_lag= 100 FAILED, return value 3003 when was expected +-- maxscale max_slave_replication_lag =100 FAILED, return value 3003 when was expected +-- maxscale max_slave_replication_lag=100 FAILED, return value 3003 when was expected + diff --git a/utils/skygw_debug.h b/utils/skygw_debug.h index 43a609a40..8935deac6 100644 --- a/utils/skygw_debug.h +++ b/utils/skygw_debug.h @@ -133,7 +133,8 @@ typedef enum skygw_chk_t { ((t) == QUERY_TYPE_SESSION_WRITE ? "QUERY_TYPE_SESSION_WRITE" : \ ((t) == QUERY_TYPE_UNKNOWN ? "QUERY_TYPE_UNKNOWN" : \ ((t) == QUERY_TYPE_LOCAL_READ ? "QUERY_TYPE_LOCAL_READ" : \ - "Unknown query type"))))) + ((t) == QUERY_TYPE_EXEC_STMT ? "QUERY_TYPE_EXEC_STMT" : \ + "Unknown query type")))))) #define STRLOGID(i) ((i) == LOGFILE_TRACE ? "LOGFILE_TRACE" : \ ((i) == LOGFILE_MESSAGE ? "LOGFILE_MESSAGE" : \