0

I am currently taking CS50x and am on Pset5: Speller. My code works as intended, however when I run the check50 function my program fails Valgrind.

This is particularly confusing to me because when I run Valgrind on my program with the help50, it passes with no memory leaks. I ran help50 Valgrind on my program after changing the default dictionary to the dictionaries/small, and it still passed.

Her is my code and the error log I get from check50. I suspect my Load() function is the culprit, perhaps something to do with the fopen. I have wondered if I needed to add fclose() for the dictionary, but when I add that line to the end of Load() I get a "munmap_chunk(): invalid pointer" when I try to compile.

If anyone could help me out or had any idea where I should look I would be most grateful!

// Implements a dictionary's functionality

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

#include "dictionary.h"

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

// Number of bucket for Hash Table
const unsigned int N = 676;

// Hash table
node *table[N];

int word_counter = 0;

// Returns true if word is in dictionary, else false
bool check(const char *word)
{
    // TODO
    int index = hash(word);
    node *cursor = table[index];
    //printf("cursor: %s\n", cursor->word);

    while (cursor != NULL)
    {
        if (strcasecmp(word, cursor->word) == 0)
        {
            return true;
        }
    cursor = cursor->next;

    }

    return false;
}

// Hashes word to a number
unsigned int hash(const char *word)
{
    // TODO: Improve this hash function

    unsigned int index = 0;
    for (int i = 0; i < 26; i++)
    {
        if (word[0] == i + 'a' || word[0] == i + 'A')
        {
            index += i * 26;
        }

        if (word[1] == i + 'a' || word[1] == i + 'A')
        {
            index += i;
        }
    }

return index;
}

// Loads dictionary into memory, returning true if successful, else false
bool load(const char *dictionary)
{
    FILE *dict_file = fopen(dictionary, "r");
    if (dict_file == NULL)
    {
        return false;
    }

    table_clear();

    // Use fscanf() to look at each string in the dictionary and store to buffer
    char *buffer = malloc(sizeof(LENGTH + 1));
    if (buffer == NULL)
    {
        return false;
    }
    while (fscanf(dict_file, "%s", buffer) != EOF)
    {
        // Create a new memory space for each dictionary word, and store it here
        node *n = malloc(sizeof(node));
        if (n == NULL)
        {
            free(buffer);
            return false;
        }
        // Copy each word from buffer to the location (*dict_entry).word
        strcpy(n->word, buffer);
        n->next = NULL;

        // Use hash function to find the hash value of the dictionary word
        int index = hash(n->word);
        if (table[index] != NULL)
        {
            n->next = table[index];
        }
        table[index] = n;
        n = NULL;
        free(n);
        word_counter++;
    }
    //fclose(dict_file); <----- If activate, this line causes the munmap_chunk: invalid pointer error
    free(buffer);
    return true;
}

// Returns number of words in dictionary if loaded, else 0 if not yet loaded
unsigned int size(void)
{
    return word_counter;
}

// Unloads dictionary from memory, returning true if successful, else false
bool unload(void)
{
     for (int i = 0; i < N; i++)
     {
        node *cursor = table[i];
        while (cursor != NULL)
        {
            cursor = cursor->next;
            free(table[i]);
            table[i] = cursor;
        }

        free(cursor);
     }

    return true;
}

// Sets every value in table to NULL
void table_clear(void)
{
    for (int i = 0; i < N; i++)
    {
        table[i] = NULL;
    }
    return;
}

**Cause valgrind tests failed; see log for more information

LOG:

running valgrind --show-leak-kinds=all --xml=yes --xml-file=/tmp/tmpr2rknwok -- ./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...
Invalid write of size 1: (file: dictionary.c, line: 88)
Invalid read of size 1: (file: dictionary.c, line: 99)
472 bytes in 1 blocks are still reachable in loss record 1 of 1: (file: dictionary.c, line: 74)

See above for exlaination

1 Answer 1

0

There are two notable problems that I saw.

First, there's this in the valgrind report:

472 bytes in 1 blocks are still reachable in loss record 1 of 1: (file: dictionary.c, line: 74)

Going to line 74, we see this:

FILE *dict_file = fopen(dictionary, "r");

This is obvious. Where is dict_file closed? It's not, so the file pointer was left open and is the source of this memory leak. If files were repeatedly open and not closed, this would be a serious problem in a production system.

The next issue is much more subtle. There's a clue in the valgrind report:

Invalid write of size 1: (file: dictionary.c, line: 88)

Line 88 is this:

char *buffer = malloc(sizeof(LENGTH + 1));

How much space do you believe was allocated in this malloc call? Let's look carefully at what is going on here. The malloc call is allocating the sizeof(something). What is that something? It's an address of the variable LENGTH, plus 1. It is definitely not the value contained in LENGTH. Since an address is 8 bytes in a 64 bit system or 4 bytes in a 32 bit system, that's what's being allocated. in the IDE, it's allocating 4 bytes, not 46.

That will mean that the program will run with unpredictable results. Short words will be fine, but long words will lead to memory overwrites and may or may not cause errors and corrupted data.

The correct use would be this:

    char *buffer = malloc( LENGTH + 1 );

This addresses your memory leaks. As for any other issues that may exist, that's for another question. Happy coding! ;-)

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .