From bb00bc002816dae3e8fcdd9df82e3fac2cc7b076 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Wed, 8 Nov 2023 08:28:54 -0500 Subject: [PATCH] memmove for overlaping memory (#434) (#437) * memmove for overlaping memory * Add test for memcpy to memmove Signed-off-by: Tyler Weaver (cherry picked from commit 5b99aad816d5e49db22c29e11588fafa4e7365f7) Co-authored-by: Tyler Weaver --- src/array_list.c | 6 +++++- test/test_array_list.cpp | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/array_list.c b/src/array_list.c index 22dc8188..a9a08db8 100644 --- a/src/array_list.c +++ b/src/array_list.c @@ -172,7 +172,11 @@ rcutils_array_list_remove(rcutils_array_list_t * array_list, size_t index) if (copy_count > 0) { uint8_t * dst_ptr = rcutils_array_list_get_pointer_for_index(array_list, index); uint8_t * src_ptr = rcutils_array_list_get_pointer_for_index(array_list, index + 1); - memcpy(dst_ptr, src_ptr, array_list->impl->data_size * copy_count); + // If the size of the list is >1 the regions of memory overlap. + // POSIX and C standards are explicit that employing memcpy() with overlapping + // areas produces undefined behavior. The recomendation is to use memmove. + // Reference: https://man7.org/linux/man-pages/man3/memcpy.3.html + memmove(dst_ptr, src_ptr, array_list->impl->data_size * copy_count); } array_list->impl->size--; diff --git a/test/test_array_list.cpp b/test/test_array_list.cpp index 6ed9d951..731514f0 100644 --- a/test/test_array_list.cpp +++ b/test/test_array_list.cpp @@ -248,6 +248,30 @@ TEST_F(ArrayListPreInitTest, remove_success_removes_from_list) { EXPECT_EQ(size, (size_t)0); } +TEST_F(ArrayListPreInitTest, remove_success_removes_from_list_with_multiple_items) { + uint32_t data = 22; + size_t index = 0; + size_t size = 0; + rcutils_ret_t ret = RCUTILS_RET_OK; + + // Add a few things first so we know the index isn't out of bounds + for (size_t i = 0; i < 3; ++i) { + ret = rcutils_array_list_add(&list, &data); + ASSERT_EQ(RCUTILS_RET_OK, ret) << rcutils_get_error_string().str; + } + + ret = rcutils_array_list_get_size(&list, &size); + ASSERT_EQ(RCUTILS_RET_OK, ret) << rcutils_get_error_string().str; + EXPECT_EQ(size, (size_t)3); + + ret = rcutils_array_list_remove(&list, index); + ASSERT_EQ(RCUTILS_RET_OK, ret) << rcutils_get_error_string().str; + + ret = rcutils_array_list_get_size(&list, &size); + ASSERT_EQ(RCUTILS_RET_OK, ret) << rcutils_get_error_string().str; + EXPECT_EQ(size, (size_t)2); +} + TEST_F(ArrayListTest, get_list_null_fails) { size_t index = 0; uint32_t data = 0;