Proper memory free for pointers inside struct C

Question

I have a struct list which contains a dynamic array of type struct pair, and each element of this array points to two variable of type struct element. In the main of my program I allocate, via a malloc call, memory for struct pair pairs array and for each element pointer. Then I assign a value to elements, check the value and free the memory. Here is the code

#include "stdlib.h"
#include "stdio.h"

struct element{
  int i;
};

struct pair{
  struct element *element1, *element2;
};

struct list{
  struct pair *pairs;
};

int main(void) {

  // declare variable my_list
  struct list my_list;

  // set size of pairs array (one for the example)
  my_list.pairs = (struct pair*)malloc(sizeof(struct pair));

  // set element1 and element2 of pairs[0]
  struct element element1 = {.i = 1};
  struct element element2 = {.i = 2};
  my_list.pairs[0].element1 = (struct element*)malloc(sizeof(struct element));
  my_list.pairs[0].element2 = (struct element*)malloc(sizeof(struct element));
  my_list.pairs[0].element1 = &element1;
  my_list.pairs[0].element2 = &element2;

  // check element values
  printf("elements are: %d %d\n", my_list.pairs[0].element1->i, my_list.pairs[0].element2->i); 

  // free memory
  free(my_list.pairs);

  return 0;
}

I can compile and run the code without problems but if I run the code in valgrind I get the following memory leak:

*** valgrind --leak-check=full -s ./a.out 
==6676== Memcheck, a memory error detector
==6676== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==6676== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==6676== Command: ./a.out
==6676== 
elements are: 1 2
==6676== 
==6676== HEAP SUMMARY:
==6676==     in use at exit: 8 bytes in 2 blocks
==6676==   total heap usage: 4 allocs, 2 frees, 1,048 bytes allocated
==6676== 
==6676== 4 bytes in 1 blocks are definitely lost in loss record 1 of 2
==6676==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==6676==    by 0x1091EE: main (foo.c:27)
==6676== 
==6676== 4 bytes in 1 blocks are definitely lost in loss record 2 of 2
==6676==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==6676==    by 0x1091FF: main (foo.c:28)
==6676== 
==6676== LEAK SUMMARY:
==6676==    definitely lost: 8 bytes in 2 blocks
==6676==    indirectly lost: 0 bytes in 0 blocks
==6676==      possibly lost: 0 bytes in 0 blocks
==6676==    still reachable: 0 bytes in 0 blocks
==6676==         suppressed: 0 bytes in 0 blocks
==6676== 
==6676== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

Lines 27 and 28 are responsible of the leak. If I try to add (before line 36), something like free(my_list.pairs[0].element1); I get the following error in valgrind

==6684== 1 errors in context 1 of 3:
==6684== Invalid free() / delete / delete[] / realloc()
==6684==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==6684==    by 0x10924F: main (foo.c:36)
==6684==  Address 0x1ffefff918 is on thread 1's stack
==6684==  in frame #1, created by main (foo.c:16)

So my questions are:

• how can I avoid the memory leak and properly free the memory allocated by lines 27 and 28?
• why trying to free the pointer my_list.pairs[0].element1 rise the error Invalid free() / delete / delete[] / realloc()?

Code has been compiler with gcc as follow: gcc -Wall -g foo.c and my version of gcc is

gcc (Ubuntu 9.4.0-1ubuntu1~20.04.2) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Answer 1

Here your program is leaking memory:

  my_list.pairs[0].element1 = &element1;
  my_list.pairs[0].element2 = &element2;

reason is - my_list.pairs[0].element1 and my_list.pairs[0].element2 are holding the reference of memory allocated dynamically and after the above assignment, they lose those memory references. Hence, resulting in memory leak.

Note that, in your code, element1 and element2 are not the dynamically allocated objects. They are the objects created on the stack:

struct element element1 = {.i = 1};   
struct element element2 = {.i = 2};

It's not valid to free them:

free(my_list.pairs[0].element1); 

free can only be used for pointers allocated by malloc(), calloc(), aligned_alloc() or realloc(), otherwise it will result in undefined behavior.

