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.
This commit is contained in:
parent
ca3c7b7ea6
commit
089d5f9765
|
@ -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,
|
static section_t *ensure_section(private_settings_t *this, section_t *section,
|
||||||
const char *key, va_list args)
|
const char *key, va_list args)
|
||||||
{
|
{
|
||||||
char buf[128], keybuf[512];
|
char buf[128], keybuf[512];
|
||||||
section_t *found;
|
|
||||||
|
|
||||||
if (snprintf(keybuf, sizeof(keybuf), "%s", key) >= sizeof(keybuf))
|
if (snprintf(keybuf, sizeof(keybuf), "%s", key) >= sizeof(keybuf))
|
||||||
{
|
{
|
||||||
return NULL;
|
return NULL;
|
||||||
}
|
}
|
||||||
/* we might have to change the tree */
|
return find_section_buffered(section, keybuf, keybuf, args, buf,
|
||||||
this->lock->write_lock(this->lock);
|
sizeof(buf), TRUE);
|
||||||
found = find_section_buffered(section, keybuf, keybuf, args, buf,
|
|
||||||
sizeof(buf), TRUE);
|
|
||||||
this->lock->unlock(this->lock);
|
|
||||||
return found;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
@ -944,37 +939,32 @@ METHOD(settings_t, add_fallback, void,
|
||||||
va_list args;
|
va_list args;
|
||||||
char buf[512];
|
char buf[512];
|
||||||
|
|
||||||
/* find/create the section */
|
this->lock->write_lock(this->lock);
|
||||||
va_start(args, fallback);
|
va_start(args, fallback);
|
||||||
section = ensure_section(this, this->top, key, args);
|
section = ensure_section(this, this->top, key, args);
|
||||||
va_end(args);
|
va_end(args);
|
||||||
|
|
||||||
va_start(args, fallback);
|
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);
|
settings_reference_add(section, strdup(buf), TRUE);
|
||||||
this->lock->unlock(this->lock);
|
|
||||||
}
|
}
|
||||||
va_end(args);
|
va_end(args);
|
||||||
|
this->lock->unlock(this->lock);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Load settings from files matching the given file pattern or from a string.
|
* 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.
|
* 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,
|
static section_t *load_internal(char *pattern, bool string)
|
||||||
char *pattern, bool merge, bool string)
|
|
||||||
{
|
{
|
||||||
section_t *section;
|
section_t *section;
|
||||||
bool loaded;
|
bool loaded;
|
||||||
|
|
||||||
if (pattern == NULL || !pattern[0])
|
if (pattern == NULL || !pattern[0])
|
||||||
{ /* TODO: Clear parent if merge is FALSE? */
|
{
|
||||||
return TRUE;
|
return settings_section_create(NULL);
|
||||||
}
|
}
|
||||||
|
|
||||||
section = 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)
|
if (!loaded)
|
||||||
{
|
{
|
||||||
settings_section_destroy(section, NULL);
|
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);
|
this->lock->unlock(this->lock);
|
||||||
|
|
||||||
settings_section_destroy(section, NULL);
|
settings_section_destroy(section, NULL);
|
||||||
return TRUE;
|
return parent != NULL;
|
||||||
}
|
}
|
||||||
|
|
||||||
METHOD(settings_t, load_files, bool,
|
METHOD(settings_t, load_files, bool,
|
||||||
private_settings_t *this, char *pattern, bool merge)
|
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,
|
METHOD(settings_t, load_files_section, bool,
|
||||||
private_settings_t *this, char *pattern, bool merge, char *key, ...)
|
private_settings_t *this, char *pattern, bool merge, char *key, ...)
|
||||||
{
|
{
|
||||||
section_t *section;
|
section_t *section, *parent;
|
||||||
va_list args;
|
va_list args;
|
||||||
|
|
||||||
va_start(args, key);
|
section = load_internal(pattern, FALSE);
|
||||||
section = ensure_section(this, this->top, key, args);
|
|
||||||
va_end(args);
|
|
||||||
|
|
||||||
if (!section)
|
if (!section)
|
||||||
{
|
{
|
||||||
return FALSE;
|
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,
|
METHOD(settings_t, load_string, bool,
|
||||||
private_settings_t *this, char *settings, bool merge)
|
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,
|
METHOD(settings_t, load_string_section, bool,
|
||||||
private_settings_t *this, char *settings, bool merge, char *key, ...)
|
private_settings_t *this, char *settings, bool merge, char *key, ...)
|
||||||
{
|
{
|
||||||
section_t *section;
|
section_t *section, *parent;
|
||||||
va_list args;
|
va_list args;
|
||||||
|
|
||||||
va_start(args, key);
|
section = load_internal(settings, TRUE);
|
||||||
section = ensure_section(this, this->top, key, args);
|
|
||||||
va_end(args);
|
|
||||||
|
|
||||||
if (!section)
|
if (!section)
|
||||||
{
|
{
|
||||||
return FALSE;
|
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,
|
METHOD(settings_t, destroy, void,
|
||||||
|
|
|
@ -1,5 +1,5 @@
|
||||||
/*
|
/*
|
||||||
* Copyright (C) 2014 Tobias Brunner
|
* Copyright (C) 2014-2018 Tobias Brunner
|
||||||
* HSR Hochschule fuer Technik Rapperswil
|
* HSR Hochschule fuer Technik Rapperswil
|
||||||
*
|
*
|
||||||
* This program is free software; you can redistribute it and/or modify it
|
* 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);
|
enumerator = settings->create_section_enumerator(settings, parent);
|
||||||
ver = verifier->create_enumerator(verifier);
|
ver = verifier->create_enumerator(verifier);
|
||||||
while (enumerator->enumerate(enumerator, §ion) &&
|
while (enumerator->enumerate(enumerator, §ion))
|
||||||
ver->enumerate(ver, ¤t))
|
|
||||||
{
|
{
|
||||||
|
ck_assert_msg(ver->enumerate(ver, ¤t),
|
||||||
|
"no more sections expected, found %s", section);
|
||||||
ck_assert_str_eq(section, current);
|
ck_assert_str_eq(section, current);
|
||||||
verifier->remove_at(verifier, ver);
|
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);
|
enumerator = settings->create_key_value_enumerator(settings, parent);
|
||||||
enum_keys = keys->create_enumerator(keys);
|
enum_keys = keys->create_enumerator(keys);
|
||||||
enum_values = values->create_enumerator(values);
|
enum_values = values->create_enumerator(values);
|
||||||
while (enumerator->enumerate(enumerator, &key, &value) &&
|
while (enumerator->enumerate(enumerator, &key, &value))
|
||||||
enum_keys->enumerate(enum_keys, ¤t_key) &&
|
|
||||||
enum_values->enumerate(enum_values, ¤t_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_key, key);
|
||||||
ck_assert_str_eq(current_value, value);
|
ck_assert_str_eq(current_value, value);
|
||||||
keys->remove_at(keys, enum_keys);
|
keys->remove_at(keys, enum_keys);
|
||||||
|
@ -519,8 +521,8 @@ START_TEST(test_key_value_enumerator)
|
||||||
{
|
{
|
||||||
linked_list_t *keys, *values;
|
linked_list_t *keys, *values;
|
||||||
|
|
||||||
keys = linked_list_create_with_items("key1", "key2", "empty", "key3", 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", 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");
|
verify_key_values(keys, values, "main");
|
||||||
|
|
||||||
keys = linked_list_create_with_items("key", "key2", "subsub", NULL);
|
keys = linked_list_create_with_items("key", "key2", "subsub", NULL);
|
||||||
|
@ -894,7 +896,6 @@ START_TEST(test_load_string)
|
||||||
}
|
}
|
||||||
END_TEST
|
END_TEST
|
||||||
|
|
||||||
|
|
||||||
START_TEST(test_load_string_section)
|
START_TEST(test_load_string_section)
|
||||||
{
|
{
|
||||||
char *content =
|
char *content =
|
||||||
|
@ -914,13 +915,6 @@ START_TEST(test_load_string_section)
|
||||||
ck_assert(settings->load_string_section(settings, include_content2, TRUE, "main.sub1"));
|
ck_assert(settings->load_string_section(settings, include_content2, TRUE, "main.sub1"));
|
||||||
verify_include();
|
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"));
|
ck_assert(settings->load_string_section(settings, include_content2, FALSE, "main"));
|
||||||
verify_null("main.key1");
|
verify_null("main.key1");
|
||||||
verify_string("v2", "main.key2");
|
verify_string("v2", "main.key2");
|
||||||
|
@ -934,6 +928,56 @@ START_TEST(test_load_string_section)
|
||||||
}
|
}
|
||||||
END_TEST
|
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)
|
START_SETUP(setup_fallback_config)
|
||||||
{
|
{
|
||||||
create_settings(chunk_from_str(
|
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_checked_fixture(tc, setup_include_config, teardown_config);
|
||||||
tcase_add_test(tc, test_load_string);
|
tcase_add_test(tc, test_load_string);
|
||||||
tcase_add_test(tc, test_load_string_section);
|
tcase_add_test(tc, test_load_string_section);
|
||||||
|
tcase_add_test(tc, test_load_string_section_null);
|
||||||
suite_add_tcase(s, tc);
|
suite_add_tcase(s, tc);
|
||||||
|
|
||||||
tc = tcase_create("fallback");
|
tc = tcase_create("fallback");
|
||||||
|
|
Loading…
Reference in New Issue