From: Tobias Brunner <tobias@strongswan.org>
Date: Fri, 29 Aug 2014 07:36:57 +0000 (+0200)
Subject: array: Fix array_remove_at() for non-pointer arrays
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7113c1426470b907b62c717fbe396af0d375da71;p=thirdparty%2Fstrongswan.git

array: Fix array_remove_at() for non-pointer arrays

Because array_remove_at() or rather the called array_remove() may move
elements over the element at the currently enumerated position, simply
returning a pointer to that position for non-pointer arrays could result
in the pointer passed to enumerate() pointing to the previous or next
array element after the array_remove_at() call.  The caller would thus
operate on the wrong element if the that pointer is accessed again before
calling enumerate().

This had an impact on auth_cfg_t where purge() may optionally keep CA
certificates (which is a feature used during multiple IKEv2 authentication
rounds), but due to this bug such certificates may still have been destroyed
while the pointer to them remained in the array, which then lead to
incorrect reference counts and eventually crashes.
---

diff --git a/src/libstrongswan/collections/array.c b/src/libstrongswan/collections/array.c
index 8d619116a7..c1fc8f3976 100644
--- a/src/libstrongswan/collections/array.c
+++ b/src/libstrongswan/collections/array.c
@@ -211,6 +211,8 @@ typedef struct {
 	array_t *array;
 	/** current index +1, initialized at 0 */
 	int idx;
+	/** current element for element based arrays */
+	void *cur;
 } array_enumerator_t;
 
 METHOD(enumerator_t, enumerate, bool,
@@ -227,8 +229,11 @@ METHOD(enumerator_t, enumerate, bool,
 		  get_size(this->array, this->idx + this->array->head);
 	if (this->array->esize)
 	{
-		/* for element based arrays we return a pointer to the element */
-		*out = pos;
+		/* for element based arrays we copy the element and return a pointer to
+		 * the copy. this is required in case remove_at is called, which might
+		 * overwrite the current element */
+		memcpy(this->cur, pos, this->array->esize);
+		*out = this->cur;
 	}
 	else
 	{
@@ -239,6 +244,13 @@ METHOD(enumerator_t, enumerate, bool,
 	return TRUE;
 }
 
+METHOD(enumerator_t, enumerator_destroy, void,
+	array_enumerator_t *this)
+{
+	free(this->cur);
+	free(this);
+}
+
 enumerator_t* array_create_enumerator(array_t *array)
 {
 	array_enumerator_t *enumerator;
@@ -251,10 +263,14 @@ enumerator_t* array_create_enumerator(array_t *array)
 	INIT(enumerator,
 		.public = {
 			.enumerate = (void*)_enumerate,
-			.destroy = (void*)free,
+			.destroy = _enumerator_destroy,
 		},
 		.array = array,
 	);
+	if (array->esize)
+	{
+		enumerator->cur = malloc(array->esize);
+	}
 	return &enumerator->public;
 }
 
diff --git a/src/libstrongswan/tests/suites/test_array.c b/src/libstrongswan/tests/suites/test_array.c
index ba2aff4607..0097c1d2ba 100644
--- a/src/libstrongswan/tests/suites/test_array.c
+++ b/src/libstrongswan/tests/suites/test_array.c
@@ -277,6 +277,78 @@ START_TEST(test_enumerate)
 }
 END_TEST
 
+static int remove_at_data[] = {0, 1, 2, 3, 4};
+
+START_TEST(test_remove_at_obj)
+{
+	array_t *array;
+	int i, *x;
+	enumerator_t *enumerator;
+
+	array = array_create(sizeof(remove_at_data[0]), 0);
+
+	array_insert(array, ARRAY_TAIL, &remove_at_data[0]);
+	array_insert(array, ARRAY_TAIL, &remove_at_data[1]);
+	array_insert(array, ARRAY_TAIL, &remove_at_data[2]);
+	array_insert(array, ARRAY_TAIL, &remove_at_data[3]);
+	array_insert(array, ARRAY_TAIL, &remove_at_data[4]);
+
+	i = 0;
+	enumerator = array_create_enumerator(array);
+	while (enumerator->enumerate(enumerator, &x))
+	{
+		if (i >= _i)
+		{
+			ck_assert_int_eq(*x, remove_at_data[i]);
+			array_remove_at(array, enumerator);
+			ck_assert_int_eq(array_count(array),
+							 countof(remove_at_data) - i - 1 + _i);
+			ck_assert_int_eq(*x, remove_at_data[i]);
+		}
+		i++;
+	}
+	enumerator->destroy(enumerator);
+	ck_assert_int_eq(array_count(array), _i);
+
+	array_destroy(array);
+}
+END_TEST
+
+START_TEST(test_remove_at_ptr)
+{
+	array_t *array;
+	int i, *x;
+	enumerator_t *enumerator;
+
+	array = array_create(0, 0);
+
+	array_insert(array, ARRAY_TAIL, &remove_at_data[0]);
+	array_insert(array, ARRAY_TAIL, &remove_at_data[1]);
+	array_insert(array, ARRAY_TAIL, &remove_at_data[2]);
+	array_insert(array, ARRAY_TAIL, &remove_at_data[3]);
+	array_insert(array, ARRAY_TAIL, &remove_at_data[4]);
+
+	i = 0;
+	enumerator = array_create_enumerator(array);
+	while (enumerator->enumerate(enumerator, &x))
+	{
+		if (i >= _i)
+		{
+			ck_assert(x == &remove_at_data[i]);
+			array_remove_at(array, enumerator);
+			ck_assert_int_eq(array_count(array),
+							 countof(remove_at_data) - i - 1 + _i);
+			ck_assert(x == &remove_at_data[i]);
+		}
+		i++;
+	}
+	enumerator->destroy(enumerator);
+	ck_assert_int_eq(array_count(array), _i);
+
+	array_destroy(array);
+}
+END_TEST
+
 static int comp_obj(const void *a, const void *b, void *arg)
 {
 	ck_assert_str_eq(arg, "arg");
@@ -510,6 +582,11 @@ Suite *array_suite_create()
 	tcase_add_test(tc, test_enumerate);
 	suite_add_tcase(s, tc);
 
+	tc = tcase_create("remove_at");
+	tcase_add_loop_test(tc, test_remove_at_obj, 0, countof(remove_at_data));
+	tcase_add_loop_test(tc, test_remove_at_ptr, 0, countof(remove_at_data));
+	suite_add_tcase(s, tc);
+
 	tc = tcase_create("sort");
 	tcase_add_test(tc, test_sort_obj);
 	tcase_add_test(tc, test_sort_ptr);