To fix this problem either don't allocate memory to my_list.pairs[0].element1 and my_list.pairs[0].element2 dynamically or allocate them dynamically and free them once you are done with them. Implementation of later suggestion:

  struct element * element1 = (struct element*)malloc(sizeof(struct element));
  struct element * element2 = (struct element*)malloc(sizeof(struct element));
  element1->i = 1;
  element2->i = 2;
  my_list.pairs[0].element1 = element1;
  my_list.pairs[0].element2 = element2;

  // check element values
  printf("elements are: %d %d\n", my_list.pairs[0].element1->i, my_list.pairs[0].element2->i); 

  // free memory
  free(my_list.pairs[0].element1);
  free(my_list.pairs[0].element2);

Additional:

Follow good programming practice, make sure to check the returned value of malloc().

Answer 2

Your memory leak is here:

my_list.pairs[0].element1 = (struct element*)malloc(sizeof(struct element));
my_list.pairs[0].element2 = (struct element*)malloc(sizeof(struct element));
my_list.pairs[0].element1 = &element1; // overwrite of my_list.pairs[0].element1 --> memory leak
my_list.pairs[0].element2 = &element2; // overwrite of my_list.pairs[0].element2 --> memory leak

Solution:

Option 1

Delete the two lines doing malloc.

struct element element1 = {.i = 1};
struct element element2 = {.i = 2};
my_list.pairs[0].element1 = &element1;
my_list.pairs[0].element2 = &element2;

Option 2

Delete the lines creating and using local variables.

my_list.pairs[0].element1 = malloc(sizeof(struct element));
my_list.pairs[0].element2 = malloc(sizeof(struct element));
my_list.pairs[0].element1->i = 1;
my_list.pairs[0].element2->i = 2;

....
....

free(my_list.pairs[0].element1);
free(my_list.pairs[0].element2);

note: In C you don't need to cast the value returned by malloc

Answer 3

These statements

  my_list.pairs[0].element1 = (struct element*)malloc(sizeof(struct element));
  my_list.pairs[0].element2 = (struct element*)malloc(sizeof(struct element));
  my_list.pairs[0].element1 = &element1;
  my_list.pairs[0].element2 = &element2;

produce memory leaks.

At first memory was allocated and its addresses were assigned to pointers my_list.pairs[0].element1 and my_list.pairs[0].element2 and then the pointers were reassigned with addresses of local variable element1 and element2

  // set element1 and element2 of pairs[0]
  struct element element1 = {.i = 1};
  struct element element2 = {.i = 2};

So the addreses of the allocated memory were lost.

Instead of using this assignment

  my_list.pairs[0].element1 = &element1;
  my_list.pairs[0].element2 = &element2;

you should write

  *my_list.pairs[0].element1 = element1;
  *my_list.pairs[0].element2 = element2;

to copy values of element1 and element2 in the allocated memory.

To free correctly the allocated memory you need to free it in the reverse order. That is

free( my_list.pairs[0].element1 );
free( my_list.pairs[0].element2 );
free( my_list.pairs );

As for your code then this statement

  // free memory
  free(my_list.pairs);

frees only the first allocated extent of memory while you allocated three extents of memory

  my_list.pairs = (struct pair*)malloc(sizeof(struct pair));
  //...
  my_list.pairs[0].element1 = (struct element*)malloc(sizeof(struct element));
  my_list.pairs[0].element2 = (struct element*)malloc(sizeof(struct element));

Answer 4

At this point you have some good answers as for why you have a memory leak, but I will leave here an example of a way of creating this type of things that can be useful. Or ignored. :)

From the original code:

I changed element1 for el1 for clarity. Also el2.

    my_list.pairs[0].el1 =  (struct element*)malloc(sizeof(struct element));
    my_list.pairs[0].el2 =  (struct element*)malloc(sizeof(struct element));
    my_list.pairs[0].el1 = &el1;
    my_list.pairs[0].el2 = &el2;

