From 089d5f9765c88c95e588737ff902a59e3f186d3b Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 22 May 2018 10:51:50 +0200 Subject: [PATCH] settings: Properly lock when extending sections or adding fallbacks There was a potential chance for a race condition if the ensured section was purged for some reason before using it later. This also changes the behavior for NULL/empty strings via load_string* with merge == FALSE, which now purges the config/section. --- src/libstrongswan/settings/settings.c | 106 +++++++++++------- .../tests/suites/test_settings.c | 77 ++++++++++--- 2 files changed, 129 insertions(+), 54 deletions(-) diff --git a/src/libstrongswan/settings/settings.c b/src/libstrongswan/settings/settings.c index 4d1f74344..1224c7417 100644 --- a/src/libstrongswan/settings/settings.c +++ b/src/libstrongswan/settings/settings.c @@ -281,24 +281,19 @@ static void find_sections_buffered(private_settings_t *this, section_t *section, } /** - * Ensure that the section with the given key exists (thread-safe). + * Ensure that the section with the given key exists (not thread-safe). */ static section_t *ensure_section(private_settings_t *this, section_t *section, const char *key, va_list args) { char buf[128], keybuf[512]; - section_t *found; if (snprintf(keybuf, sizeof(keybuf), "%s", key) >= sizeof(keybuf)) { return NULL; } - /* we might have to change the tree */ - this->lock->write_lock(this->lock); - found = find_section_buffered(section, keybuf, keybuf, args, buf, - sizeof(buf), TRUE); - this->lock->unlock(this->lock); - return found; + return find_section_buffered(section, keybuf, keybuf, args, buf, + sizeof(buf), TRUE); } /** @@ -944,37 +939,32 @@ METHOD(settings_t, add_fallback, void, va_list args; char buf[512]; - /* find/create the section */ + this->lock->write_lock(this->lock); va_start(args, fallback); section = ensure_section(this, this->top, key, args); va_end(args); va_start(args, fallback); - if (vsnprintf(buf, sizeof(buf), fallback, args) < sizeof(buf)) + if (section && vsnprintf(buf, sizeof(buf), fallback, args) < sizeof(buf)) { - this->lock->write_lock(this->lock); settings_reference_add(section, strdup(buf), TRUE); - this->lock->unlock(this->lock); } va_end(args); + this->lock->unlock(this->lock); } /** * Load settings from files matching the given file pattern or from a string. - * All sections and values are added relative to "parent". * All files (even included ones) have to be loaded successfully. - * If merge is FALSE the contents of parent are replaced with the parsed - * contents, otherwise they are merged together. */ -static bool load_internal(private_settings_t *this, section_t *parent, - char *pattern, bool merge, bool string) +static section_t *load_internal(char *pattern, bool string) { section_t *section; bool loaded; if (pattern == NULL || !pattern[0]) - { /* TODO: Clear parent if merge is FALSE? */ - return TRUE; + { + return settings_section_create(NULL); } section = settings_section_create(NULL); @@ -983,61 +973,101 @@ static bool load_internal(private_settings_t *this, section_t *parent, if (!loaded) { settings_section_destroy(section, NULL); - return FALSE; + section = NULL; } + return section; +} - this->lock->write_lock(this->lock); - settings_section_extend(parent, section, this->contents, !merge); +/** + * Add sections and values in "section" relative to "parent". + * If merge is FALSE the contents of parent are replaced with the parsed + * contents, otherwise they are merged together. + * + * Releases the write lock and destroys the given section. + * If parent is NULL this is all that happens. + */ +static bool extend_section(private_settings_t *this, section_t *parent, + section_t *section, bool merge) +{ + if (parent) + { + settings_section_extend(parent, section, this->contents, !merge); + } this->lock->unlock(this->lock); - settings_section_destroy(section, NULL); - return TRUE; + return parent != NULL; } METHOD(settings_t, load_files, bool, private_settings_t *this, char *pattern, bool merge) { - return load_internal(this, this->top, pattern, merge, FALSE); + section_t *section; + + section = load_internal(pattern, FALSE); + if (!section) + { + return FALSE; + } + + this->lock->write_lock(this->lock); + return extend_section(this, this->top, section, merge); } METHOD(settings_t, load_files_section, bool, private_settings_t *this, char *pattern, bool merge, char *key, ...) { - section_t *section; + section_t *section, *parent; va_list args; - va_start(args, key); - section = ensure_section(this, this->top, key, args); - va_end(args); - + section = load_internal(pattern, FALSE); if (!section) { return FALSE; } - return load_internal(this, section, pattern, merge, FALSE); + + this->lock->write_lock(this->lock); + + va_start(args, key); + parent = ensure_section(this, this->top, key, args); + va_end(args); + + return extend_section(this, parent, section, merge); } METHOD(settings_t, load_string, bool, private_settings_t *this, char *settings, bool merge) { - return load_internal(this, this->top, settings, merge, TRUE); + section_t *section; + + section = load_internal(settings, TRUE); + if (!section) + { + return FALSE; + } + + this->lock->write_lock(this->lock); + return extend_section(this, this->top, section, merge); } METHOD(settings_t, load_string_section, bool, private_settings_t *this, char *settings, bool merge, char *key, ...) { - section_t *section; + section_t *section, *parent; va_list args; - va_start(args, key); - section = ensure_section(this, this->top, key, args); - va_end(args); - + section = load_internal(settings, TRUE); if (!section) { return FALSE; } - return load_internal(this, section, settings, merge, TRUE); + + this->lock->write_lock(this->lock); + + va_start(args, key); + parent = ensure_section(this, this->top, key, args); + va_end(args); + + return extend_section(this, parent, section, merge); } METHOD(settings_t, destroy, void, diff --git a/src/libstrongswan/tests/suites/test_settings.c b/src/libstrongswan/tests/suites/test_settings.c index 39c59bbf2..6259c9ed0 100644 --- a/src/libstrongswan/tests/suites/test_settings.c +++ b/src/libstrongswan/tests/suites/test_settings.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2014 Tobias Brunner + * Copyright (C) 2014-2018 Tobias Brunner * HSR Hochschule fuer Technik Rapperswil * * This program is free software; you can redistribute it and/or modify it @@ -452,9 +452,10 @@ static void verify_sections(linked_list_t *verifier, char *parent) enumerator = settings->create_section_enumerator(settings, parent); ver = verifier->create_enumerator(verifier); - while (enumerator->enumerate(enumerator, §ion) && - ver->enumerate(ver, ¤t)) + while (enumerator->enumerate(enumerator, §ion)) { + ck_assert_msg(ver->enumerate(ver, ¤t), + "no more sections expected, found %s", section); ck_assert_str_eq(section, current); verifier->remove_at(verifier, ver); } @@ -498,10 +499,11 @@ static void verify_key_values(linked_list_t *keys, linked_list_t *values, enumerator = settings->create_key_value_enumerator(settings, parent); enum_keys = keys->create_enumerator(keys); enum_values = values->create_enumerator(values); - while (enumerator->enumerate(enumerator, &key, &value) && - enum_keys->enumerate(enum_keys, ¤t_key) && - enum_values->enumerate(enum_values, ¤t_value)) + while (enumerator->enumerate(enumerator, &key, &value)) { + ck_assert_msg(enum_keys->enumerate(enum_keys, ¤t_key), + "no more key/value expected, found %s = %s", key, value); + ck_assert(enum_values->enumerate(enum_values, ¤t_value)); ck_assert_str_eq(current_key, key); ck_assert_str_eq(current_value, value); keys->remove_at(keys, enum_keys); @@ -519,8 +521,8 @@ START_TEST(test_key_value_enumerator) { linked_list_t *keys, *values; - keys = linked_list_create_with_items("key1", "key2", "empty", "key3", NULL); - values = linked_list_create_with_items("val1", "with space", "", "string with\nnewline", NULL); + keys = linked_list_create_with_items("key1", "key2", "empty", "key3", "key4", "key5", NULL); + values = linked_list_create_with_items("val1", "with space", "", "string with\nnewline", "multi line\nstring", "escaped newline", NULL); verify_key_values(keys, values, "main"); keys = linked_list_create_with_items("key", "key2", "subsub", NULL); @@ -894,7 +896,6 @@ START_TEST(test_load_string) } END_TEST - START_TEST(test_load_string_section) { char *content = @@ -914,13 +915,6 @@ START_TEST(test_load_string_section) ck_assert(settings->load_string_section(settings, include_content2, TRUE, "main.sub1")); verify_include(); - /* invalid strings are a failure */ - ck_assert(!settings->load_string_section(settings, "conf {", TRUE, "")); - /* NULL or empty strings are OK though */ - ck_assert(settings->load_string_section(settings, "", TRUE, "")); - ck_assert(settings->load_string_section(settings, NULL, TRUE, "")); - verify_include(); - ck_assert(settings->load_string_section(settings, include_content2, FALSE, "main")); verify_null("main.key1"); verify_string("v2", "main.key2"); @@ -934,6 +928,56 @@ START_TEST(test_load_string_section) } END_TEST +START_TEST(test_load_string_section_null) +{ + linked_list_t *keys, *values; + + char *content = + "main {\n" + " key1 = val1\n" + " key2 = val2\n" + " none = x\n" + " sub1 {\n" + " include = value\n" + " key2 = value2\n" + " }\n" + "}"; + + settings = settings_create_string(content); + + ck_assert(settings->load_string_section(settings, include_content1, TRUE, "")); + ck_assert(settings->load_string_section(settings, include_content2, TRUE, "main.sub1")); + verify_include(); + + /* invalid strings are a failure */ + ck_assert(!settings->load_string_section(settings, "conf {", TRUE, "")); + /* NULL or empty strings are OK though when merging */ + ck_assert(settings->load_string_section(settings, "", TRUE, "")); + ck_assert(settings->load_string_section(settings, NULL, TRUE, "")); + verify_include(); + + /* they do purge the settings if merge is not TRUE */ + ck_assert(settings->load_string_section(settings, "", FALSE, "main")); + verify_null("main.key1"); + verify_null("main.sub1.key2"); + + keys = linked_list_create_with_items(NULL); + verify_sections(keys, "main"); + + keys = linked_list_create_with_items(NULL); + values = linked_list_create_with_items(NULL); + verify_key_values(keys, values, "main"); + + keys = linked_list_create_with_items("main", NULL); + verify_sections(keys, ""); + + ck_assert(settings->load_string_section(settings, NULL, FALSE, "")); + + keys = linked_list_create_with_items(NULL); + verify_sections(keys, ""); +} +END_TEST + START_SETUP(setup_fallback_config) { create_settings(chunk_from_str( @@ -1664,6 +1708,7 @@ Suite *settings_suite_create() tcase_add_checked_fixture(tc, setup_include_config, teardown_config); tcase_add_test(tc, test_load_string); tcase_add_test(tc, test_load_string_section); + tcase_add_test(tc, test_load_string_section_null); suite_add_tcase(s, tc); tc = tcase_create("fallback");