Search code examples
cmemory-leakshashtablecs50

cs50 pset5 memory leak when in unload function, when freeing memory


I am currently working on cs50 pset5.

I have encountered a memory leak when I attempt to free all the memory I have located in the program.
The error seems to occur in my unload function, where it malloc for pointer cursor.
If anyone can provide me directions on how to approach this issue, please let me know.
I have also provided comments to my code to highlight what it is doing.

// Represents a node in a hash table
typedef struct node
{
    char word[LENGTH + 1];
    struct node *next;
}
node;

// Number of buckets in hash table
const unsigned int N = 50;

node *table[N] = {NULL};

// Unloads dictionary from memory, returning true if successful else false
bool unload(void)
{
    // allocate memory for struct type `node` pointer 
    node *cursor = (node*)malloc(sizeof(node));                 // MEMORY LEAK happens here !!!! :(
    node *tmp = (node*)malloc(sizeof(node));

    // if memory cannot be allocated successfully, return false
    if (cursor == NULL || tmp == NULL)
    {
        return false;
    }

    // iterate the table for N'times (N = 50, for now)
    for (int i = 0; i < N; i++)
    {
        // if the table has nothing inside the i'th bucket, return false 
        // this means, table has no linked list inside i'th bucket 
        if (table[i] == NULL)
        {
            continue;
        }
        else
        {
            // assign tmp pointer to the head node, and cursor to anything next to tmp 
            tmp = table[i];
            cursor = tmp;
            cursor = cursor->next;
            // free memory for tmp pointer 
            free(tmp);

            // continue traversing the linked list until cursor pointer points to NULL (end of the linked list)
            while (cursor != NULL)
            {
                tmp = cursor;
                cursor = cursor->next;
                free(tmp);
            }
        }
    }

    // free memory for cursor
    free(cursor);
    return true;
}

The error I get, after running, help50 valgrind ./speller dictionaries/small texts/cat.txt.

==14685== Memcheck, a memory error detector
==14685== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==14685== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==14685== Command: ./speller dictionaries/small texts/cat.txt
==14685== 
32
==14685== Invalid write of size 1
==14685==    at 0x40121F: hash (dictionary.c:67)
==14685==    by 0x401354: load (dictionary.c:99)
==14685==    by 0x400A2B: main (speller.c:41)
==14685==  Address 0x55cc88a is 2 bytes after a block of size 8 alloc'd
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x4011D5: hash (dictionary.c:61)
==14685==    by 0x401354: load (dictionary.c:99)
==14685==    by 0x400A2B: main (speller.c:41)
==14685== 
==14685== Invalid read of size 1
==14685==    at 0x40122C: hash (dictionary.c:68)
==14685==    by 0x401354: load (dictionary.c:99)
==14685==    by 0x400A2B: main (speller.c:41)
==14685==  Address 0x55cc88a is 2 bytes after a block of size 8 alloc'd
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x4011D5: hash (dictionary.c:61)
==14685==    by 0x401354: load (dictionary.c:99)
==14685==    by 0x400A2B: main (speller.c:41)
==14685== 
19

MISSPELLED WORDS

47
A
32
20
is
27
not
47
a
==14685== Invalid write of size 1
==14685==    at 0x40121F: hash (dictionary.c:67)
==14685==    by 0x401124: check (dictionary.c:32)
==14685==    by 0x400D50: main (speller.c:114)
==14685==  Address 0x55cdf9a is 2 bytes after a block of size 8 alloc'd
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x4011D5: hash (dictionary.c:61)
==14685==    by 0x401124: check (dictionary.c:32)
==14685==    by 0x400D50: main (speller.c:114)
==14685== 
==14685== Invalid read of size 1
==14685==    at 0x40122C: hash (dictionary.c:68)
==14685==    by 0x401124: check (dictionary.c:32)
==14685==    by 0x400D50: main (speller.c:114)
==14685==  Address 0x55cdf9a is 2 bytes after a block of size 8 alloc'd
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x4011D5: hash (dictionary.c:61)
==14685==    by 0x401124: check (dictionary.c:32)
==14685==    by 0x400D50: main (speller.c:114)
==14685== 
19

