From 6b771247338433829013fabfc711ef2ca3b596dc Mon Sep 17 00:00:00 2001 From: Joao Pereira Date: Thu, 25 Oct 2018 16:15:04 -0400 Subject: [PATCH] Add initial implementation of memory leak check In this commit an initial implementation of the memory leak check functionality is added. A new constraint to do the check `check_memory_leak` is now available Also replace all the memory management function with cgreen specific so that we can have some more control over the memory This patch does not widely add the memory check to all tests. If we want this to happen we need to call the new assertion directly. TODO: Add the assertion to the end of the test suite. Ensure that the memory cache is cleaned per test --- include/cgreen/CMakeLists.txt | 1 + include/cgreen/cgreen.h | 1 + include/cgreen/constraint_syntax_helpers.h | 1 + include/cgreen/memory.h | 22 +++ src/CMakeLists.txt | 1 + src/memory.c | 182 ++++++++++++++------- src/memory.h | 12 -- src/message_formatting.c | 1 + src/messaging.c | 5 + src/posix_cgreen_pipe.c | 6 +- src/utils.c | 1 + tests/CMakeLists.txt | 12 ++ tests/memory_leak_tests.c | 91 +++++++++++ tests/memory_leak_tests.expected | 12 ++ tests/normalize_memory_leak_tests.sed | 8 + tools/cgreen-runner.c | 9 +- tools/runner.c | 14 +- 17 files changed, 302 insertions(+), 77 deletions(-) create mode 100644 include/cgreen/memory.h delete mode 100644 src/memory.h create mode 100644 tests/memory_leak_tests.c create mode 100644 tests/memory_leak_tests.expected create mode 100644 tests/normalize_memory_leak_tests.sed diff --git a/include/cgreen/CMakeLists.txt b/include/cgreen/CMakeLists.txt index 6c0c5262..4142e853 100644 --- a/include/cgreen/CMakeLists.txt +++ b/include/cgreen/CMakeLists.txt @@ -12,6 +12,7 @@ set(cgreen_HDRS cute_reporter.h legacy.h mocks.h + memory.h string_comparison.h reporter.h runner.h diff --git a/include/cgreen/cgreen.h b/include/cgreen/cgreen.h index 68b14c34..7d59bb73 100644 --- a/include/cgreen/cgreen.h +++ b/include/cgreen/cgreen.h @@ -6,5 +6,6 @@ #include #include #include +#include #include #include diff --git a/include/cgreen/constraint_syntax_helpers.h b/include/cgreen/constraint_syntax_helpers.h index 1b202d0d..c718fac4 100644 --- a/include/cgreen/constraint_syntax_helpers.h +++ b/include/cgreen/constraint_syntax_helpers.h @@ -45,6 +45,7 @@ #define will_return_double(value) create_return_double_value_constraint(value) #define will_set_contents_of_parameter(parameter_name, value, size) create_set_parameter_value_constraint(#parameter_name, (intptr_t)value, (size_t)size) +void check_memory_leak(TestReporter *reporter); #ifdef __cplusplus extern "C" { diff --git a/include/cgreen/memory.h b/include/cgreen/memory.h new file mode 100644 index 00000000..a2fd0761 --- /dev/null +++ b/include/cgreen/memory.h @@ -0,0 +1,22 @@ +#ifndef MEMORY_HEADER +#define MEMORY_HEADER + +#ifndef __cplusplus +void* cgreen_malloc(size_t size, const char * filename, int line); +#define malloc(size) cgreen_malloc(size, __FILE__, __LINE__) + +void* cgreen_calloc(size_t num_members, size_t size, const char * filename, int line); +#define calloc(num_members, size) cgreen_calloc(num_members, size, __FILE__, __LINE__) + +void* cgreen_realloc(void* pointer, size_t size, const char * filename, int line); +#define realloc(pointer, size) cgreen_realloc(pointer, size, __FILE__, __LINE__) + +void cgreen_free(void* pointer, const char * filename, int line); +#define free(pointer) cgreen_free(pointer, __FILE__, __LINE__) + +#else +#undef check_memory_leak +#define check_memory_leak(reporter) +#endif + +#endif diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 52fcf088..a9919fce 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -36,6 +36,7 @@ set(cgreen_SRCS constraint_syntax_helpers.c cute_reporter.c cdash_reporter.c + memory.c messaging.c message_formatting.c mocks.c diff --git a/src/memory.c b/src/memory.c index ca5cef1d..6be11942 100644 --- a/src/memory.c +++ b/src/memory.c @@ -1,78 +1,150 @@ #include #include -#include "memory.h" +#include +#include +#include "include/cgreen/memory.h" -#define MEMORY_INCREMENT 1024 +typedef struct MemoryEntry_ { + void * memory_location; + size_t size; + const char* filename; + int line_number; +} MemoryEntry; -struct MemoryPool_ { - void** blocks; - long size; - long space; -}; -static void enlarge(MemoryPool *pool); -static int move_to_front(MemoryPool *pool, void *pointer); -static void move_up_one(void **blocks, long amount); +static MemoryEntry ** allocation_memory = NULL; +static int number_of_memory_entries = 0; -MemoryPool *create_memory_pool() { - MemoryPool *pool = (MemoryPool *)malloc(sizeof(MemoryPool)); - if (pool == NULL) { - return NULL; - } - pool->blocks = (void **)malloc(MEMORY_INCREMENT * sizeof(void *)); - if (pool->blocks == NULL) { - free(pool); - return NULL; - } - pool->size = 0; - pool->space = MEMORY_INCREMENT; - return pool; + +static int find_memory_position(void * pointer); +static void move_memory_back(int position_to_start_on); +static MemoryEntry * get_memory_entry(void * pointer); + +#undef malloc +static void create_allocation_memory(void) { + allocation_memory = malloc(sizeof(MemoryEntry)*100); } +#define malloc cgreen_malloc -void free_memory_pool(MemoryPool *pool) { - long i; - for (i = 0; i < pool->size; i++) { - free(pool->blocks[i]); - } - free(pool->blocks); - free(pool); +#undef malloc +void* cgreen_malloc(size_t size, const char * filename, int line) { + if(allocation_memory == NULL) + create_allocation_memory(); + + void * new_memory_pointer = malloc(size); + MemoryEntry * new_memory_entry = malloc(sizeof(MemoryEntry)); + new_memory_entry->memory_location = new_memory_pointer; + new_memory_entry->size = size; + new_memory_entry->filename = filename; + new_memory_entry->line_number = line; + allocation_memory[number_of_memory_entries] = new_memory_entry; + number_of_memory_entries++; + + return new_memory_pointer; } +#define malloc cgreen_malloc -void *memory_pool_allocate(MemoryPool *pool, size_t bytes) { - enlarge(pool); - return pool->blocks[pool->size++] = malloc(bytes); +void* cgreen_calloc(size_t num_members, size_t size, const char * filename, int line) { + void * allocated_memory = cgreen_malloc(num_members * size, filename, line); + if (allocated_memory) + memset(allocated_memory, 0, num_members * size); + return allocated_memory; } -void *memory_pool_reallocate(MemoryPool *pool, void *pointer, size_t bytes) { - if (! move_to_front(pool, pointer)) { +void* cgreen_realloc(void* pointer, size_t size, const char * filename, int line) { + if (pointer == NULL) + return cgreen_malloc(size, filename, line); + + if (size == 0) { + cgreen_free(pointer, filename, line); return NULL; } - return pool->blocks[0] = realloc(pool->blocks[0], bytes); + + MemoryEntry * memory_position = get_memory_entry(pointer); + + void * new_pointer = cgreen_malloc(size, filename, line); + + size_t actual_size = size; + if (size > memory_position->size) + actual_size = memory_position->size; + + memcpy(new_pointer, pointer, actual_size); + + cgreen_free(pointer, filename, line); + + return new_pointer; } -static void enlarge(MemoryPool *pool) { - if (pool->size == pool->space) { - pool->blocks = (void**)realloc(pool->blocks, (pool->space + MEMORY_INCREMENT) * sizeof(void *)); - pool->space += MEMORY_INCREMENT; + +#undef free +void cgreen_free(void * pointer, const char * filename, const int line) { + va_list null_arguments; + memset(&null_arguments, 0, sizeof(null_arguments)); + if (pointer == NULL) { + + TestReporter *reporter = get_test_reporter(); + reporter->show_fail(reporter, filename, line, "Trying to free memory using a NULL pointer", null_arguments); + send_reporter_exception_notification(reporter); + return; + } + + int memory_position = find_memory_position(pointer); + + if (memory_position == -1) { + char message[100]; + sprintf(message, "Trying to free memory [%p] that was not previously allocated", pointer); + + TestReporter *reporter = get_test_reporter(); + reporter->show_fail(reporter, filename, line, message, null_arguments); + send_reporter_exception_notification(reporter); + return; } + + free(allocation_memory[memory_position]); + move_memory_back(memory_position); + number_of_memory_entries--; } +#define free cgreen_free -static int move_to_front(MemoryPool *pool, void *pointer) { - long i; - for (i = 1; i < pool->size; i++) { - if (pool->blocks[i] == pointer) { - move_up_one(pool->blocks, i); - pool->blocks[0] = pointer; - return 1; +void check_memory_leak(TestReporter *reporter) { + if (number_of_memory_entries > 0) { + for( int i = 0 ; i < number_of_memory_entries ; i++) { + MemoryEntry * memoryEntry = allocation_memory[i]; + (reporter->assert_true)( + reporter, + memoryEntry->filename, + memoryEntry->line_number, + 0, + "Memory allocated [%p] was not freed [%s:%d]", + memoryEntry->memory_location, + memoryEntry->filename, + memoryEntry->line_number + ); } } - return 0; } -/* static void move_up_one(void **blocks, long amount) { */ -/* if (amount == 0) { */ -/* return; */ -/* } */ -/* // FIXME: this doesn't compile and the semantics might be wrong */ -/* //memmove(blocks[0], blocks[1], (size_t)(blocks[amount] - blocks[1])); */ -/* } */ +int find_memory_position(void * pointer) { + for( int i = 0 ; i < number_of_memory_entries ; i++) { + MemoryEntry * memoryEntry = allocation_memory[i]; + if(memoryEntry && memoryEntry->memory_location == pointer) + return i; + } + + return -1; +} + +MemoryEntry * get_memory_entry(void * pointer) { + int position = find_memory_position(pointer); + + if (position == -1) + return NULL; + + return allocation_memory[position]; +} + +void move_memory_back(int position_to_start_on) { + for(int i = position_to_start_on ; i < number_of_memory_entries - 1 ; i++) { + allocation_memory[i] = allocation_memory[i+1]; + } +} \ No newline at end of file diff --git a/src/memory.h b/src/memory.h deleted file mode 100644 index 38ac590f..00000000 --- a/src/memory.h +++ /dev/null @@ -1,12 +0,0 @@ -#ifndef MEMORY_HEADER -#define MEMORY_HEADER - - -typedef struct MemoryPool_ MemoryPool; - -MemoryPool *create_memory_pool(); -void free_memory_pool(MemoryPool *pool); -void *memory_pool_allocate(MemoryPool *pool, size_t bytes); -void *memory_pool_reallocate(MemoryPool *pool, void *pointer, size_t bytes); - -#endif diff --git a/src/message_formatting.c b/src/message_formatting.c index 063e2b21..4d80df5a 100644 --- a/src/message_formatting.c +++ b/src/message_formatting.c @@ -18,6 +18,7 @@ #endif #include "constraint_internal.h" +#include // Handling of percent signs diff --git a/src/messaging.c b/src/messaging.c index 8f7360db..0778a8f8 100644 --- a/src/messaging.c +++ b/src/messaging.c @@ -19,6 +19,7 @@ #include #endif +#include #define message_content_size(Type) (sizeof(Type) - sizeof(long)) @@ -61,6 +62,7 @@ int get_pipe_write_handle(void) static void clean_up_messaging(void); +#undef realloc int start_cgreen_messaging(int tag) { CgreenMessageQueue *tmp; int pipes[2]; @@ -95,6 +97,7 @@ int start_cgreen_messaging(int tag) { queues[queue_count - 1].tag = tag; return queue_count - 1; } +#define realloc cgreen_realloc void send_cgreen_message(int messaging, int result) { CgreenMessage *message; @@ -128,6 +131,7 @@ int receive_cgreen_message(int messaging) { return result; } +#undef free static void clean_up_messaging(void) { int i; for (i = 0; i < queue_count; i++) { @@ -140,5 +144,6 @@ static void clean_up_messaging(void) { queues = NULL; queue_count = 0; } +#define free cgreen_free /* vim: set ts=4 sw=4 et cindent: */ diff --git a/src/posix_cgreen_pipe.c b/src/posix_cgreen_pipe.c index b61ac1a5..455a0087 100644 --- a/src/posix_cgreen_pipe.c +++ b/src/posix_cgreen_pipe.c @@ -18,7 +18,8 @@ /* One way to do it the old way is: ioctl(fd, FIOBIO, &flags); */ #endif - +#undef malloc +#undef free int cgreen_pipe_open(int pipes[2]) { int pipe_open_result; @@ -70,5 +71,6 @@ ssize_t cgreen_pipe_write(int p, const void *buf, size_t count) } return pipe_write_result; } - +#define malloc cgreen_malloc +#define free cgreen_free /* vim: set ts=4 sw=4 et cindent: */ diff --git a/src/utils.c b/src/utils.c index 2242eee5..b6ed1258 100644 --- a/src/utils.c +++ b/src/utils.c @@ -4,6 +4,7 @@ #include #include #include +#include char *string_dup(const char *string) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 5363c1b1..53aed05f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -178,6 +178,11 @@ set(xml_output_library_SRCS xml_output_tests.c) add_library(${xml_output_library} SHARED ${xml_output_library_SRCS}) target_link_libraries(${xml_output_library} ${CGREEN_LIBRARY}) +set(memory_leak_library memory_leak_tests) +set(memory_leak_library_SRCS memory_leak_tests.c) +add_library(${memory_leak_library} SHARED ${memory_leak_library_SRCS}) +target_link_libraries(${memory_leak_library} ${CGREEN_LIBRARY}) + set(TEST_TARGET_LIBRARIES ${CGREEN_LIBRARY}) macro_add_test( @@ -224,5 +229,12 @@ macro_add_test(NAME xml_output ${xml_output_library}.expected ) +macro_add_test(NAME memory_leaks + COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/../tools/cgreen_runner_output_diff + memory_leak_tests # Name + ${CMAKE_CURRENT_SOURCE_DIR} # Where sources are + ${memory_leak_library}.expected +) + # add verification that all public api is available as it should add_subdirectory(api) diff --git a/tests/memory_leak_tests.c b/tests/memory_leak_tests.c new file mode 100644 index 00000000..993fe720 --- /dev/null +++ b/tests/memory_leak_tests.c @@ -0,0 +1,91 @@ +#include +#include +#include +#include +#include "include/cgreen/memory.h" + +Describe(MemoryLeak) +BeforeEach(MemoryLeak) {} +AfterEach(MemoryLeak) {} + +Ensure(MemoryLeak, free_null_pointer) { + free(NULL); +} + +Ensure(MemoryLeak, alloc_and_free_memory) { + int* new_int = malloc(sizeof(int)); + free(new_int); + check_memory_leak(get_test_reporter()); + assert_that(true, is_true); +} + +Ensure(MemoryLeak, multiple_allocations_and_free_of_memory_in_different_order_works) { + int* new_int = malloc(sizeof(int)); + int* new_int1 = malloc(sizeof(int)); + int* new_int2 = malloc(sizeof(int)); + + free(new_int); + + int* new_int3 = malloc(sizeof(int)); + + free(new_int2); + free(new_int1); + free(new_int3); + + check_memory_leak(get_test_reporter()); + assert_that(true, is_true); +} + +Ensure(MemoryLeak, free_unallocate_memory) { + free((void*)1); + + check_memory_leak(get_test_reporter()); +} + +Ensure(MemoryLeak, fail_test_when_memory_is_leaked) { + malloc(sizeof(int)); + + check_memory_leak(get_test_reporter()); +} + +Ensure(MemoryLeak, use_realloc_maintain_data_in_string) { + char * new_pointer = malloc(sizeof(char) * 5); + strcpy(new_pointer, "123"); + + char* new_string = realloc(new_pointer, sizeof(char) * 10); + assert_that(new_string, is_equal_to_string("123")); + + free(new_string); +} + +Ensure(MemoryLeak, use_realloc_to_increase_the_size_of_a_pointer) { + char * new_pointer = malloc(sizeof(char) * 5); + + char* new_string = realloc(new_pointer, sizeof(char) * 10); + strcpy(new_string, "123456789"); + free(new_string); + check_memory_leak(get_test_reporter()); +} + +Ensure(MemoryLeak, use_realloc_to_decrease_the_size_of_a_pointer) { + char * new_pointer = malloc(sizeof(char) * 10); + + char* new_string = realloc(new_pointer, sizeof(char) * 5); + strcpy(new_string, "1234"); + + free(new_string); + check_memory_leak(get_test_reporter()); +} + +Ensure(MemoryLeak, use_calloc_to_allocate_an_array) { + int * array = calloc(5, sizeof(int)); + + array[0] = 1; + array[1] = 1; + array[2] = 1; + array[3] = 1; + array[4] = 1; + + free(array); + check_memory_leak(get_test_reporter()); +} diff --git a/tests/memory_leak_tests.expected b/tests/memory_leak_tests.expected new file mode 100644 index 00000000..2c5a26ad --- /dev/null +++ b/tests/memory_leak_tests.expected @@ -0,0 +1,12 @@ +Running "memory_leak_tests" (9 tests)... +memory_leak_tests.c: Failure: MemoryLeak -> fail_test_when_memory_is_leaked + Memory allocated [...] was not freed [memory_leak_tests.c:46] + +memory_leak_tests.c: Failure: MemoryLeak -> free_null_pointer + Trying to free memory using a NULL pointer + +memory_leak_tests.c: Failure: MemoryLeak -> free_unallocate_memory + Trying to free memory [0x1] that was not previously allocated + + "MemoryLeak": 3 passes, 1 failure, 2 exceptions in 0ms. +Completed "memory_leak_tests": 3 passes, 1 failure, 2 exceptions in 0ms. diff --git a/tests/normalize_memory_leak_tests.sed b/tests/normalize_memory_leak_tests.sed new file mode 100644 index 00000000..cedb64c0 --- /dev/null +++ b/tests/normalize_memory_leak_tests.sed @@ -0,0 +1,8 @@ +# 'time="?.?????"' => time="0.00000" +s/time=".+"/time="0.00000"/g +s/line=".+"/line="0"/g +# filenames, suites, libraries and "classnames" starting with "lib" or "cyg" +s/"lib/"/g +s/"cyg/"/g +# pointers address to ... +s/y allocated \[[0-9xa-f]+\]/y allocated \[...\]/g diff --git a/tools/cgreen-runner.c b/tools/cgreen-runner.c index d3a6ae96..6a30905d 100644 --- a/tools/cgreen-runner.c +++ b/tools/cgreen-runner.c @@ -13,7 +13,10 @@ #include "runner.h" #include "discoverer.h" - +#undef free +#undef malloc +#undef calloc +#undef realloc /*----------------------------------------------------------------------*/ static int file_exists(const char *filename) { @@ -66,8 +69,8 @@ static char* get_a_suite_name(const char *suite_option, const char *test_library char *suite_name; char *s; /* basename can return the parameter or an internal static string. Work around this. */ - s = string_dup(test_library_name); - suite_name = string_dup(basename(s)); + s = strdup(test_library_name); + suite_name = strdup(basename(s)); free(s); if ((s = strchr(suite_name, '.'))) { *s = '\0'; diff --git a/tools/runner.c b/tools/runner.c index 29795110..ad7c0ee2 100644 --- a/tools/runner.c +++ b/tools/runner.c @@ -30,6 +30,10 @@ typedef struct ContextSuite { struct ContextSuite *next; } ContextSuite; +#undef free +#undef malloc +#undef calloc +#undef realloc /*----------------------------------------------------------------------*/ static void destroy_context_suites(ContextSuite *context_suite) { if (context_suite != NULL) { @@ -48,10 +52,10 @@ static char *context_name_of(const char* symbolic_name) { char *context_name; if (strchr(symbolic_name, ':')) { - context_name = string_dup(symbolic_name); + context_name = strdup(symbolic_name); *strchr(context_name, ':') = '\0'; } else { - context_name = string_dup(CGREEN_DEFAULT_SUITE); + context_name = strdup(CGREEN_DEFAULT_SUITE); } return context_name; @@ -62,10 +66,10 @@ static char *context_name_of(const char* symbolic_name) { static char *test_name_of(const char *symbolic_name) { const char *colon = strchr(symbolic_name, ':'); if (colon) { - return string_dup(colon+1); + return strdup(colon+1); } - return string_dup(symbolic_name); + return strdup(symbolic_name); } @@ -105,7 +109,7 @@ static TestSuite *find_suite_for_context(ContextSuite *suites, const char *conte static ContextSuite *add_new_context_suite(TestSuite *parent, const char* context_name, ContextSuite *next) { ContextSuite *new_context_suite = (ContextSuite *)calloc(1, sizeof(ContextSuite)); - new_context_suite->context_name = string_dup(context_name); + new_context_suite->context_name = strdup(context_name); new_context_suite->suite = create_named_test_suite(context_name); new_context_suite->next = next; add_suite_(parent, context_name, new_context_suite->suite);