From d877b9a635562a3405e5ce1e89b11efaf99af1b1 Mon Sep 17 00:00:00 2001 From: Soner Tari Date: Wed, 22 Sep 2021 15:27:45 +0300 Subject: [PATCH] Fix and improve get_name_value() and unit tests Fix possible segfault if name has leading white space Pass the name param to get_name_value() as char *, so it cannot be modified ever Improve unit tests for get_name_value and proxyspec_parse --- src/opts.c | 43 ++++++--- src/opts.h | 2 +- tests/check/opts.t.c | 214 +++++++++++++++++-------------------------- 3 files changed, 115 insertions(+), 144 deletions(-) diff --git a/src/opts.c b/src/opts.c index 424f4fd..5b47cdc 100644 --- a/src/opts.c +++ b/src/opts.c @@ -4515,32 +4515,36 @@ set_proxyspec_option(proxyspec_t *spec, const char *argv0, /* * Separator param is needed for command line options only. - * Conf file option separator is ' '. - * Do not modify the name param, the caller may try to free it causing signal 6 crash. + * Conf file option separator is ' ', on the command line is '='. * Allows multiple separators between name and value. */ int -get_name_value(char **name, char **value, const char sep, int line_num) +get_name_value(char *name, char **value, const char sep, int line_num) { - size_t len = strlen(*name); + size_t len = strlen(name); // Find end of name and null-terminate - char *n = *name; - while (*n != ' ' && *n != '\t' && *n != '\r' && *n != '\n' && *n != sep) + char *n = name; + while (*n != '\0' && *n != ' ' && *n != '\t' && *n != '\r' && *n != '\n' && *n != sep) n++; *n = '\0'; - if (!*name) { + size_t name_len = strlen(name); + + if (!name_len) { fprintf(stderr, "Error in option: No option name on line %d\n", line_num); + // Return empty value + *value = name; return -1; } - size_t name_len = strlen(*name); if (len == name_len) { - fprintf(stderr, "Error in option: No option value on line %d\n", line_num); +#ifdef DEBUG_OPTS + log_dbg_printf("Warning in option: No option separator on line %d\n", line_num); +#endif /* DEBUG_OPTS */ // Return empty value - *value = *name + name_len; - return -1; + *value = name + name_len; + return 0; } // Trim left of value (skip white space and sep until value) @@ -4549,8 +4553,17 @@ get_name_value(char **name, char **value, const char sep, int line_num) *value = n; + size_t value_len = strlen(*value); + + if (!value_len) { +#ifdef DEBUG_OPTS + log_dbg_printf("Warning in option: No option value on line %d\n", line_num); +#endif /* DEBUG_OPTS */ + return 0; + } + // Trim right of value - n = *value + strlen(*value) - 1; + n = *value + value_len - 1; while (*n == ' ' || *n == '\t' || *n == '\r' || *n == '\n' || *n == sep) n--; *(n + 1) = '\0'; @@ -4631,7 +4644,7 @@ load_proxyspec_struct(global_t *global, const char *argv0, char **natengine, int continue; } - retval = get_name_value(&name, &value, ' ', *line_num); + retval = get_name_value(name, &value, ' ', *line_num); if (retval == 0) { retval = set_proxyspec_option(spec, argv0, name, value, natengine, spec_addrs, *line_num); } @@ -4871,7 +4884,7 @@ global_set_option(global_t *global, const char *argv0, const char *optarg, for (name = line; *name == ' ' || *name == '\t'; name++); /* Command line option separator is '=' */ - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); if (retval == 0) { /* Line number param is for conf file, pass 0 for command line options */ int line_num = 0; @@ -4923,7 +4936,7 @@ opts_load_conffile(global_t *global, const char *argv0, char *conffile, char **n continue; } - retval = get_name_value(&name, &value, ' ', line_num); + retval = get_name_value(name, &value, ' ', line_num); if (retval == 0) { retval = set_global_option(global, argv0, name, value, natengine, &line_num, f, tmp_global_opts); } diff --git a/src/opts.h b/src/opts.h index 8fb4e2f..3702f68 100644 --- a/src/opts.h +++ b/src/opts.h @@ -457,7 +457,7 @@ int global_set_debug_level(const char *) NONNULL(1) WUNRES; void global_set_statslog(global_t *) NONNULL(1); int is_yesno(const char *) WUNRES; -int get_name_value(char **, char **, const char, int) WUNRES; +int get_name_value(char *, char **, const char, int) WUNRES; int global_set_option(global_t *, const char *, const char *, char **, tmp_global_opts_t *) NONNULL(1,2,3,5) WUNRES; int global_set_leafkey(global_t *, const char *, const char *) NONNULL(1,2,3) WUNRES; int global_set_leafcertdir(global_t *, const char *, const char *) NONNULL(1,2,3) WUNRES; diff --git a/tests/check/opts.t.c b/tests/check/opts.t.c index 018fb3e..2b14b15 100644 --- a/tests/check/opts.t.c +++ b/tests/check/opts.t.c @@ -163,34 +163,42 @@ START_TEST(proxyspec_parse_03) tmp_global_opts_t *tmp_global_opts = malloc(sizeof(tmp_global_opts_t)); memset(tmp_global_opts, 0, sizeof(tmp_global_opts_t)); + // Disable error messages close(2); + int rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); - global_free(global); - tmp_global_opts_free(tmp_global_opts); - if (rv == -1) - exit(EXIT_FAILURE); -} -END_TEST + fail_unless(rv == -1, "failed to reject spec"); -START_TEST(proxyspec_parse_04) -{ - global_t *global = global_new(); - int argc = 5; - char **argv = argv01; + argc = 5; + rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); + fail_unless(rv == -1, "failed to reject spec"); - tmp_global_opts_t *tmp_global_opts = malloc(sizeof(tmp_global_opts_t)); - memset(tmp_global_opts, 0, sizeof(tmp_global_opts_t)); + argc = 5; + argv = argv07; + rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); + fail_unless(rv == -1, "failed to reject spec"); + + argc = 5; + argv = argv06; + rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); + fail_unless(rv == -1, "failed to reject spec"); + + argc = 5; + argv = argv08; + rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); + fail_unless(rv == -1, "failed to reject spec"); + + argc = 6; + argv = argv13; + rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); + fail_unless(rv == -1, "failed to reject spec"); - close(2); - int rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); global_free(global); tmp_global_opts_free(tmp_global_opts); - if (rv == -1) - exit(EXIT_FAILURE); } END_TEST -START_TEST(proxyspec_parse_05) +START_TEST(proxyspec_parse_04) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -220,7 +228,7 @@ START_TEST(proxyspec_parse_05) } END_TEST -START_TEST(proxyspec_parse_06) +START_TEST(proxyspec_parse_05) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -250,7 +258,7 @@ START_TEST(proxyspec_parse_06) } END_TEST -START_TEST(proxyspec_parse_07) +START_TEST(proxyspec_parse_06) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -280,7 +288,7 @@ START_TEST(proxyspec_parse_07) } END_TEST -START_TEST(proxyspec_parse_08) +START_TEST(proxyspec_parse_07) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -309,43 +317,7 @@ START_TEST(proxyspec_parse_08) } END_TEST -START_TEST(proxyspec_parse_09) -{ - global_t *global = global_new(); - int argc = 5; - char **argv = argv07; - - tmp_global_opts_t *tmp_global_opts = malloc(sizeof(tmp_global_opts_t)); - memset(tmp_global_opts, 0, sizeof(tmp_global_opts_t)); - - close(2); - int rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); - global_free(global); - tmp_global_opts_free(tmp_global_opts); - if (rv == -1) - exit(EXIT_FAILURE); -} -END_TEST - -START_TEST(proxyspec_parse_10) -{ - global_t *global = global_new(); - int argc = 5; - char **argv = argv06; - - tmp_global_opts_t *tmp_global_opts = malloc(sizeof(tmp_global_opts_t)); - memset(tmp_global_opts, 0, sizeof(tmp_global_opts_t)); - - close(2); - int rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); - global_free(global); - tmp_global_opts_free(tmp_global_opts); - if (rv == -1) - exit(EXIT_FAILURE); -} -END_TEST - -START_TEST(proxyspec_parse_11) +START_TEST(proxyspec_parse_08) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -375,26 +347,8 @@ START_TEST(proxyspec_parse_11) } END_TEST -START_TEST(proxyspec_parse_12) -{ - global_t *global = global_new(); - int argc = 5; - char **argv = argv08; - - tmp_global_opts_t *tmp_global_opts = malloc(sizeof(tmp_global_opts_t)); - memset(tmp_global_opts, 0, sizeof(tmp_global_opts_t)); - - close(2); - int rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); - global_free(global); - tmp_global_opts_free(tmp_global_opts); - if (rv == -1) - exit(EXIT_FAILURE); -} -END_TEST - #ifndef TRAVIS -START_TEST(proxyspec_parse_13) +START_TEST(proxyspec_parse_09) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -435,7 +389,7 @@ START_TEST(proxyspec_parse_13) } END_TEST -START_TEST(proxyspec_parse_14) +START_TEST(proxyspec_parse_10) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -478,7 +432,7 @@ START_TEST(proxyspec_parse_14) END_TEST #endif /* !TRAVIS */ -START_TEST(proxyspec_parse_15) +START_TEST(proxyspec_parse_11) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -507,7 +461,7 @@ START_TEST(proxyspec_parse_15) } END_TEST -START_TEST(proxyspec_parse_16) +START_TEST(proxyspec_parse_12) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -548,25 +502,7 @@ START_TEST(proxyspec_parse_16) } END_TEST -START_TEST(proxyspec_parse_17) -{ - global_t *global = global_new(); - int argc = 6; - char **argv = argv13; - - tmp_global_opts_t *tmp_global_opts = malloc(sizeof(tmp_global_opts_t)); - memset(tmp_global_opts, 0, sizeof(tmp_global_opts_t)); - - close(2); - int rv = proxyspec_parse(&argc, &argv, NATENGINE, global, "sslproxy", tmp_global_opts); - global_free(global); - tmp_global_opts_free(tmp_global_opts); - if (rv == -1) - exit(EXIT_FAILURE); -} -END_TEST - -START_TEST(proxyspec_parse_18) +START_TEST(proxyspec_parse_13) { global_t *global = global_new(); proxyspec_t *spec = NULL; @@ -913,100 +849,127 @@ START_TEST(opts_get_name_value_01) char *value; name = strdup("Name Value"); - retval = get_name_value(&name, &value, ' ', 0); + retval = get_name_value(name, &value, ' ', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, "Value"), "failed parsing value"); fail_unless(retval == 0, "failed parsing name value"); free(name); name = strdup("Name Value"); - retval = get_name_value(&name, &value, ' ', 0); + retval = get_name_value(name, &value, ' ', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, "Value"), "failed parsing value"); fail_unless(retval == 0, "failed parsing name value"); free(name); - // Leading white space must be handled by the caller, - // so we don't have a test for " Name Value", or similar + // Leading white space must be handled by the caller + // We cannot modify the name pointer, so we return -1 + // So we don't actually need a test for " Name Value", or similar + name = strdup(" Name Value"); + retval = get_name_value(name, &value, ' ', 0); + fail_unless(!strcmp(name, ""), "failed parsing name"); + fail_unless(!strcmp(value, ""), "failed parsing value"); + fail_unless(retval == -1, "failed rejecting leading white space, empty name"); + free(name); name = strdup("Name Value "); - retval = get_name_value(&name, &value, ' ', 0); + retval = get_name_value(name, &value, ' ', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, "Value"), "failed parsing value"); fail_unless(retval == 0, "failed parsing name value"); free(name); name = strdup("Name=Value"); - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, "Value"), "failed parsing value"); fail_unless(retval == 0, "failed parsing name value"); free(name); + // Leading white space must be handled by the caller + // We cannot modify the name pointer, so we return -1 + // So we don't actually need a test for " Name Value", or similar + name = strdup(" Name=Value"); + retval = get_name_value(name, &value, ' ', 0); + fail_unless(!strcmp(name, ""), "failed parsing name"); + fail_unless(!strcmp(value, ""), "failed parsing value"); + fail_unless(retval == -1, "failed rejecting leading white space, empty name"); + free(name); + name = strdup("Name=Value "); - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, "Value"), "failed parsing value"); fail_unless(retval == 0, "failed parsing name value"); free(name); name = strdup("Name = Value"); - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, "Value"), "failed parsing value"); fail_unless(retval == 0, "failed parsing name value"); free(name); name = strdup("Name = Value "); - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, "Value"), "failed parsing value"); fail_unless(retval == 0, "failed parsing name value"); free(name); + // Name without value, e.g. '}' char is used for marking the end of structured proxyspecs + // so do not reject any form of just name, return success name = strdup("Name"); - retval = get_name_value(&name, &value, ' ', 0); + retval = get_name_value(name, &value, ' ', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); - fail_unless(retval == -1, "failed rejecting just name"); + fail_unless(!strcmp(value, ""), "failed parsing value"); + fail_unless(retval == 0, "failed parsing just name"); free(name); name = strdup("Name "); - retval = get_name_value(&name, &value, ' ', 0); + retval = get_name_value(name, &value, ' ', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, ""), "failed parsing value"); fail_unless(retval == 0, "failed parsing name empty value"); free(name); name = strdup("Name "); - retval = get_name_value(&name, &value, ' ', 0); + retval = get_name_value(name, &value, ' ', 0); + fail_unless(!strcmp(name, "Name"), "failed parsing name"); + fail_unless(!strcmp(value, ""), "failed parsing value"); + fail_unless(retval == 0, "failed parsing name empty value"); + free(name); + + name = strdup("Name"); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, ""), "failed parsing value"); fail_unless(retval == 0, "failed parsing name empty value"); free(name); name = strdup("Name="); - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, ""), "failed parsing value"); fail_unless(retval == 0, "failed parsing name empty value"); free(name); name = strdup("Name= "); - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, ""), "failed parsing value"); fail_unless(retval == 0, "failed parsing name empty value"); free(name); name = strdup("Name ="); - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, ""), "failed parsing value"); fail_unless(retval == 0, "failed parsing name empty value"); free(name); name = strdup("Name = "); - retval = get_name_value(&name, &value, '=', 0); + retval = get_name_value(name, &value, '=', 0); fail_unless(!strcmp(name, "Name"), "failed parsing name"); fail_unless(!strcmp(value, ""), "failed parsing value"); fail_unless(retval == 0, "failed parsing name empty value"); @@ -1026,24 +989,19 @@ opts_suite(void) #ifndef TRAVIS tcase_add_test(tc, proxyspec_parse_02); /* IPv6 */ #endif /* !TRAVIS */ - tcase_add_exit_test(tc, proxyspec_parse_03, EXIT_FAILURE); - tcase_add_exit_test(tc, proxyspec_parse_04, EXIT_FAILURE); + tcase_add_test(tc, proxyspec_parse_03); + tcase_add_test(tc, proxyspec_parse_04); tcase_add_test(tc, proxyspec_parse_05); tcase_add_test(tc, proxyspec_parse_06); tcase_add_test(tc, proxyspec_parse_07); tcase_add_test(tc, proxyspec_parse_08); - tcase_add_exit_test(tc, proxyspec_parse_09, EXIT_FAILURE); - tcase_add_exit_test(tc, proxyspec_parse_10, EXIT_FAILURE); - tcase_add_test(tc, proxyspec_parse_11); - tcase_add_exit_test(tc, proxyspec_parse_12, EXIT_FAILURE); #ifndef TRAVIS - tcase_add_test(tc, proxyspec_parse_13); /* IPv6 */ - tcase_add_test(tc, proxyspec_parse_14); /* IPv6 */ + tcase_add_test(tc, proxyspec_parse_09); /* IPv6 */ + tcase_add_test(tc, proxyspec_parse_10); /* IPv6 */ #endif /* !TRAVIS */ - tcase_add_test(tc, proxyspec_parse_15); - tcase_add_test(tc, proxyspec_parse_16); - tcase_add_exit_test(tc, proxyspec_parse_17, EXIT_FAILURE); - tcase_add_test(tc, proxyspec_parse_18); + tcase_add_test(tc, proxyspec_parse_11); + tcase_add_test(tc, proxyspec_parse_12); + tcase_add_test(tc, proxyspec_parse_13); tcase_add_test(tc, proxyspec_set_proto_01); suite_add_tcase(s, tc);