From 2ce8ff9357033e6457be8cb65ea36a88bd48a0f1 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 24 Feb 2025 18:59:05 +0100 Subject: [PATCH 1/4] Log incorrect env vars Signed-off-by: DL6ER --- src/config/env.c | 69 ++++++++++++++++++++---------------------------- 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/src/config/env.c b/src/config/env.c index 043b1f315..1d61668ff 100644 --- a/src/config/env.c +++ b/src/config/env.c @@ -23,6 +23,8 @@ #include // openFTLtoml() #include "config/toml_helper.h" +// escape_json() +#include "webserver/http-common.h" struct env_item { bool used; @@ -174,6 +176,25 @@ void freeEnvVars(void) } } +/** + * @brief Marks an environment item as invalid and logs a warning message. + * + * @param envvar The value of the environment variable. + * @param conf_item A pointer to the configuration item structure. + * @param item A pointer to the environment item structure to be marked as invalid. + */ +static void invalid_item(const char *envvar, struct conf_item *conf_item, struct env_item *item) +{ + item->error = "not an allowed option"; + item->allowed = conf_item->h; + item->valid = false; + + char *escaped_value = escape_json(envvar); + log_warn("ENV %s = \"%s\" is %s, allowed options are: %s", + conf_item->e, escaped_value, item->error, item->allowed); + free(escaped_value); +} + bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, struct config *newconf, cJSON *forced_vars, bool *reset) { // First check if a environmental variable with the given key exists by @@ -386,11 +407,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not an allowed option"; - item->allowed = conf_item->h; - log_warn("ENV %s is %s, allowed options are: %s", - conf_item->e, item->error, item->allowed); - item->valid = false; + invalid_item(envvar, conf_item, item); } break; } @@ -405,11 +422,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - item->error = "not an allowed option"; - item->allowed = conf_item->h; - log_warn("ENV %s is %s, allowed options are: %s", - conf_item->e, item->error, item->allowed); - item->valid = false; + invalid_item(envvar, conf_item, item); } break; } @@ -424,11 +437,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - item->error = "not an allowed option"; - item->allowed = conf_item->h; - log_warn("ENV %s is %s, allowed options are: %s", - conf_item->e, item->error, item->allowed); - item->valid = false; + invalid_item(envvar, conf_item, item); } break; } @@ -443,11 +452,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - item->error = "not an allowed option"; - item->allowed = conf_item->h; - log_warn("ENV %s is %s, allowed options are: %s", - conf_item->e, item->error, item->allowed); - item->valid = false; + invalid_item(envvar, conf_item, item); } break; } @@ -462,11 +467,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - item->error = "not an allowed option"; - item->allowed = conf_item->h; - log_warn("ENV %s is %s, allowed options are: %s", - conf_item->e, item->error, item->allowed); - item->valid = false; + invalid_item(envvar, conf_item, item); } break; } @@ -481,11 +482,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - item->error = "not an allowed option"; - item->allowed = conf_item->h; - log_warn("ENV %s is %s, allowed options are: %s", - conf_item->e, item->error, item->allowed); - item->valid = false; + invalid_item(envvar, conf_item, item); } break; } @@ -500,11 +497,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - item->error = "not an allowed option"; - item->allowed = conf_item->h; - log_warn("ENV %s is %s, allowed options are: %s", - conf_item->e, item->error, item->allowed); - item->valid = false; + invalid_item(envvar, conf_item, item); } break; } @@ -519,11 +512,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - item->error = "not an allowed option"; - item->allowed = conf_item->h; - log_warn("ENV %s is %s, allowed options are: %s", - conf_item->e, item->error, item->allowed); - item->valid = false; + invalid_item(envvar, conf_item, item); } break; } From 494d27ada54b1584b61d2be5a762d55c86cfb241 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 24 Feb 2025 19:09:11 +0100 Subject: [PATCH 2/4] Explicitly log allowed items Signed-off-by: DL6ER --- src/config/env.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/config/env.c b/src/config/env.c index 1d61668ff..67d8ec648 100644 --- a/src/config/env.c +++ b/src/config/env.c @@ -183,16 +183,26 @@ void freeEnvVars(void) * @param conf_item A pointer to the configuration item structure. * @param item A pointer to the environment item structure to be marked as invalid. */ -static void invalid_item(const char *envvar, struct conf_item *conf_item, struct env_item *item) +static void invalid_enum_item(const char *envvar, struct conf_item *conf_item, struct env_item *item) { item->error = "not an allowed option"; item->allowed = conf_item->h; item->valid = false; + cJSON *allowed_items = cJSON_CreateArray(); + cJSON *it = NULL; + cJSON_ArrayForEach(it, conf_item->a) + { + cJSON *sub_item = cJSON_GetObjectItem(it, "item"); + cJSON_AddItemToArray(allowed_items, cJSON_Duplicate(sub_item, true)); + } + char *allowed_values = cJSON_PrintUnformatted(allowed_items); char *escaped_value = escape_json(envvar); - log_warn("ENV %s = \"%s\" is %s, allowed options are: %s", - conf_item->e, escaped_value, item->error, item->allowed); + + log_warn("ENV %s = %s is %s, allowed options are: %s", + conf_item->e, escaped_value, item->error, allowed_values); free(escaped_value); + free(allowed_values); } bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, struct config *newconf, cJSON *forced_vars, bool *reset) @@ -407,7 +417,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - invalid_item(envvar, conf_item, item); + invalid_enum_item(envvar, conf_item, item); } break; } @@ -422,7 +432,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - invalid_item(envvar, conf_item, item); + invalid_enum_item(envvar, conf_item, item); } break; } @@ -437,7 +447,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - invalid_item(envvar, conf_item, item); + invalid_enum_item(envvar, conf_item, item); } break; } @@ -452,7 +462,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - invalid_item(envvar, conf_item, item); + invalid_enum_item(envvar, conf_item, item); } break; } @@ -467,7 +477,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - invalid_item(envvar, conf_item, item); + invalid_enum_item(envvar, conf_item, item); } break; } @@ -482,7 +492,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - invalid_item(envvar, conf_item, item); + invalid_enum_item(envvar, conf_item, item); } break; } @@ -497,7 +507,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - invalid_item(envvar, conf_item, item); + invalid_enum_item(envvar, conf_item, item); } break; } @@ -512,7 +522,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s else { - invalid_item(envvar, conf_item, item); + invalid_enum_item(envvar, conf_item, item); } break; } From 9857a18940bf5942ebab63645e309580e70e80af Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 24 Feb 2025 20:59:42 +0100 Subject: [PATCH 3/4] Ensure warnings/errors about invalid env vars are only printed once Signed-off-by: DL6ER --- src/config/env.c | 64 ++++++++++++++++++-------------------------- test/test_suite.bats | 4 +-- 2 files changed, 28 insertions(+), 40 deletions(-) diff --git a/src/config/env.c b/src/config/env.c index 67d8ec648..3ff6bddd6 100644 --- a/src/config/env.c +++ b/src/config/env.c @@ -27,12 +27,12 @@ #include "webserver/http-common.h" struct env_item { - bool used; - bool valid; + bool used :1; + bool valid :1; + bool error_allocated :1; char *key; char *value; - const char *error; - const char *allowed; + char *error; struct env_item *next; }; @@ -79,7 +79,6 @@ void getEnvVars(void) new_item->key = strdup(key); new_item->value = strdup(value); new_item->error = NULL; - new_item->allowed = NULL; new_item->next = env_list; env_list = new_item; @@ -122,15 +121,15 @@ void printFTLenv(void) log_info(" %s %s is used", cli_tick(), item->key); else { - if(item->error != NULL && item->allowed == NULL) - log_err(" %s %s is invalid (%s)", - cli_cross(), item->key, item->error); - else if(item->error != NULL && item->allowed != NULL) - log_err(" %s %s is invalid (%s, allowed options are: %s)", - cli_cross(), item->key, item->error, item->allowed); + if(item->error != NULL) + log_err(" %s %s %s", + cli_cross(), item->key, item->error); else log_err(" %s %s is invalid", cli_cross(), item->key); + + if(item->error_allocated) + free(item->error); } continue; @@ -185,8 +184,6 @@ void freeEnvVars(void) */ static void invalid_enum_item(const char *envvar, struct conf_item *conf_item, struct env_item *item) { - item->error = "not an allowed option"; - item->allowed = conf_item->h; item->valid = false; cJSON *allowed_items = cJSON_CreateArray(); @@ -199,8 +196,10 @@ static void invalid_enum_item(const char *envvar, struct conf_item *conf_item, s char *allowed_values = cJSON_PrintUnformatted(allowed_items); char *escaped_value = escape_json(envvar); - log_warn("ENV %s = %s is %s, allowed options are: %s", - conf_item->e, escaped_value, item->error, allowed_values); + // Calculate the size of the error message + asprintf(&item->error, "= %s is %s, allowed options are: %s", + escaped_value, item->error, allowed_values); + free(escaped_value); free(allowed_values); } @@ -275,8 +274,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type bool"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not a boolean"; item->valid = false; } break; @@ -295,8 +293,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type bool"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not a boolean"; item->valid = false; } break; @@ -311,8 +308,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type integer"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not an integer"; item->valid = false; } break; @@ -327,8 +323,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type unsigned integer"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not an unsigned integer"; item->valid = false; } break; @@ -343,8 +338,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type unsigned integer (16 bit"; - log_warn("ENV %s is %s)", conf_item->e, item->error); + item->error = (char *)"is not an unsigned integer (16 bit)"; item->valid = false; } break; @@ -359,8 +353,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type long"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not a long integer"; item->valid = false; } break; @@ -375,8 +368,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type unsigned long"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not an unsigned long integer"; item->valid = false; } break; @@ -391,8 +383,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type double"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not a double"; item->valid = false; } break; @@ -536,8 +527,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type integer or outside allowed bounds"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not an integer or outside allowed bounds"; item->valid = false; } break; @@ -557,8 +547,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type IPv4 address"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not an IPv4 address"; item->valid = false; } break; @@ -578,8 +567,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s } else { - item->error = "not of type IPv6 address"; - log_warn("ENV %s is %s", conf_item->e, item->error); + item->error = (char *)"is not an IPv6 address"; item->valid = false; } break; @@ -616,7 +604,7 @@ bool __attribute__((nonnull(1,2,3))) readEnvValue(struct conf_item *conf_item, s { if(!set_and_check_password(conf_item, envvar)) { - log_warn("ENV %s is invalid", conf_item->e); + item->error = (char *)"is not a valid password"; item->valid = false; break; } diff --git a/test/test_suite.bats b/test/test_suite.bats index e06281bc0..4c50ef050 100644 --- a/test/test_suite.bats +++ b/test/test_suite.bats @@ -756,7 +756,7 @@ @test "No ERROR messages in FTL.log (besides known/intended error)" { run bash -c 'grep "ERROR: " /var/log/pihole/FTL.log' printf "%s\n" "${lines[@]}" - run bash -c 'grep "ERROR: " /var/log/pihole/FTL.log | grep -c -v -E "(index\.html)|(Failed to create shared memory object)|(FTLCONF_debug_api is invalid)|(Failed to set|adjust time during NTP sync: Insufficient permissions)"' + run bash -c 'grep "ERROR: " /var/log/pihole/FTL.log | grep -c -v -E "(index\.html)|(Failed to create shared memory object)|(FTLCONF_debug_api is not a boolean)|(Failed to set|adjust time during NTP sync: Insufficient permissions)"' printf "count: %s\n" "${lines[@]}" [[ ${lines[0]} == "0" ]] } @@ -1642,7 +1642,7 @@ } @test "Invalid environmental variable is logged" { - run bash -c 'grep -q "FTLCONF_debug_api is invalid" /var/log/pihole/FTL.log' + run bash -c 'grep -q "FTLCONF_debug_api is not a boolean" /var/log/pihole/FTL.log' printf "%s\n" "${lines[@]}" [[ $status == 0 ]] } From ab80387b7f441063f330c65e2f1bcd58054be592 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Mon, 24 Feb 2025 21:54:33 +0100 Subject: [PATCH 4/4] Avoid (null) as explanation Signed-off-by: DL6ER --- src/config/env.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config/env.c b/src/config/env.c index 3ff6bddd6..098fa88a1 100644 --- a/src/config/env.c +++ b/src/config/env.c @@ -197,8 +197,8 @@ static void invalid_enum_item(const char *envvar, struct conf_item *conf_item, s char *escaped_value = escape_json(envvar); // Calculate the size of the error message - asprintf(&item->error, "= %s is %s, allowed options are: %s", - escaped_value, item->error, allowed_values); + asprintf(&item->error, "= %s is invalid, allowed options are: %s", + escaped_value, allowed_values); free(escaped_value); free(allowed_values);