jakewies jakewies - 4 months ago 16
C Question

Valgrind newbie, can't seem to make it happy

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!

Answer

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.