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.

diff --git a/src/libstrongswan/settings/settings.c b/src/libstrongswan/settings/settings.c
index 4d1f743449da29300f4e9bd5dac68ab0930012e5..1224c74176e6f69da05a62e91e166deda383c9e6 100644 (file)
--- 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 39c59bbf2005b674287c65d82feeb3572cc76233..6259c9ed08129c2016581d0205fcb5d37e4f8756 100644 (file)
--- 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, &section) &&
-                  ver->enumerate(ver, &current))
+       while (enumerator->enumerate(enumerator, &section))
        {
+               ck_assert_msg(ver->enumerate(ver, &current),
+                                         "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, &current_key) &&
-                  enum_values->enumerate(enum_values, &current_value))
+       while (enumerator->enumerate(enumerator, &key, &value))
        {
+               ck_assert_msg(enum_keys->enumerate(enum_keys, &current_key),
+                                         "no more key/value expected, found %s = %s", key, value);
+               ck_assert(enum_values->enumerate(enum_values, &current_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");