Thread: C - freeing memory to avoid leak

I have a program below that manages a library full of Books and when I test for memory leaks using valgrind and then enter 'CTRL+Z' to quit, it says there are hundreds of bytes lost. When the program exits naturally as its supposed to rather than me forcing an exit however, there are 0 bytes lost.

I deallocated my code using 'free' at the end of main.c and I was wondering if there's something wrong with the way I'm doing it? I was told every time I use 'malloc' I also have to use the 'free keyword' and aside from the 2 times I use malloc in main.c, I also use malloc in addBook() in library.c when initializing the Book object. I'm not sure if this is where the memory leak is occurring because I didn't use free in this function?

I included my header file as well so you can see the kind of data types/arrays I'm creating.

main.c

Code:

    int main(){


        LibraryType* library  = malloc(sizeof(LibraryType)); 
        BookCollectionType* book = malloc(sizeof(BookCollectionType));
        
        addBook(book, "The Hunger Games", 2012, 405.5); 
        addBook(book, "Twilight", 2007, 302); 
        addBook(book, "The Martian", 2015, 203.2); 


        }


    //.....end of program.....//


        for(int i = 0; i < MAX_BOOKS; ++i){
            free(p->books[i]);
            free(library->bookCollection[i]);
        }
        free(book);
        free(library);


    }

library.c

Code:

    int addBook(BookCollectionType* bookCollection, char* t, int c, float p){


        bookCollection->books[bookCollection->numBooks] = malloc(sizeof(BookType));
        bookCollection->books[bookCollection->numBooks]->title = t;
        bookCollection->books[bookCollection->numBooks]->numCopies = c;
        bookCollection->books[bookCollection->numBooks]->pages = pages;
        bookCollection->numBooks++;
        return 1;


    }

defs.h

Code:

    typedef struct{
        char* title;
        int year;
        int numCopies;
        float pages;
    }BookType;




    typedef struct {
        BookType* books[MAX_PAGES]; 
        int numBooks;
    }BookCollectionType;




    typedef struct{
        char* libraryName;
        BookCollectionType* bookCollection[MAX_BOOKS];
    }LibraryType;

I would focus on these lines first:

Code:

LibraryType* library = malloc(sizeof(LibraryType));
BookCollectionType* book = malloc(sizeof(BookCollectionType));

addBook(book, "The Hunger Games", 2012, 405.5);
addBook(book, "Twilight", 2007, 302);
addBook(book, "The Martian", 2015, 203.2);

Basically, why do you have a library variable, and then also have a book variable?

I would expect something like this instead:

Code:

LibraryType* library = malloc(sizeof(LibraryType));
library->bookCollection[0] = malloc(sizeof(BookCollectionType));
library->numCollections = 1;
addBook(library->bookCollection[0], "The Hunger Games", 2012, 405.5);
addBook(library->bookCollection[0], "Twilight", 2007, 302);
addBook(library->bookCollection[0], "The Martian", 2015, 203.2);

So the idea is that you have a library that consists of one or more book collections. You create a book collection and add three books to that collection. Notice that I added a member named numCollections to LibraryType, so it should look like this:

Code:

typedef struct{
    char* libraryName;
    BookCollectionType* bookCollection[MAX_BOOKS];
    int numCollections;
}LibraryType;

This is akin to what you did with BookCollectionType.

Having done this, you do something with the library, e.g., print the books, and then when done, you clean up:

Code:

for (int i = 0; i < library->numCollections; i++)
{
    for (int j = 0; j < library->bookCollection[i]->numBooks; j++)
    {
        free(library->bookCollection[i]->books[j]);
    }
    free(library->bookCollection[i]);
}
free(library);

Note that your named constants are poorly named:

• MAX_BOOKS sounds like the maximum number of books, but you're using it to mean the maximum number of book collections, so it would be better named MAX_BOOK_COLLECTIONS
• MAX_PAGES sounds like the maximum number of pages in a book, but you're using it to mean the maximum number of books in a collection, so it would be better named MAX_BOOKS

If it frees all the memory you allocated during a normal exit, that should be fine.

Freeing all the memory in various exception cases is rather more involved.