From cb2e3b898e51eedaced6b4858d4edfe489db9420 Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Tue, 23 Feb 2016 21:35:45 +0200 Subject: [PATCH] Cleaned up qlafilter, topfilter and namedserverfilter instance creation The change fixes a crash when no filebase parameter is given for the filter. The instance creation functions of these filters had spossible memory leaks in case errors in the configuration occurred. In addition to these, they would be successfully created even if unexpected parameters were given. --- Documentation/Filters/Query-Log-All-Filter.md | 4 +- Documentation/Filters/Top-N-Filter.md | 4 +- server/modules/filter/namedserverfilter.c | 55 ++++++++++++------ server/modules/filter/qlafilter.c | 56 +++++++++++------- server/modules/filter/topfilter.c | 58 +++++++++++++------ 5 files changed, 115 insertions(+), 62 deletions(-) diff --git a/Documentation/Filters/Query-Log-All-Filter.md b/Documentation/Filters/Query-Log-All-Filter.md index 892f4ad4e..ec9d5639b 100644 --- a/Documentation/Filters/Query-Log-All-Filter.md +++ b/Documentation/Filters/Query-Log-All-Filter.md @@ -43,11 +43,11 @@ anymore and the `filebase` parameter should be used instead. ## Filter Parameters -The QLA filter accepts a number of optional parameters, these were introduced in the 1.0 release of MaxScale. +The QLA filter has one mandatory parameter, `filebase`, and a number of optional parameters. These were introduced in the 1.0 release of MaxScale. ### Filebase -The basename of the output file created for each session. A session index is added to the filename for each file written. +The basename of the output file created for each session. A session index is added to the filename for each file written. This is a mandatory parameter. ``` filebase=/tmp/SqlQueryLog diff --git a/Documentation/Filters/Top-N-Filter.md b/Documentation/Filters/Top-N-Filter.md index 5920d9c6d..978f23aba 100644 --- a/Documentation/Filters/Top-N-Filter.md +++ b/Documentation/Filters/Top-N-Filter.md @@ -40,11 +40,11 @@ options=case,extended ## Filter Parameters -The top filter accepts a number of optional parameters. +The top filter has one mandatory parameter, `filebase`, and a number of optional parameters. ### Filebase -The basename of the output file created for each session. A session index is added to the filename for each file written. +The basename of the output file created for each session. A session index is added to the filename for each file written. This is a mandatory parameter. ``` filebase=/tmp/SqlQueryLog diff --git a/server/modules/filter/namedserverfilter.c b/server/modules/filter/namedserverfilter.c index e5e30a4e1..4156e7857 100644 --- a/server/modules/filter/namedserverfilter.c +++ b/server/modules/filter/namedserverfilter.c @@ -85,7 +85,6 @@ typedef struct char *user; /* User name to restrict matches */ char *match; /* Regular expression to match */ char *server; /* Server to route to */ - int cflags; /* Regexec compile flags */ regex_t re; /* Compiled regex text */ } REGEXHINT_INSTANCE; @@ -147,14 +146,17 @@ static FILTER * createInstance(char **options, FILTER_PARAMETER **params) { REGEXHINT_INSTANCE *my_instance; - int i, cflags = REG_ICASE; + int cflags = REG_ICASE; - if ((my_instance = calloc(1, sizeof(REGEXHINT_INSTANCE))) != NULL) + if ((my_instance = malloc(sizeof(REGEXHINT_INSTANCE))) != NULL) { my_instance->match = NULL; my_instance->server = NULL; + my_instance->source = NULL; + my_instance->user = NULL; + bool error = false; - for (i = 0; params && params[i]; i++) + for (int i = 0; params && params[i]; i++) { if (!strcmp(params[i]->name, "match")) { @@ -176,12 +178,13 @@ createInstance(char **options, FILTER_PARAMETER **params) { MXS_ERROR("namedserverfilter: Unexpected parameter '%s'.", params[i]->name); + error = true; } } if (options) { - for (i = 0; options[i]; i++) + for (int i = 0; options[i]; i++) { if (!strcasecmp(options[i], "ignorecase")) { @@ -197,32 +200,48 @@ createInstance(char **options, FILTER_PARAMETER **params) } else { - MXS_ERROR("namedserverfilter: unsupported option '%s'.", + MXS_ERROR("namedserverfilter: Unsupported option '%s'.", options[i]); + error = true; } } } - my_instance->cflags = cflags; - if (my_instance->match == NULL || my_instance->server == NULL) + if (my_instance->match == NULL) { - MXS_ERROR("namedserverfilter: Missing required configured" - " option. You must specify a match and server " - "option as a minimum."); - free(my_instance); - return NULL; + MXS_ERROR("namedserverfilter: Missing required parameters 'match'."); + error = true; } - if (regcomp(&my_instance->re, my_instance->match, - my_instance->cflags)) + if (my_instance->server == NULL) + { + MXS_ERROR("namedserverfilter: Missing required parameters 'server'."); + error = true; + } + if (my_instance->server && my_instance->match && + regcomp(&my_instance->re, my_instance->match, cflags)) { MXS_ERROR("namedserverfilter: Invalid regular expression '%s'.\n", my_instance->match); free(my_instance->match); - free(my_instance->server); - free(my_instance); - return NULL; + my_instance->match = NULL; + error = true; } + + if (error) + { + if (my_instance->match) + { + regfree(&my_instance->re); + free(my_instance->match); + } + free(my_instance->server); + free(my_instance->source); + free(my_instance->user); + free(my_instance); + my_instance = NULL; + } + } return(FILTER *) my_instance; } diff --git a/server/modules/filter/qlafilter.c b/server/modules/filter/qlafilter.c index 241548959..9be4ba21a 100644 --- a/server/modules/filter/qlafilter.c +++ b/server/modules/filter/qlafilter.c @@ -175,12 +175,15 @@ createInstance(char **options, FILTER_PARAMETER **params) QLA_INSTANCE *my_instance; int i; - if ((my_instance = calloc(1, sizeof(QLA_INSTANCE))) != NULL) + if ((my_instance = malloc(sizeof(QLA_INSTANCE))) != NULL) { my_instance->source = NULL; my_instance->userName = NULL; my_instance->match = NULL; my_instance->nomatch = NULL; + my_instance->filebase = NULL; + bool error = false; + if (params) { for (i = 0; params[i]; i++) @@ -203,17 +206,13 @@ createInstance(char **options, FILTER_PARAMETER **params) } else if (!strcmp(params[i]->name, "filebase")) { - if (my_instance->filebase) - { - free(my_instance->filebase); - my_instance->filebase = NULL; - } my_instance->filebase = strdup(params[i]->value); } else if (!filter_standard_parameter(params[i]->name)) { MXS_ERROR("qlafilter: Unexpected parameter '%s'.", params[i]->name); + error = true; } } } @@ -238,46 +237,59 @@ createInstance(char **options, FILTER_PARAMETER **params) } else { - MXS_ERROR("qlafilter: unsupported option '%s'.", + MXS_ERROR("qlafilter: Unsupported option '%s'.", options[i]); + error = true; } } } + if (my_instance->filebase == NULL) + { + MXS_ERROR("qlafilter: No 'filebase' parameter defined."); + error = true; + } + my_instance->sessions = 0; if (my_instance->match && regcomp(&my_instance->re, my_instance->match, cflags)) { MXS_ERROR("qlafilter: Invalid regular expression '%s'" - " for the match parameter.\n", + " for the 'match' parameter.\n", my_instance->match); free(my_instance->match); - free(my_instance->source); - if (my_instance->filebase) - { - free(my_instance->filebase); - } - free(my_instance); - return NULL; + my_instance->match = NULL; + error = true; } if (my_instance->nomatch && regcomp(&my_instance->nore, my_instance->nomatch, cflags)) { MXS_ERROR("qlafilter: Invalid regular expression '%s'" - " for the nomatch paramter.", - my_instance->match); + " for the 'nomatch' parameter.", + my_instance->nomatch); + free(my_instance->nomatch); + my_instance->nomatch = NULL; + error = true; + } + + if (error) + { if (my_instance->match) { + free(my_instance->match); regfree(&my_instance->re); } - free(my_instance->match); - free(my_instance->source); - if (my_instance->filebase) + + if (my_instance->match) { - free(my_instance->filebase); + free(my_instance->match); + regfree(&my_instance->re); } + free(my_instance->filebase); + free(my_instance->source); + free(my_instance->userName); free(my_instance); - return NULL; + my_instance = NULL; } } return(FILTER *) my_instance; diff --git a/server/modules/filter/topfilter.c b/server/modules/filter/topfilter.c index 6b40713f0..87cff29dd 100644 --- a/server/modules/filter/topfilter.c +++ b/server/modules/filter/topfilter.c @@ -176,7 +176,6 @@ GetModuleObject() { return &MyObject; } - /** * Create an instance of the filter for a particular service * within MaxScale. @@ -192,14 +191,16 @@ createInstance(char **options, FILTER_PARAMETER **params) int i; TOPN_INSTANCE *my_instance; - if ((my_instance = calloc(1, sizeof(TOPN_INSTANCE))) != NULL) + if ((my_instance = malloc(sizeof(TOPN_INSTANCE))) != NULL) { my_instance->topN = 10; my_instance->match = NULL; my_instance->exclude = NULL; my_instance->source = NULL; my_instance->user = NULL; - my_instance->filebase = strdup("top"); + my_instance->filebase = NULL; + bool error = false; + for (i = 0; params && params[i]; i++) { if (!strcmp(params[i]->name, "count")) @@ -208,7 +209,6 @@ createInstance(char **options, FILTER_PARAMETER **params) } else if (!strcmp(params[i]->name, "filebase")) { - free(my_instance->filebase); my_instance->filebase = strdup(params[i]->value); } else if (!strcmp(params[i]->name, "match")) @@ -231,6 +231,7 @@ createInstance(char **options, FILTER_PARAMETER **params) { MXS_ERROR("topfilter: Unexpected parameter '%s'.", params[i]->name); + error = true; } } @@ -254,39 +255,60 @@ createInstance(char **options, FILTER_PARAMETER **params) } else { - MXS_ERROR("topfilter: unsupported option '%s'.", + MXS_ERROR("topfilter: Unsupported option '%s'.", options[i]); + error = true; } } } + if (my_instance->filebase == NULL) + { + MXS_ERROR("topfilter: No 'filebase' parameter defined."); + error = true; + } + my_instance->sessions = 0; if (my_instance->match && regcomp(&my_instance->re, my_instance->match, cflags)) { MXS_ERROR("topfilter: Invalid regular expression '%s'" - " for the match parameter.", + " for the 'match' parameter.", my_instance->match); + regfree(&my_instance->re); free(my_instance->match); - free(my_instance->source); - free(my_instance->user); - free(my_instance->filebase); - free(my_instance); - return NULL; + my_instance->match = NULL; + error = true; } if (my_instance->exclude && regcomp(&my_instance->exre, my_instance->exclude, cflags)) { - MXS_ERROR("qlafilter: Invalid regular expression '%s'" - " for the nomatch paramter.\n", - my_instance->match); - regfree(&my_instance->re); - free(my_instance->match); + MXS_ERROR("topfilter: Invalid regular expression '%s'" + " for the 'nomatch' parameter.\n", + my_instance->exclude); + regfree(&my_instance->exre); + free(my_instance->exclude); + my_instance->exclude = NULL; + error = true; + } + + if (error) + { + if (my_instance->exclude) + { + regfree(&my_instance->exre); + free(my_instance->exclude); + } + if (my_instance->match) + { + regfree(&my_instance->re); + free(my_instance->match); + } + free(my_instance->filebase); free(my_instance->source); free(my_instance->user); - free(my_instance->filebase); free(my_instance); - return NULL; + my_instance = NULL; } } return(FILTER *) my_instance;