From cc54d80a8b7811da06ddeb40c2402f957441eb1f Mon Sep 17 00:00:00 2001 From: ekorh475 Date: Fri, 25 Nov 2016 12:53:15 +0200 Subject: [PATCH] Fix memory leaks in maxinfo (modified for 2.0 and develop-branch) MXS-1009. This commit adds a gwbuf_free after maxinfo_execute() to free a buffer with an sql-query after it has been processed. Also, the parse tree in maxinfo_execute_query() is now freed. The tree_free- function was renamed to maxinfo_tree_free, since it is now globally available. This commit has additional changes (in relation to the 1.4.4 branch) to remove errors caused by differences in the html and sql-sides of MaxInfo. --- server/modules/include/maxinfo.h | 1 + server/modules/routing/maxinfo/maxinfo.c | 189 +++++++++--------- server/modules/routing/maxinfo/maxinfo_exec.c | 36 ++-- .../modules/routing/maxinfo/maxinfo_parse.c | 116 ++++++----- 4 files changed, 186 insertions(+), 156 deletions(-) diff --git a/server/modules/include/maxinfo.h b/server/modules/include/maxinfo.h index 676a06ea7..c2d0d4b48 100644 --- a/server/modules/include/maxinfo.h +++ b/server/modules/include/maxinfo.h @@ -132,6 +132,7 @@ typedef enum extern MAXINFO_TREE *maxinfo_parse(char *, PARSE_ERROR *); +extern void maxinfo_free_tree(MAXINFO_TREE *); extern void maxinfo_execute(DCB *, MAXINFO_TREE *); extern void maxinfo_send_error(DCB *, int, char *); extern void maxinfo_send_parse_error(DCB *, char *, PARSE_ERROR); diff --git a/server/modules/routing/maxinfo/maxinfo.c b/server/modules/routing/maxinfo/maxinfo.c index d836b20d4..c5ce5d472 100644 --- a/server/modules/routing/maxinfo/maxinfo.c +++ b/server/modules/routing/maxinfo/maxinfo.c @@ -335,54 +335,60 @@ static void handleError( static int execute(ROUTER *rinstance, void *router_session, GWBUF *queue) { -INFO_INSTANCE *instance = (INFO_INSTANCE *)rinstance; -INFO_SESSION *session = (INFO_SESSION *)router_session; -uint8_t *data; -int length, len, residual; -char *sql; + INFO_INSTANCE *instance = (INFO_INSTANCE *)rinstance; + INFO_SESSION *session = (INFO_SESSION *)router_session; + uint8_t *data; + int length, len, residual; + char *sql; - if (GWBUF_TYPE(queue) == GWBUF_TYPE_HTTP) - { - return handle_url(instance, session, queue); - } - if (session->queue) - { - queue = gwbuf_append(session->queue, queue); - session->queue = NULL; - queue = gwbuf_make_contiguous(queue); - } - data = (uint8_t *)GWBUF_DATA(queue); - length = data[0] + (data[1] << 8) + (data[2] << 16); - if (length + 4 > GWBUF_LENGTH(queue)) - { - // Incomplete packet, must be buffered - session->queue = queue; - return 1; - } + if (GWBUF_TYPE(queue) == GWBUF_TYPE_HTTP) + { + return handle_url(instance, session, queue); + } + if (session->queue) + { + queue = gwbuf_append(session->queue, queue); + session->queue = NULL; + queue = gwbuf_make_contiguous(queue); + } + data = (uint8_t *)GWBUF_DATA(queue); + length = data[0] + (data[1] << 8) + (data[2] << 16); + if (length + 4 > GWBUF_LENGTH(queue)) + { + // Incomplete packet, must be buffered + session->queue = queue; + return 1; + } - // We have a complete request in a signle buffer - if (modutil_MySQL_Query(queue, &sql, &len, &residual)) - { - sql = strndup(sql, len); - int rc = maxinfo_execute_query(instance, session, sql); - free(sql); - return rc; - } - else - { - switch (MYSQL_COMMAND(queue)) - { - case COM_PING: - return maxinfo_ping(instance, session, queue); - case COM_STATISTICS: - return maxinfo_statistics(instance, session, queue); - default: - MXS_ERROR("maxinfo: Unexpected MySQL command 0x%x", - MYSQL_COMMAND(queue)); - } - } - - return 1; + int rc = 1; + // We have a complete request in a single buffer + if (modutil_MySQL_Query(queue, &sql, &len, &residual)) + { + sql = strndup(sql, len); + rc = maxinfo_execute_query(instance, session, sql); + free(sql); + } + else + { + switch (MYSQL_COMMAND(queue)) + { + case COM_PING: + rc = maxinfo_ping(instance, session, queue); + break; + case COM_STATISTICS: + rc = maxinfo_statistics(instance, session, queue); + break; + case COM_QUIT: + break; + default: + MXS_ERROR("maxinfo: Unexpected MySQL command 0x%x", + MYSQL_COMMAND(queue)); + break; + } + } + // MaxInfo doesn't route the data forward so it should be freed. + gwbuf_free(queue); + return rc; } /** @@ -601,51 +607,54 @@ uint8_t *ptr; static int maxinfo_execute_query(INFO_INSTANCE *instance, INFO_SESSION *session, char *sql) { -MAXINFO_TREE *tree; -PARSE_ERROR err; + MAXINFO_TREE *tree; + PARSE_ERROR err; - MXS_INFO("maxinfo: SQL statement: '%s' for 0x%p.", - sql, session->dcb); - if (strcmp(sql, "select @@version_comment limit 1") == 0) - { - respond_vercom(session->dcb); - return 1; - } - /* Below is a kludge for MonYog, if we see - * select unix_timestamp... as starttime - * just return the starttime of MaxScale - */ - if (strncasecmp(sql, "select UNIX_TIMESTAMP", - strlen("select UNIX_TIMESTAMP")) == 0 - && (strstr(sql, "as starttime") != NULL || strstr(sql, "AS starttime") != NULL)) - { - respond_starttime(session->dcb); - return 1; - } - if (strcasecmp(sql, "set names 'utf8'") == 0) - { - return maxinfo_send_ok(session->dcb); - } - if (strncasecmp(sql, "set session", 11) == 0) - { - return maxinfo_send_ok(session->dcb); - } - if (strncasecmp(sql, "set autocommit", 14) == 0) - { - return maxinfo_send_ok(session->dcb); - } - if (strncasecmp(sql, "SELECT `ENGINES`.`SUPPORT`", 26) == 0) - { - return maxinfo_send_ok(session->dcb); - } - if ((tree = maxinfo_parse(sql, &err)) == NULL) - { - maxinfo_send_parse_error(session->dcb, sql, err); - MXS_NOTICE("Failed to parse SQL statement: '%s'.", sql); - } - else - maxinfo_execute(session->dcb, tree); - return 1; + MXS_INFO("maxinfo: SQL statement: '%s' for 0x%p.", + sql, session->dcb); + if (strcmp(sql, "select @@version_comment limit 1") == 0) + { + respond_vercom(session->dcb); + return 1; + } + /* Below is a kludge for MonYog, if we see + * select unix_timestamp... as starttime + * just return the starttime of MaxScale + */ + if (strncasecmp(sql, "select UNIX_TIMESTAMP", + strlen("select UNIX_TIMESTAMP")) == 0 + && (strstr(sql, "as starttime") != NULL || strstr(sql, "AS starttime") != NULL)) + { + respond_starttime(session->dcb); + return 1; + } + if (strcasecmp(sql, "set names 'utf8'") == 0) + { + return maxinfo_send_ok(session->dcb); + } + if (strncasecmp(sql, "set session", 11) == 0) + { + return maxinfo_send_ok(session->dcb); + } + if (strncasecmp(sql, "set autocommit", 14) == 0) + { + return maxinfo_send_ok(session->dcb); + } + if (strncasecmp(sql, "SELECT `ENGINES`.`SUPPORT`", 26) == 0) + { + return maxinfo_send_ok(session->dcb); + } + if ((tree = maxinfo_parse(sql, &err)) == NULL) + { + maxinfo_send_parse_error(session->dcb, sql, err); + MXS_NOTICE("Failed to parse SQL statement: '%s'.", sql); + } + else + { + maxinfo_execute(session->dcb, tree); + maxinfo_free_tree(tree); + } + return 1; } /** diff --git a/server/modules/routing/maxinfo/maxinfo_exec.c b/server/modules/routing/maxinfo/maxinfo_exec.c index 884d65e42..235a8d400 100644 --- a/server/modules/routing/maxinfo/maxinfo_exec.c +++ b/server/modules/routing/maxinfo/maxinfo_exec.c @@ -881,22 +881,28 @@ variable_row(RESULTSET *result, void *data) static void exec_show_variables(DCB *dcb, MAXINFO_TREE *filter) { - RESULTSET *result; - VARCONTEXT context; + RESULTSET *result; + VARCONTEXT *context; + + if ((context = malloc(sizeof(VARCONTEXT))) == NULL) + { + return; + } if (filter) { - context.like = filter->value; + context->like = filter->value; } else { - context.like = NULL; + context->like = NULL; } - context.index = 0; + context->index = 0; - if ((result = resultset_create(variable_row, &context)) == NULL) + if ((result = resultset_create(variable_row, context)) == NULL) { maxinfo_send_error(dcb, 0, "No resources available"); + free(context); return; } resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); @@ -1167,22 +1173,28 @@ status_row(RESULTSET *result, void *data) static void exec_show_status(DCB *dcb, MAXINFO_TREE *filter) { - RESULTSET *result; - VARCONTEXT context; + RESULTSET *result; + VARCONTEXT *context; + + if ((context = malloc(sizeof(VARCONTEXT))) == NULL) + { + return; + } if (filter) { - context.like = filter->value; + context->like = filter->value; } else { - context.like = NULL; + context->like = NULL; } - context.index = 0; + context->index = 0; - if ((result = resultset_create(status_row, &context)) == NULL) + if ((result = resultset_create(status_row, context)) == NULL) { maxinfo_send_error(dcb, 0, "No resources available"); + free(context); return; } resultset_add_column(result, "Variable_name", 40, COL_TYPE_VARCHAR); diff --git a/server/modules/routing/maxinfo/maxinfo_parse.c b/server/modules/routing/maxinfo/maxinfo_parse.c index 437050255..556b33f4b 100644 --- a/server/modules/routing/maxinfo/maxinfo_parse.c +++ b/server/modules/routing/maxinfo/maxinfo_parse.c @@ -41,7 +41,7 @@ #include static MAXINFO_TREE *make_tree_node(MAXINFO_OPERATOR, char *, MAXINFO_TREE *, MAXINFO_TREE *); -static void free_tree(MAXINFO_TREE *); +void maxinfo_free_tree(MAXINFO_TREE *); // This function is needed by maxinfo.c static char *fetch_token(char *, int *, char **); static MAXINFO_TREE *parse_column_list(char **sql); static MAXINFO_TREE *parse_table_name(char **sql); @@ -62,44 +62,46 @@ char *ptr, *text; MAXINFO_TREE *tree = NULL; MAXINFO_TREE *col, *table; - *parse_error = PARSE_NOERROR; - while ((ptr = fetch_token(sql, &token, &text)) != NULL) - { - switch (token) - { - case LT_SHOW: - free(text); // not needed - ptr = fetch_token(ptr, &token, &text); - if (ptr == NULL || token != LT_STRING) - { - // Expected show "name" - *parse_error = PARSE_MALFORMED_SHOW; - return NULL; - } - tree = make_tree_node(MAXOP_SHOW, text, NULL, NULL); - if ((ptr = fetch_token(ptr, &token, &text)) == NULL) - return tree; - else if (token == LT_LIKE) - { - if ((ptr = fetch_token(ptr, &token, &text)) != NULL) - { - tree->right = make_tree_node(MAXOP_LIKE, - text, NULL, NULL); - return tree; - } - else - { - // Expected expression - *parse_error = PARSE_EXPECTED_LIKE; - free_tree(tree); - return NULL; - } - } - // Malformed show - free(text); - free_tree(tree); - *parse_error = PARSE_MALFORMED_SHOW; - return NULL; + *parse_error = PARSE_NOERROR; + while ((ptr = fetch_token(sql, &token, &text)) != NULL) + { + switch (token) + { + case LT_SHOW: + free(text); // not needed + ptr = fetch_token(ptr, &token, &text); + if (ptr == NULL || token != LT_STRING) + { + // Expected show "name" + *parse_error = PARSE_MALFORMED_SHOW; + return NULL; + } + tree = make_tree_node(MAXOP_SHOW, text, NULL, NULL); + if ((ptr = fetch_token(ptr, &token, &text)) == NULL) + { + return tree; + } + else if (token == LT_LIKE) + { + if ((ptr = fetch_token(ptr, &token, &text)) != NULL) + { + tree->right = make_tree_node(MAXOP_LIKE, + text, NULL, NULL); + return tree; + } + else + { + // Expected expression + *parse_error = PARSE_EXPECTED_LIKE; + maxinfo_free_tree(tree); + return NULL; + } + } + // Malformed show + free(text); + maxinfo_free_tree(tree); + *parse_error = PARSE_MALFORMED_SHOW; + return NULL; #if 0 case LT_SELECT: free(text); // not needed @@ -128,7 +130,7 @@ MAXINFO_TREE *col, *table; { /** Unknown token after SHUTDOWN MONITOR|SERVICE */ *parse_error = PARSE_SYNTAX_ERROR; - free_tree(tree); + maxinfo_free_tree(tree); return NULL; } return tree; @@ -142,7 +144,7 @@ MAXINFO_TREE *col, *table; { /** Missing token for RESTART MONITOR|SERVICE */ *parse_error = PARSE_SYNTAX_ERROR; - free_tree(tree); + maxinfo_free_tree(tree); return NULL; } tree->right = make_tree_node(MAXOP_LITERAL, text, NULL, NULL); @@ -152,7 +154,7 @@ MAXINFO_TREE *col, *table; /** Unknown token after RESTART MONITOR|SERVICE */ *parse_error = PARSE_SYNTAX_ERROR; free(text); - free_tree(tree); + maxinfo_free_tree(tree); return NULL; } return tree; @@ -270,20 +272,26 @@ MAXINFO_TREE *node; } /** - * Recusrsively free the storage associated with a parse tree + * Recursively free the storage associated with a parse tree * * @param tree The parse tree to free */ -static void -free_tree(MAXINFO_TREE *tree) +void +maxinfo_free_tree(MAXINFO_TREE *tree) { - if (tree->left) - free_tree(tree->left); - if (tree->right) - free_tree(tree->right); - if (tree->value) - free(tree->value); - free(tree); + if (tree->left) + { + maxinfo_free_tree(tree->left); + } + if (tree->right) + { + maxinfo_free_tree(tree->right); + } + if (tree->value) + { + free(tree->value); + } + free(tree); } /** @@ -410,8 +418,8 @@ MAXINFO_TREE* maxinfo_parse_literals(MAXINFO_TREE *tree, int min_args, char *ptr (node->right = make_tree_node(MAXOP_LITERAL, text, NULL, NULL)) == NULL) { *parse_error = PARSE_SYNTAX_ERROR; - free_tree(tree); - if(ptr) + maxinfo_free_tree(tree); + if (ptr) { free(text); }