From 5aa5624551f47a8dcfec1b684de7de721ea54a70 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Wed, 8 Feb 2023 11:16:03 +0000 Subject: [PATCH 1/2] Add memory allocation checking to the list module The list module lacks memory allocation checking, and consequently can coredump on list_create() or list_add_item(). --- common/list.c | 176 ++++++++++++++++++++++++++++++++++---------------- common/list.h | 47 +++++++++++++- 2 files changed, 166 insertions(+), 57 deletions(-) diff --git a/common/list.c b/common/list.c index b4530e8e..83fde45a 100644 --- a/common/list.c +++ b/common/list.c @@ -22,23 +22,53 @@ #include #endif +#include + #include "arch.h" #include "os_calls.h" #include "string_calls.h" #include "list.h" #include "log.h" +enum +{ + DEFAULT_LIST_SIZE = 10, + DEFAULT_GROW_BY_SIZE = 10 +}; + +/*****************************************************************************/ +struct list * +list_create_sized(unsigned int size) +{ + struct list *self; + + if (size < DEFAULT_LIST_SIZE) + { + size = DEFAULT_LIST_SIZE; + } + self = (struct list *)calloc(sizeof(struct list), 1); + if (self != NULL) + { + self->items = (tbus *)malloc(sizeof(tbus) * size); + if (self->items == NULL) + { + free(self); + self = NULL; + } + else + { + self->grow_by = DEFAULT_GROW_BY_SIZE; + self->alloc_size = size; + } + } + return self; +} + /*****************************************************************************/ struct list * list_create(void) { - struct list *self; - - self = (struct list *)g_malloc(sizeof(struct list), 1); - self->grow_by = 10; - self->alloc_size = 10; - self->items = (tbus *)g_malloc(sizeof(tbus) * 10, 1); - return self; + return list_create_sized(DEFAULT_LIST_SIZE); } /*****************************************************************************/ @@ -56,34 +86,46 @@ list_delete(struct list *self) { for (i = 0; i < self->count; i++) { - g_free((void *)self->items[i]); + free((void *)self->items[i]); self->items[i] = 0; } } - g_free(self->items); - g_free(self); + free(self->items); + free(self); } /*****************************************************************************/ -void +static int +grow_list(struct list *self) +{ + int rv = 1; + unsigned int new_alloc_size = self->alloc_size + self->grow_by; + tbus *p = (tbus *)realloc(self->items, sizeof(tbus) * new_alloc_size); + if (p == NULL) + { + rv = 0; + } + else + { + self->alloc_size = new_alloc_size; + self->items = p; + } + return rv; +} + +/*****************************************************************************/ +int list_add_item(struct list *self, tbus item) { - tbus *p; - int i; - - if (self->count >= self->alloc_size) + if (self->count == self->alloc_size && !grow_list(self)) { - i = self->alloc_size; - self->alloc_size += self->grow_by; - p = (tbus *)g_malloc(sizeof(tbus) * self->alloc_size, 1); - g_memcpy(p, self->items, sizeof(tbus) * i); - g_free(self->items); - self->items = p; + return 0; } self->items[self->count] = item; self->count++; + return 1; } /*****************************************************************************/ @@ -108,16 +150,15 @@ list_clear(struct list *self) { for (i = 0; i < self->count; i++) { - g_free((void *)self->items[i]); + free((void *)self->items[i]); self->items[i] = 0; } } - g_free(self->items); self->count = 0; - self->grow_by = 10; - self->alloc_size = 10; - self->items = (tbus *)g_malloc(sizeof(tbus) * 10, 1); + self->grow_by = DEFAULT_GROW_BY_SIZE; + self->alloc_size = DEFAULT_LIST_SIZE; + self->items = (tbus *)realloc(self->items, sizeof(tbus) * self->alloc_size); } /*****************************************************************************/ @@ -147,7 +188,7 @@ list_remove_item(struct list *self, int index) { if (self->auto_free) { - g_free((void *)self->items[index]); + free((void *)self->items[index]); self->items[index] = 0; } @@ -161,57 +202,84 @@ list_remove_item(struct list *self, int index) } /*****************************************************************************/ -void +int list_insert_item(struct list *self, int index, tbus item) { - tbus *p; - int i; - if (index == self->count) + if (index > self->count) { - list_add_item(self, item); - return; + index = self->count; + } + else if (index < 0) + { + index = 0; } - if (index >= 0 && index < self->count) + if (self->count == self->alloc_size && !grow_list(self)) { - self->count++; + return 0; + } - if (self->count > self->alloc_size) - { - i = self->alloc_size; - self->alloc_size += self->grow_by; - p = (tbus *)g_malloc(sizeof(tbus) * self->alloc_size, 1); - g_memcpy(p, self->items, sizeof(tbus) * i); - g_free(self->items); - self->items = p; - } - - for (i = (self->count - 2); i >= index; i--) + self->count++; + if (self->count >= 2) + { + unsigned int i; + for (i = (self->count - 2); i >= (unsigned int)index; i--) { self->items[i + 1] = self->items[i]; } - - self->items[index] = item; } + + self->items[index] = item; + return 1; } /*****************************************************************************/ /* append one list to another using strdup for each item in the list */ /* begins copy at start_index, a zero based index on the source list */ -void +int list_append_list_strdup(struct list *self, struct list *dest, int start_index) { int index; - tbus item; - char *dup; + int rv = 1; + int entry_dest_count = dest->count; for (index = start_index; index < self->count; index++) { - item = list_get_item(self, index); - dup = g_strdup((char *)item); - list_add_item(dest, (tbus)dup); + const char *item = (const char *)list_get_item(self, index); + char *dup; + if (item == NULL) + { + // This shouldn't really happen, but if it does we'll + // copy the item anyway. + dup = NULL; + } + else + { + dup = g_strdup(item); + if (dup == NULL) + { + rv = 0; + break; + } + } + if (!list_add_item(dest, (tbus)dup)) + { + rv = 0; + break; + } } + + if (rv == 0) + { + // Remove the additional items we added + while (dest->count > entry_dest_count) + { + list_remove_item(dest, dest->count - 1); + } + } + + return rv; } /*****************************************************************************/ diff --git a/common/list.h b/common/list.h index 1f562105..7536a171 100644 --- a/common/list.h +++ b/common/list.h @@ -35,9 +35,27 @@ struct list struct list * list_create(void); +/** + * Creates a list with at least the specified number of items + * reserved + * @param size Number of items to reserve + * @return list, or NULL if no memory + */ +struct list * +list_create_sized(unsigned int size); void list_delete(struct list *self); -void +/** + * Adds an item to a list + * @param self The list + * @param item The item to add + * @result 0 if a memory allocation failure occurred. In this + * case the item is not added + * + * Memory allocation failures will not occur if the list is + * sized appropriately when created. + */ +int list_add_item(struct list *self, tintptr item); tintptr list_get_item(const struct list *self, int index); @@ -47,9 +65,32 @@ int list_index_of(struct list *self, tintptr item); void list_remove_item(struct list *self, int index); -void +/** + * Inserts an item into a list + * @param self The list + * @param index The location to insert the item before + * @param item The item to add + * @result 0 if a memory allocation failure occurred. In this + * case the item is not added + * + * Memory allocation failures will not occur if the list is + * sized appropriately when created. + */ +int list_insert_item(struct list *self, int index, tintptr item); -void +/** + * Adds strings to a list from another list + * @param self The source list + * @param dest Destination list + * @param start_index Index to start on the source list (zero based) + * + * @result 0 if a memory allocation failure occurred. In this + * case the destination list is unaltered. + * + * Strings from the source list are copied with strdup() + * The dest list should have auto_free set, or memory leaks will occur + */ +int list_append_list_strdup(struct list *self, struct list *dest, int start_index); void list_dump_items(struct list *self); From 994418cd90693a1df28338324e8ffb5b471c3f91 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 9 Feb 2023 16:16:04 +0000 Subject: [PATCH 2/2] Add simple list test cases --- tests/common/Makefile.am | 1 + tests/common/test_common.h | 1 + tests/common/test_common_main.c | 4 +- tests/common/test_list_calls.c | 132 ++++++++++++++++++++++++++++++++ 4 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 tests/common/test_list_calls.c diff --git a/tests/common/Makefile.am b/tests/common/Makefile.am index f7dbbe28..c9a1fed5 100644 --- a/tests/common/Makefile.am +++ b/tests/common/Makefile.am @@ -14,6 +14,7 @@ check_PROGRAMS = test_common test_common_SOURCES = \ test_common.h \ test_common_main.c \ + test_list_calls.c \ test_string_calls.c \ test_os_calls.c \ test_ssl_calls.c \ diff --git a/tests/common/test_common.h b/tests/common/test_common.h index 1878bafb..9aead07c 100644 --- a/tests/common/test_common.h +++ b/tests/common/test_common.h @@ -7,6 +7,7 @@ char * bin_to_hex(const char *input, int length); +Suite *make_suite_test_list(void); Suite *make_suite_test_string(void); Suite *make_suite_test_os_calls(void); Suite *make_suite_test_ssl_calls(void); diff --git a/tests/common/test_common_main.c b/tests/common/test_common_main.c index 4ad60f34..68a3ba16 100644 --- a/tests/common/test_common_main.c +++ b/tests/common/test_common_main.c @@ -46,12 +46,12 @@ int main (void) int number_failed; SRunner *sr; - sr = srunner_create (make_suite_test_string()); + sr = srunner_create (make_suite_test_list()); + srunner_add_suite(sr, make_suite_test_string()); srunner_add_suite(sr, make_suite_test_os_calls()); srunner_add_suite(sr, make_suite_test_ssl_calls()); srunner_add_suite(sr, make_suite_test_base64()); srunner_add_suite(sr, make_suite_test_guid()); - // srunner_add_suite(sr, make_list_suite()); srunner_set_tap(sr, "-"); /* diff --git a/tests/common/test_list_calls.c b/tests/common/test_list_calls.c new file mode 100644 index 00000000..af6f41c2 --- /dev/null +++ b/tests/common/test_list_calls.c @@ -0,0 +1,132 @@ + +#if defined(HAVE_CONFIG_H) +#include "config_ac.h" +#endif + +#include "list.h" + +#include "test_common.h" +#include "os_calls.h" +#include "string_calls.h" + +#define TEST_LIST_SIZE 1000 + +START_TEST(test_list__simple) +{ + struct list *lst = list_create(); + int i; + int val; + for (i = 0 ; i < TEST_LIST_SIZE ; ++i) + { + list_add_item(lst, i); + } + + ck_assert_int_eq(lst->count, TEST_LIST_SIZE); + for (i = 0 ; i < TEST_LIST_SIZE ; ++i) + { + ck_assert_int_eq(lst->items[i], i); + // Also check get method + val = list_get_item(lst, i); + ck_assert_int_eq(val, i); + } + + i = list_index_of(lst, 50); + ck_assert_int_eq(i, 50); + + list_remove_item(lst, 10); + ck_assert_int_eq(lst->count, TEST_LIST_SIZE - 1); + val = list_get_item(lst, 10); + ck_assert_int_eq(val, 11); + + list_insert_item(lst, 10, 10); + ck_assert_int_eq(lst->count, TEST_LIST_SIZE); + val = list_get_item(lst, 10); + ck_assert_int_eq(val, 10); + + list_clear(lst); + ck_assert_int_eq(lst->count, 0); + list_delete(lst); +} +END_TEST + +// This needs to be run through valgrind to truly check all memory is +// being de-allocated +START_TEST(test_list__simple_auto_free) +{ + struct list *lst = list_create(); + lst->auto_free = 1; + + int i; + for (i = 0 ; i < TEST_LIST_SIZE ; ++i) + { + char strval[64]; + g_snprintf(strval, sizeof(strval), "%d", i); + list_add_item(lst, (tintptr)g_strdup(strval)); + } + + list_remove_item(lst, 0); + list_remove_item(lst, 0); + list_remove_item(lst, 0); + list_remove_item(lst, 0); + list_remove_item(lst, 0); + ck_assert_int_eq(lst->count, TEST_LIST_SIZE - 5); + list_delete(lst); +} +END_TEST + +START_TEST(test_list__simple_append_list) +{ + int i; + struct list *src = list_create(); + struct list *dst = list_create(); + src->auto_free = 0; + dst->auto_free = 1; + + list_add_item(src, (tintptr)"6"); + list_add_item(src, (tintptr)"7"); + list_add_item(src, (tintptr)"8"); + list_add_item(src, (tintptr)"9"); + list_add_item(src, (tintptr)"10"); + list_add_item(src, (tintptr)"11"); + + list_add_item(dst, (tintptr)g_strdup("0")); + list_add_item(dst, (tintptr)g_strdup("1")); + list_add_item(dst, (tintptr)g_strdup("2")); + list_add_item(dst, (tintptr)g_strdup("3")); + list_add_item(dst, (tintptr)g_strdup("4")); + list_add_item(dst, (tintptr)g_strdup("5")); + + list_append_list_strdup(src, dst, 0); + + ck_assert_int_eq(dst->count, 12); + + for (i = 0 ; i < dst->count; ++i) + { + int val = g_atoi((const char *)list_get_item(dst, i)); + ck_assert_int_eq(val, i); + } + + list_delete(src); + list_clear(dst); // Exercises auto_free code paths in list.c + list_delete(dst); +} +END_TEST + +/******************************************************************************/ + +Suite * +make_suite_test_list(void) +{ + Suite *s; + TCase *tc_simple; + + s = suite_create("List"); + + tc_simple = tcase_create("simple"); + suite_add_tcase(s, tc_simple); + tcase_add_test(tc_simple, test_list__simple); + tcase_add_test(tc_simple, test_list__simple_auto_free); + tcase_add_test(tc_simple, test_list__simple_append_list); + + return s; +}