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 8d619116a7826d5a7c132998bc0c6faeafe6cab7..c1fc8f3976447b7506a181b367e75aae6c49fd17 100644 (file)
--- 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 ba2aff4607b25ca6decca866a5db3a514c85e868..0097c1d2ba3a7dbe766d906cc11b7c590d6337a7 100644 (file)
--- 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);