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.

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)
+		settings_kv_set(kv, strdupnull(value), this->contents);
-		{
-			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);
-		}
 	}
 	this->lock->unlock(this->lock);
 }

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)
+		settings_kv_set(found, kv->value, contents);
-		{
-			array_insert(contents, ARRAY_TAIL, found->value);
-		}
-		else
-		{
-			free(found->value);
-		}
-		found->value = kv->value;
 		kv->value = NULL;
 		settings_kv_destroy(kv, NULL);
 	}

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.
  *

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);
+	val1 = settings->get_str(settings, "main.key1", NULL);
-	ck_assert_str_eq("val1", val);
+	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 */
+	/* the pointer we got before is still valid */
-	ck_assert_str_eq("val", val);
+	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 */
+	/* the pointers we got before are still valid */
-	ck_assert_str_eq("val", val);
+	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");