3

I'm working through the CS50 course online, and the task at hand is to load a dictionary into memory (I'm using a chained hash table data structure), and check the contents of a text file against the words in the dictionary. I wrote a load() function that loads the dictionary file and stores every word into memory. The function returns true when all words are loaded. Valgrind shows no memory leaks. The problem is I have a ridiculous amount of read/write errors. It helped me pinpoint where they seem to be coming from and I've been scouring the interwebs but can't seem to make heads or tails of it.

Here's the relevant code:

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

#define HASH_SIZE 10
#define LENGTH 45

typedef struct node
{
    char* word;
    struct node* next;
} node;

// Globals
node* hashTable[HASH_SIZE];
unsigned int dictionary_size = 0;

/**
 * Loads dictionary into memory.  Returns true if successful else false.
 **/
bool load(const char* dictionary)
{
    // Initialize variables
    char currentWord[LENGTH + 1];
    int tableIndex;

    // Open dictionary
    FILE* dict = fopen(dictionary, "r");
    if (dict == NULL)
        return false;

    while(fscanf(dict, "%s", currentWord) == 1)
    {
        // Get hash value of word
        tableIndex = hash(currentWord);

        // Initialize new node
        node* newNode = malloc(sizeof(node));
        newNode->word = malloc((strlen(currentWord) + 1) * sizeof(char));
        if (newNode == NULL || newNode->word == NULL)
        {
            printf("Error: Out of memory\n");
            return false;
        }

        // Copy word into new node
        strcpy(newNode->word, currentWord);
        newNode->next = NULL;

        // If no collision, hash word into head of list
        if (hashTable[tableIndex] == NULL)
            hashTable[tableIndex] = newNode;

        // Create a pointer and move down list
        else
        {
            node* ptrNode = hashTable[tableIndex];

            while (ptrNode->next != NULL)
                ptrNode = ptrNode->next;

            // Append node to end of linked list
            ptrNode->next = newNode;
        }

        // Increase dictionary size
        dictionary_size++;

        // Free word member before actual node
        free(newNode->word);
        free(newNode);
    }

    // Close dictionary and return true
    fclose(dict);
    return true;
}

And my Valgrind:

==32487== Invalid read of size 4
==32487==    at 0x8048989: load (my_new_test.c:120)
==32487==    by 0x804873D: main (my_new_test.c:53)
==32487==  Address 0x423b30c is 4 bytes inside a block of size 8 free'd
==32487==    at 0x402B3D8: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-   linux.so)
==32487==    by 0x80489D3: load (my_new_test.c:132)
==32487==    by 0x804873D: main (my_new_test.c:53)
==32487== 
==32487== Invalid write of size 4
==32487==    at 0x80489AA: load (my_new_test.c:124)
==32487==    by 0x804873D: main (my_new_test.c:53)
==32487==  Address 0x423b30c is 4 bytes inside a block of size 8 free'd
==32487==    at 0x402B3D8: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==32487==    by 0x80489D3: load (my_new_test.c:132)
==32487==    by 0x804873D: main (my_new_test.c:53)
==32487== 
==32487== Invalid read of size 4
==32487==    at 0x8048999: load (my_new_test.c:121)
==32487==    by 0x804873D: main (my_new_test.c:53)
==32487==  Address 0x423bb24 is 4 bytes inside a block of size 8 free'd
==32487==    at 0x402B3D8: free (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==32487==    by 0x80489D3: load (my_new_test.c:132)
==32487==    by 0x804873D: main (my_new_test.c:53)

==32487== HEAP SUMMARY:
==32487==     in use at exit: 0 bytes in 0 blocks
==32487==   total heap usage: 286,183 allocs, 286,183 frees, 2,584,308 bytes        allocated
==32487== 
==32487== All heap blocks were freed -- no leaks are possible
==32487== 
==32487== For counts of detected and suppressed errors, rerun with: -v
==32487== ERROR SUMMARY: 10000000 errors from 3 contexts (suppressed: 0 from  0)

I'm still very raw with Valgrind but from what I gather my issue seems to be inside the else statement, mainly the pointer created (node* ptrNode). Can anyone see something I'm not seeing? Any help would be greatly appreciated!

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
jakewies
  • 342
  • 6
  • 18
  • The code you show will not even compile. – Some programmer dude Jun 14 '15 at 18:40
  • Why do you say that? It compiles and runs for me. I left out the main function and only provided the load function – jakewies Jun 14 '15 at 18:46
  • 2
    It's a mistake to free the memory you just allocated. Why did you do that? You must wait until the hash table's use is finished. It's complaining because you do that and then try to use the pointers later. The other mistake (though it's not causing your current problem) is trying to do 2 null checks at once. If the first malloc failed, the null check will cause a seg fault. Use two separate checks. – Gene Jun 14 '15 at 18:46
  • I edited your code to get it to compile. In the future if you edit your code before posting it you need to check that it still works (or doesn't work) as expected. – John Kugelman Jun 14 '15 at 18:48
  • 1
    @Gene That's a good answer. You should post it as one! – John Kugelman Jun 14 '15 at 18:48
  • What did you edit? The function I provided compiled and ran for me. EDIT: again I've only provided the function (left out the #includes and main(). I didn't think to post everything lol) Thank you though – jakewies Jun 14 '15 at 18:49
  • @Gene thank you I'm taking your answer into account right now. – jakewies Jun 14 '15 at 18:49
  • The `typedef` was in the wrong place, was missing a semi-colon, and was spelled wrong (`typdef`). – John Kugelman Jun 14 '15 at 18:56
  • haha I apologize. Need to take a break – jakewies Jun 14 '15 at 18:57
  • @Gene, I shouldn't be freeing the nodes at the end of each iteration of the while loop? Because I've assigned hashTable[tableIndex] = newNode and if I were to free the node i'd be losing the data? – jakewies Jun 14 '15 at 19:01
  • It didn't compiler *before* your edits. – Some programmer dude Jun 14 '15 at 19:03
  • Good lord I think I realize my issue. I'm trying to free the nodes I'm creating to hold the words in my dictionary inside of my load() function. But I need these nodes to check the words of my text file. I should free the nodes in my unload() function later in my code. – jakewies Jun 14 '15 at 19:03

1 Answers1

3

The problem is most likely these two lines:

free(newNode->word);
free(newNode);

You just added the node into your hash-table, then you immediately free it, making the pointers in the hash-table invalid as they now point to the free'd data.

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
  • Ya I was getting way too far ahead of myself. Freeing the nodes in my load function wouldn't allow me to even check the words in the text file against the words I'm loading in from the dictionary. I just started learning about malloc and I think I got a little 'free' happy! – jakewies Jun 14 '15 at 19:06