After reordering the lines:

    my_list.pairs[0].el1 =  (struct element*)malloc(sizeof(struct element));
    my_list.pairs[0].el1 = &el1;

    my_list.pairs[0].el2 =  (struct element*)malloc(sizeof(struct element));
    my_list.pairs[0].el2 = &el2;

And it makes clear that as soon the 2nd and 4th line runs each area returned from malloc is gone for good, since in each case the single pointer for the area was overwritten.

Also

    // declare variable my_list
    struct list my_list;

Maybe you could avoid this type of comment. It is clear what the declaration does.

The example

We have here a list of pairs. Could be implemented as an array, as a linked list, as a dictionary... java has Pair as of Key and Value things, and C++ has pair of first and second in std::pair as examples. And could be a list of a list of strings. Any 3-level hierarchy of things.

I will show you a way of writing this, using encapsulation, and viewing your pairs and list more like objects. It can be more productive doing so, as it uses a lot of copy and paste and simply reuses things from these other languages and (also important) form one struct to the other.

The hierarchy: a list of elements in pairs

We will use List like a simple array of pointers, of fixed capacity. It can be implemented in any other way, like using a linked list or a simple array. But a list is just it: a (possibly empty) collection of things. In this case, pairs of something. For the List it makes no difference what something is. This is in essence encapsulation. Here, note that list makes no reference to Element and references Pairs only as an opaque object.

The list List

#ifndef LIST_H
#define LIST_H

#include <stdio.h>
#include "pair.h"

typedef struct
{
    size_t size;
    size_t limit;
    Pair** pair;
} List;

List* make_list(size_t);
List* delete_list(List*);
int   insert_list(Pair*, List*);
int   show_list(List*, const char*);

#endif // LIST_H

So a list has inside it the values for its actual size and capacity. The functions, called methods in other languages, makes no reference to elements. Even the Pair could have been abstracted, using a generic name line Info or Node.

• make_list returns a pointer to a new empty list with the requested capacity
• delete_list destroys a list and return NULL. Why? This is so we can delete a List and invalidate its pointer at the same time, for safety.
• insert_list does the expected: inserts a record into a list. In this case a Pair
• show_list shows on-screen the elements of the list. To save a few printf calls, a prefix message can be added.

This is like a constructor, a destructor and a toString or serialize method in other places/languages. Only pointers are used. It is easier.

A possible implementation for List.c

#include "list.h"

List* make_list(size_t size)
{
    List* one = malloc(sizeof(List));
    if (one == NULL) return NULL;
    one->limit = size;
    one->size  = 0;
    one->pair  = (Pair**)malloc(size * sizeof(Pair*));
    if (one->pair == NULL)
    {
        free(one);
        return NULL;
    }
    return one;
}

List* delete_list(List* L)
{
    if (L == NULL) return NULL;
    fprintf(
        stderr, "\n    deleting %llu elements list\n",
        L->size);
    for (size_t i = 0; i < L->size; i += 1)
        delete_pair(L->pair[i]);
    free(L);
    return NULL;
}

int insert_list(Pair* p, List* l)
{
    if (p == NULL) return -1;
    if (l == NULL) return -2;
    if (l->size == l->limit) return -3;
    Pair* pair       = make_pair(p->first, p->second);
    l->pair[l->size] = pair;
    l->size += 1;
    return 0;
}

int show_list(List* L, const char* msg)
{
    if (L == NULL) return -1;
    if (msg != NULL) printf("%s", msg);
    printf(
        "[%llu/%llu] element(s) in list\n", L->size,
        L->limit);
    for (size_t i = 0; i < L->size; i += 1)
        show_pair(L->pair[i], NULL);
    printf("\n");
    return 0;
}

The pair in Pair.h

#ifndef PAIR_H
#define PAIR_H
#include <stdio.h>
#include <stdlib.h>
#include "el1.h"

typedef struct
{
    Element* first;
    Element* second;
} Pair;

Pair* delete_pair(Pair*);
Pair* make_pair(Element*, Element*);
int   show_pair(Pair*, const char*);
#endif // PAIR_H

Here we have the C++ naming for pairs: elements are first and second. java uses K and V for key and value.

