From: Tobias Brunner <tobias@strongswan.org>
Date: Tue, 11 Mar 2014 10:08:15 +0000 (+0100)
Subject: settings: Don't overwrite values in-place
X-Git-Tag: 5.2.0dr4~1^2~14
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2fbbea55c592c8b3eb9f40295620455434d7f651;p=thirdparty%2Fstrongswan.git

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 3891578df1..0acf9bf932 100644
--- 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 8171bbc3e1..50359108fb 100644
--- 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 49f75dc16d..5bc322f0a5 100644
--- 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 b5bd2c12ea..b66ecd5e9b 100644
--- 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");