Pset5 - Speller - memory leak?

Question

My programme runs and I'm passing all the checks bar the last one. When I run the check with --log I get this response:

:( program is free of memory errors
    valgrind tests failed; rerun with --log for more information.
    running valgrind --show-leak-kinds=all --xml=yes --xml-file=/tmp/tmpm_fk5rjy -- ./speller substring/dict substring/text...
    checking for output "MISSPELLED WORDS\n\nca\ncats\ncaterpill\ncaterpillars\n\nWORDS MISSPELLED:     4\nWORDS IN DICTIONARY:  2\nWORDS IN TEXT:        6\n"...
    checking that program exited with status 0...
    checking for valgrind errors...
        112 bytes in 2 blocks are still reachable in loss record 1 of 1: (file: dictionary.c, line: 70)

But when I run valgrind, I don't seem to be leaking any memory?

==5258== 
==5258== HEAP SUMMARY:
==5258==     in use at exit: 8,013,096 bytes in 143,091 blocks
==5258==   total heap usage: 143,096 allocs, 5 frees, 8,023,416 bytes allocated
==5258== 
==5258== 8,013,096 bytes in 143,091 blocks are still reachable in loss record 1 of 1
==5258==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5258==    by 0x4011B2: load (dictionary.c:70)
==5258==    by 0x400964: main (speller.c:40)
==5258== 
==5258== LEAK SUMMARY:
==5258==    definitely lost: 0 bytes in 0 blocks
==5258==    indirectly lost: 0 bytes in 0 blocks
==5258==      possibly lost: 0 bytes in 0 blocks
==5258==    still reachable: 8,013,096 bytes in 143,091 blocks
==5258==         suppressed: 0 bytes in 0 blocks
==5258== 
==5258== For counts of detected and suppressed errors, rerun with: -v
==5258== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

The issue check50 is giving me seems to be with my load function specifically the line where I allocate memory.

bool load(const char *dictionary)
{
    FILE *dict = fopen(dictionary, "r");                    // open dictionary file
    if (!dict)                                              // check for errors in opening file
    {
        return 1;                                           // if errors, exit programme
    }
    char newword[LENGTH + 1];                               // create variable to store newwords
    while (fscanf(dict, "%s", newword) != EOF)              // read words out of dictionary one at a time until the end of file
    {
       node *n = malloc(sizeof(node));                      // allocate memory for a new node
       if(n == NULL)
       {
           unload();
           return 1;                                        // exit programme if malloc fails
       }
       int nloc = hash(newword);                            // hash word and store hash value in nloc
       strcpy(n->word, newword);                            // insert newword in node
       n->next = table[nloc];                               // insert the new table loc in next
       table[nloc] = n;                                     // insert newword at location nloc in the hash table
       dict_words++;                                        // tracks words loaded into dictionary
    }
    fclose(dict);
    return true;
}

And as it has to do with a memory leak, here's my unload function.

bool unload(void)
{
    for (int i = 0; i < N -1; i++)
    {
        for (node *cursor = table[i]; cursor != NULL; cursor = cursor->next)
        {
            if (cursor == NULL)
            {
                free(cursor);
                return false;
            }
            else
            {
                node *tmp = cursor;
                if (tmp == NULL)
                {
                    free(tmp);
                    return false;
                }
            }
        }
    }
    return true;
}

If anyone can offer some insight I'd be most grateful!

Answer 1

Since all the memory is still reachable, it's a good bet that it wasn't freed correctly in unload.

There are several issues with the unload function. First, the two return statements inside the for loop will cause the unload function to terminate prematurely, before it has traversed the tree. Get rid of them.

Next, there's nothing in unload() that actually frees any allocated memory (i.e., a pointer that contains a valid address.)

What concerns me is this. Valgrind reports some memory is freed. Where is this happening? Could be normal/ok, or could indicate another issue, perhaps nodes being freed prematurely.

Further, looking at the interaction of the for loop and the else clause, if the code frees tmp (which contains the same address as cursor), the loop will fail on the next pass because cursor-next will no longer exist. The code needs to get the address of the next node BEFORE freeing the current node!

Finally, there are 112 bytes still reachable in 2 blocks. That's two blocks of 56 bytes. Anytime a block of 56 bytes is involved, it's usually something to do with a file pointer. When they're "still reachable" it's usually because a file wasn't closed before the program terminated. Did you close the dictionary and the test files? Or is the program terminating prematurely?

Programming note: If a pointer is set to null, free(pointer) does nothing. There's nothing to free, so such a statement is dead code and can be removed.