WORDS MISSPELLED:     4
WORDS IN DICTIONARY:  2
WORDS IN TEXT:        6
TIME IN load:         0.03
TIME IN check:        0.00
TIME IN size:         0.00
TIME IN unload:       0.00
TIME IN TOTAL:        0.03

==14685== 
==14685== HEAP SUMMARY:
==14685==     in use at exit: 1,000 bytes in 9 blocks
==14685==   total heap usage: 23 allocs, 14 frees, 10,944 bytes allocated
==14685== 
==14685== 56 bytes in 1 blocks are definitely lost in loss record 1 of 4
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x4013F1: unload (dictionary.c:128)
==14685==    by 0x400ED0: main (speller.c:154)
==14685== 
==14685== 56 bytes in 1 blocks are definitely lost in loss record 2 of 4
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x4013FF: unload (dictionary.c:129)
==14685==    by 0x400ED0: main (speller.c:154)
==14685== 
==14685== 336 bytes in 6 blocks are definitely lost in loss record 3 of 4
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x401131: check (dictionary.c:33)
==14685==    by 0x400D50: main (speller.c:114)
==14685== 
==14685== 552 bytes in 1 blocks are still reachable in loss record 4 of 4
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x5258E49: __fopen_internal (iofopen.c:65)
==14685==    by 0x5258E49: fopen@@GLIBC_2.2.5 (iofopen.c:89)
==14685==    by 0x4012DE: load (dictionary.c:83)
==14685==    by 0x400A2B: main (speller.c:41)
==14685== 
==14685== LEAK SUMMARY:
==14685==    definitely lost: 448 bytes in 8 blocks
==14685==    indirectly lost: 0 bytes in 0 blocks
==14685==      possibly lost: 0 bytes in 0 blocks
==14685==    still reachable: 552 bytes in 1 blocks
==14685==         suppressed: 0 bytes in 0 blocks
==14685== 
==14685== For counts of detected and suppressed errors, rerun with: -v
==14685== ERROR SUMMARY: 15 errors from 7 contexts (suppressed: 0 from 0)

Asking for help...

==14685== 56 bytes in 1 blocks are definitely lost in loss record 1 of 4
==14685==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14685==    by 0x4013F1: unload (dictionary.c:128)
==14685==    by 0x400ED0: main (speller.c:154)

Looks like your program leaked 56 bytes of memory. Did you forget to free memory that you allocated via malloc? Take a closer look at line 128 of dictionary.c.

So why does memory leak happen?
A memory leak happens when a programmer creates a memory from heap but forgets to free it.

However, in my code, I have free'd both cursor and tmp pointer after it's used.
I do not know why I am encountering the error, although I am freeing it at the end of the function.


Solution

  • Yes, "MEMORY LEAK happens here !!!!" as you say.

    You allocated two buffers, but later their pointers are overwritten by table[i] without being freed. This is memory leak.

    To avoid memory leak, stop allocating unused buffers.

    The part

        // allocate memory for struct type `node` pointer 
        node *cursor = (node*)malloc(sizeof(node));                 // MEMORY LEAK happens here !!!! :(
        node *tmp = (node*)malloc(sizeof(node));
    
        // if memory cannot be allocated successfully, return false
        if (cursor == NULL || tmp == NULL)
        {
            return false;
        }
    

    should be just

        // allocate memory for struct type `node` pointer 
        node *cursor;
        node *tmp;
    

    The memory for pointer is allocated on the stack.

    Also the part

        // free memory for cursor
        free(cursor);
    

    should be removed because the list is already freed and the memory for cursor will be automatically freed (from the stack) on returning from the function.