• make_pair returns a Pair with the provided elements.
• show_pair does as expected.
• delete_pair frees a pair and return NULL.

These are just copied from List and adapted. Only pointers are used.

A possible implementation for Pair.c

#include "pair.h"
Pair* delete_pair(Pair* gone)
{
    if (gone == NULL) return NULL;
    delete_el(gone->first);
    delete_el(gone->second);
    free(gone);
    return NULL;
}

Pair* make_pair(Element* a, Element* b)
{
    if (a == NULL) return NULL;
    if (b == NULL) return NULL;
    Pair* one = malloc(sizeof(Pair));
    if (one == NULL) return NULL;
    one->first  = copy_el(a);
    one->second = copy_el(b);
    return one;
}

int show_pair(Pair* p, const char* msg)
{
    if (p == NULL) return -1;
    if (msg != NULL) printf("%s", msg);
    printf("  [");
    show_el(p->first, NULL);
    printf(",");
    show_el(p->second, NULL);
    printf("]\n");
    return 0;
}

The elements, at last

Here is where things are customized. I will use 2 Element:

typedef struct
{
    int i;
} Element;

and

typedef struct
{
    char* name;
    int   code;
} Element;

The first is the same as in the question. The second is just to show that an Element can allocate memory, can have pointers inside, so we can not just copy its value inside a Pair.

So, in order to Pair handle such contents the user needs to provide the methods. Doing so Pair --- and List --- go unchanged from program to program like in other languages. pair.h has an #include for the Element header.

This is what we do in qsort providing the compare function, so the code can sort anything.

The reference to Element could be done in other smarter ways, but this here is just a toy to show how to build these things fast, so an #include is used.

The functions needed for Element

Element* copy_el(Element*);
Element* delete_el(Element*);
int      show_el(Element*, const char*);

It is almost the same as for the other "classes":

• copy_el is the copy constructor, and it is all we need to insert an element in a List: it returns the address of a copy of the element, so it is owned by the list and can be inserted at a Pair
• delete_el is the destructor, since the Element can have complex allocation issues.
• show_el is a method to display on-screen an Element

an example using int

#include <stdio.h>
#include <stdlib.h>
#include "list.h"

int main(void)
{
    Element* one     = make_el(42);
    Element* other   = make_el(-42);
    Pair*    my_pair = make_pair(one, other);
    delete_el(one);
    delete_el(other);

    show_pair(my_pair, "test_pair is ");
    List* my_list = make_list(5);  // capacity = 5
    show_list(my_list, "\n(list still empty) ");
    while (0 == insert_list(my_pair, my_list)) {};
    show_list(
        my_list, "\n(list now filled with same pair) ");
    my_pair = delete_pair(my_pair);  // free test_pair
    my_list = delete_list(my_list);  // free list
    return 0;
}

• make_el is just a helper to create an Element in these examples. The container only needs to know how to copy one, not how to create one.

This program

• creates a Pair
• creates a ' List of 5 elements
• fills the list with this pair, since the contents are irrelevant here.
• shows the list on-screen
• free's everithing

output

test_pair is   [42,-42]

(list still empty) [0/5] element(s) in list


(list now filled with same pair) [5/5] element(s) in list
  [42,-42]
  [42,-42]
  [42,-42]
  [42,-42]
  [42,-42]


    deleting 5 elements list

a simple implementation for the trivial Element

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct
{
    int i;
} Element;

Element* copy_el(Element*);
Element* delete_el(Element*);
Element* make_el(const int);
int      show_el(Element*, const char*);

Element* copy_el(Element* orig)
{
    if (orig == NULL) return NULL;
    Element* one = malloc(sizeof(Element));
    if (one == NULL) return NULL;
    one->i = orig->i;
    return one;
}

Element* delete_el(Element* orig)
{
    if (orig == NULL) return NULL;
    free(orig);
    return NULL;
}

Element* make_el(const int value)
{
    Element* one = malloc(sizeof(Element));
    if (one == NULL) return NULL;
    one->i = value;
    return one;
}

