settings: Don't overwrite values in-place

This is not thread safe.  If threads are reading from pointers to existing
values they could get a partially updated invalid value.

Refactored assignment to a separate function.

diff --git a/src/libstrongswan/settings/settings.c b/src/libstrongswan/settings/settings.c
index 3891578df14a86794109f1aa83d82ca6e0c90a7a..0acf9bf93293fa5db075387dbe10f31be4cf5e5f 100644 (file)
--- a/src/libstrongswan/settings/settings.c
+++ b/src/libstrongswan/settings/settings.c
@@ -474,26 +474,7 @@ static void set_value(private_settings_t *this, section_t *section,
                                                         TRUE);
        if (kv)
        {
-               if (!value)
-               {
-                       if (kv->value)
-                       {
-                               array_insert(this->contents, ARRAY_TAIL, kv->value);
-                       }
-                       kv->value = NULL;
-               }
-               else if (kv->value && (strlen(value) <= strlen(kv->value)))
-               {       /* overwrite in-place, if possible */
-                       strcpy(kv->value, value);
-               }
-               else
-               {       /* otherwise clone the string and cache the replaced one */
-                       if (kv->value)
-                       {
-                               array_insert(this->contents, ARRAY_TAIL, kv->value);
-                       }
-                       kv->value = strdup(value);
-               }
+               settings_kv_set(kv, strdupnull(value), this->contents);
        }
        this->lock->unlock(this->lock);
 }

diff --git a/src/libstrongswan/settings/settings_types.c b/src/libstrongswan/settings/settings_types.c
index 8171bbc3e105e1c98f6816c58bfcfc356c618e27..50359108fbc5dffb07289fd7e13e6bbea2e4139e 100644 (file)
--- a/src/libstrongswan/settings/settings_types.c
+++ b/src/libstrongswan/settings/settings_types.c
@@ -81,6 +81,31 @@ void settings_section_destroy(section_t *this, array_t *contents)
        free(this);
 }
 
+/*
+ * Described in header
+ */
+void settings_kv_set(kv_t *kv, char *value, array_t *contents)
+{
+       if (value && kv->value && streq(value, kv->value))
+       {       /* no update required */
+               free(value);
+               return;
+       }
+
+       /* if the new value was shorter we could overwrite the existing one but that
+        * could lead to reads of partially updated values from other threads that
+        * have a pointer to the existing value, so we replace it anyway */
+       if (kv->value && contents)
+       {
+               array_insert(contents, ARRAY_TAIL, kv->value);
+       }
+       else
+       {
+               free(kv->value);
+       }
+       kv->value = value;
+}
+
 /*
  * Described in header
  */
@@ -95,15 +120,7 @@ void settings_kv_add(section_t *section, kv_t *kv, array_t *contents)
        }
        else
        {
-               if (contents && found->value)
-               {
-                       array_insert(contents, ARRAY_TAIL, found->value);
-               }
-               else
-               {
-                       free(found->value);
-               }
-               found->value = kv->value;
+               settings_kv_set(found, kv->value, contents);
                kv->value = NULL;
                settings_kv_destroy(kv, NULL);
        }

diff --git a/src/libstrongswan/settings/settings_types.h b/src/libstrongswan/settings/settings_types.h
index 49f75dc16d1e3c1e1e7ec1a160b30b17b30f539d..5bc322f0a5852c0a2aaf3792ad81bec86937cd48 100644 (file)
--- a/src/libstrongswan/settings/settings_types.h
+++ b/src/libstrongswan/settings/settings_types.h
@@ -87,6 +87,15 @@ kv_t *settings_kv_create(char *key, char *value);
  */
 void settings_kv_destroy(kv_t *this, array_t *contents);
 
+/**
+ * Set the value of the given key/value pair.
+ *
+ * @param kv           key/value pair
+ * @param value                new value (gets adopted), may be NULL
+ * @param contents     optional array to store replaced values in
+ */
+void settings_kv_set(kv_t *kv, char *value, array_t *contents);
+
 /**
  * Add the given key/value pair to the given section.
  *

diff --git a/src/libstrongswan/tests/suites/test_settings.c b/src/libstrongswan/tests/suites/test_settings.c
index b5bd2c12ea5eba7e8684e6178b982db85ae1e4b2..b66ecd5e9bbc7b2d3706455771972a800be9c8b7 100644 (file)
--- a/src/libstrongswan/tests/suites/test_settings.c
+++ b/src/libstrongswan/tests/suites/test_settings.c
@@ -133,19 +133,28 @@ END_TEST
 
 START_TEST(test_set_str)
 {
-       char *val;
+       char *val1, *val2;
 
-       val = settings->get_str(settings, "main.key1", NULL);
-       ck_assert_str_eq("val1", val);
+       val1 = settings->get_str(settings, "main.key1", NULL);
+       ck_assert_str_eq("val1", val1);
        settings->set_str(settings, "main.key1", "val");
        verify_string("val", "main.key1");
-       /* since it is shorter the pointer we got above points to the new value */
-       ck_assert_str_eq("val", val);
+       /* the pointer we got before is still valid */
+       ck_assert_str_eq("val1", val1);
 
+       val2 = settings->get_str(settings, "main.key1", NULL);
+       ck_assert_str_eq("val", val2);
        settings->set_str(settings, "main.key1", "longer value");
        verify_string("longer value", "main.key1");
-       /* here it gets replaced but the pointer is still valid */
-       ck_assert_str_eq("val", val);
+       /* the pointers we got before are still valid */
+       ck_assert_str_eq("val1", val1);
+       ck_assert_str_eq("val", val2);
+
+       val1 = settings->get_str(settings, "main.key1", NULL);
+       settings->set_str(settings, "main.key1", "longer value");
+       val2 = settings->get_str(settings, "main.key1", NULL);
+       /* setting the same string should should get us the same pointer */
+       ck_assert(val1 == val2);
 
        settings->set_str(settings, "main", "main val");
        verify_string("main val", "main");