List Ops memory leaks about c HOT 9 CLOSED

I see what you mean.

However, this leakage is when tests fail. And in a complete and finished solution tests should not be failing.

I think we need to draw a distinction between leaky implementation and leaky tests. Otherwise this problem will appy to all exercises with dynamic allocation and would always need to "give away" the implementation for clearing up.

from c.

@wolf99 Yes, you are absolutely right. The append tests use list1 and list2, which is why I specified that the example was just for the map test. One thing we could do is change list1 to list and just add list2. That's a change that the student wouldn't see. The teardown would have three list_t pointers to check. Plus there is an issue with the error_message. Here's a more complete example

#include "test-framework/unity.h"
#include "list_ops.h"

const int MAX_STRING_LEN = 100;
list_t *list = NULL;
list_t *list2 = NULL;
list_t *actual = NULL;
char *error_message = NULL;

void setUp(void)
{
}

static void release(list_t **list)
{
   if (*list)
   {
      free(*list);
      *list = NULL;
   }
}

void tearDown(void)
{
   release(&list);
   release(&list2);
   release(&actual);
   if (error_message)
   {
      free(error_message);
      error_message = NULL;
   }
}

// code snipped
static void check_lists_match(size_t expected_length,
                              list_element_t expected_elements[],
                              list_t *actual)
{
   // check actual list is a valid list
   TEST_ASSERT_NOT_NULL(actual);

   // check lengths match
   TEST_ASSERT_EQUAL_MESSAGE(expected_length, actual->length,
                             "List lengths differ");

   // check elements match in non-zero length list
   if (expected_length)
   {
      error_message = create_error_message(
          expected_length, expected_elements, actual->elements);
      TEST_ASSERT_EQUAL_MEMORY_ARRAY_MESSAGE(
          expected_elements, actual->elements, sizeof(list_element_t),
          expected_length, error_message);
      free(error_message);
      error_message = NULL;
   }
}
// code snipped

static void test_append_list_to_empty_list(void)
{
   TEST_IGNORE(); // delete this line to run test
   list = new_list(0, NULL);
   list2 = new_list(3, (list_element_t[]){1, 3, 4});
   size_t expected_length = 3;
   list_element_t expected_elements[] = {1, 3, 4};

   actual = append_list(list, list2);
   check_lists_match(expected_length, expected_elements, actual);

   delete_list(&list);
   delete_list(&list2);
   delete_list(&actual);
}
// code snipped
static void test_map_non_empty_list(void)
{

   TEST_IGNORE();
   list = new_list(4, (list_element_t[]){1, 3, 5, 7});
   size_t expected_length = 4;
   list_element_t expected_elements[] = {2, 4, 6, 8};

   actual = map_list(list, map_increment);
   check_lists_match(expected_length, expected_elements, actual);

   delete_list(&list);
   delete_list(&actual);
}

from c.

To summarize my understanding of the proposal, there are two options:
Ask the implementation to clearing the pointer
Take care of clearing the pointer in the client code (the tests)

We currently delegate clearing the pointer to the student's solution. Whether we take that away from the student or not, we still need the tests to make sure the pointers are cleared on a failed ASSERT (not only for the one the student allocates, but also the ones the tests allocate, including the error_message.) I suppose we could remove the delete_list from the solution, but removing it from the header would also break the solutions. Plus, keeping it, but changing the signature, may be a good opportunity for the student to learn how to thoroughly clear a pointer and how to do it in a helper function via a pointer to the pointer being cleared. It's somewhat problematical whether we keep it the way it is leaking memory, or try to fix it. 😢

from c.

Instead of changing the instructions, perhaps we could just change the comment for delete_list when its signature is changed. Maybe something like

// destroy the entire list
// a thoroughly destroyed list is freed and set to NULL
void delete_list(list_t **list);

from c.

I'm looking at Pascal's Triangle, which also delegates freeing allocated memory to the student, and I realized that the List Ops tests can set the freed lists to NULL. For example

static void test_map_non_empty_list(void)
{
   TEST_IGNORE();
   list = new_list(4, (list_element_t[]){1, 3, 5, 7});
   size_t expected_length = 4;
   list_element_t expected_elements[] = {2, 4, 6, 8};

   actual = map_list(list, map_increment);
   check_lists_match(expected_length, expected_elements, actual);

   delete_list(list);
   list = NULL;
   delete_list(actual);
   actual = NULL;
}

That way the delete_list signature does not have to change, and there is no double-free if the test passes. So I think that might be the way to go. Any currently passing solutions will still pass.

from c.

To summarise my understanding of the proposal, there are two options:

1. Ask the implementation to clearing the pointer
2. Take care of clearing the pointer in the client code (the tests)

I've seen it done, and done it myself, both ways in the wild.
The first option (1 above) may be a more informative option for students to learn about clearing pointers - if we add a test that enforces it somehow.

One wrinkle to consider if moving declaration and clearing outside of the test function scope, is that some test functions use list and actual, but other test functions require more lists so have list1, list2 and actual. So there are now three pointers used within the file, of which some test functions some of them are used, while in other test functions other of them are used...

That is:

list_t *list = NULL;
list_t *list1 = NULL;
list_t *list2 = NULL;
list_t *actual = NULL;

test_foo_list_op(void)
{
   /* list1 and list2 not used */
   list = new_list( /*some args*/ );
   actual = foo_list_op( list, /*some other args */ );
   check_foo_list_op(actual);
}

test_bar_list_op(void)
{
   /* list not used */
   list1 = new_list( /* some args*/ );
   list2 = new_list( /*some other args*/ );
   actual = bar_list_op(list1, list2, /* further args */ );
   check_bar_list_op(actual);
}

Now we could do something like the following. But as you can see, this gets messy fast. Is there another option?

void tearDown(void)
{
   if (actual)
   {
      free(actual);
      actual = NULL;
   }
   if (list)
   {
      free(list);
      list = NULL;
   }
   if (list1)
   {
      free(list1);
      list1 = NULL;
   }
   if (list2)
   {
      free(list2);
      list2 = NULL;
   }
}

from c.

@wolf99 If the student develops the solution in the browser, that can be a lot of leaky outcomes before they pass all the tests. And you are absolutely right that this problem applies to all exercises with dynamic allocation. Which makes it even more of a problem for whatever virtual machines they're running in, in terms of eventually being starved of resources from many potentially leaking test runs.

Giving away the implementation is a valid issue. We don't have to do it. They can receive the double-free error and ask for mentoring if they don't understand why. I agree with the distinction that the student shouldn't be informed about the tests being leaky, but only about their own implementation faults. They will see the tests leaking now if they locally fail a test when running "make memcheck", which is how I discovered it. So the proposed fix would meet your distinction of the student not being exposed to the tests leaking.

I've looked around at the 40+ exercises I've completed and this is not an issue for almost all of them. That leaves 20+ I haven't checked, but many of them are math-centric without dynamic allocation. So I don't anticipate this coming up as a problem too many more times. At least I hope not! 😆

from c.

I should also mention, so far, this is the only exercise I've seen where clearing of a pointer defined in the test is delegated to the student. There may not be another exercise with that unique situation, but I haven't thoroughly checked them all yet.

from c.

Closed by #756

from c.