int show_el(Element* el, const char* msg)
{
    if (el == NULL) return -1;
    if (msg != NULL) printf("%s", msg);
    printf("%d", el->i);
    return 0;
}

Again, make_el is here just for the example. The container --- any one --- does not create an element in itself. It just copy one when needed and destroy when needed.

an Element using a struct that allocates memory

This is an example that has a pointer inside and allocates memory. It can be used the same way since the user provides a copy constructor.

main for this case

Here make_el is of course different, but this is just for the example. List does not see this and Pair does not create elements, just copy them. Sure the user of a List of MagicStuff has some items to manage, and knows how to do it. As long as copy_el works, it is ok to 'List'. As long as show_el works List can do their job.

In fact pointers to these functions could be added to List and we would have something closer to C++.

Only the first five lines are different, since we need to build a pair of elements. The rest of the code is the same. The more important is the idea that the code for Pair and List is the same.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "list.h"

int main(void)
{
    Element* one     = make_el("Stack", 42);
    Element* other   = make_el("Overflow", -42);
    Pair*    my_pair = make_pair(one, other);
    delete_el(one);
    delete_el(other);
    show_pair(my_pair, "test_pair is ");

    List* my_list = make_list(5);  // capacity = 5
    show_list(my_list, "\n(list still empty) ");

    while (0 == insert_list(my_pair, my_list)) {};
    show_list(
        my_list, "\n(list now filled with same pair ");

    my_pair = delete_pair(my_pair);  // free test_pair
    my_list = delete_list(my_list);  // free list
    return 0;
}

outout for this example

test_pair is   [ ("Stack",42), ("Overflow",-42)]

(list still empty) [0/5] element(s) in list


(list now filled with same pair [5/5] element(s) in list
  [ ("Stack",42), ("Overflow",-42)]
  [ ("Stack",42), ("Overflow",-42)]
  [ ("Stack",42), ("Overflow",-42)]
  [ ("Stack",42), ("Overflow",-42)]
  [ ("Stack",42), ("Overflow",-42)]


    deleting 5 elements list

a simple implementation for the non-trivial Element

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

typedef struct
{
    char* name;
    int   code;
} Element;

Element* copy_el(Element*);
Element* delete_el(Element*);
int      show_el(Element*, const char*);

Element* copy_el(Element* orig)
{
    if (orig == NULL) return NULL;
    Element* one = malloc(sizeof(Element));
    if (one == NULL) return NULL;
    one->name = malloc(1 + strlen(orig->name));
    if (one->name == NULL)
    {
        free(one);
        return NULL;
    }
    strcpy(one->name, orig->name);
    one->code = orig->code;
    return one;
}

Element* delete_el(Element* orig)
{
    if (orig == NULL) return NULL;
    free(orig->name);
    free(orig);
    return NULL;
}

int show_el(Element* el, const char* msg)
{
    if (el == NULL) return -1;
    if (msg != NULL) printf("%s", msg);
    printf(" (\"%s\",%d)", el->name, el->code);
    return 0;
}

Element* make_el(const char* name, const int code)
{
    if (name == NULL) return NULL;
    Element* one = malloc(sizeof(Element));
    if (one == NULL) return NULL;
    one->name = malloc(1 + strlen(name));
    if (one->name == NULL)
    {
        free(one);
        return NULL;
    }
    strcpy(one->name, name);
    one->code = code;
    return one;
}```

### A note on *encapsulation* ###

Here in `insert_list` a pair is inserted
```C
    Pair* pair       = make_pair(p->first, p->second);

but the code to do it come from Pair implementation.

But there in make_pair

    one->first  = copy_el(a);
    one->second = copy_el(b);

We see that the user-provided copy creator is used.

a note on building this

• Each level of container adds in general a pair of files, a .h header and the implementation file .c.
• Each level sees only the one below and the code is just copied and adapted
• Is is normal to have a constructor, a destructor, a copy constructor, a print function, declared as here or following some pattern. It works fine in general, since is what is done in other fields and languages.

code and a visual studio project are available at GitHub on this link