Pset 5 Help understanding Valgrind output

Question

I was wondering if anyone could help me with interpreting the Valgrind error to trouble shoot. There are no memory leaks, but I am getting this error "Conditional jump or move depends on uninitialized values" along with others. Apologies if it seems like a lot, I am just a little lost!

dictionary.c:45 refers to the line in the function check, starting with "if(strcasecmp(word, checktemp->word) !=0)"

line 32 is slightly before that "node *checktemp = malloc(sizeof(node))

Thank you very much, really appreciate it.

The Valgrind error is as follows, followed by my code:

==18498== Conditional jump or move depends on uninitialised value(s)
==18498==    at 0x520A60F: tolower (ctype.c:46)
==18498==    by 0x4C33629: strcasecmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4010D9: check (dictionary.c:45)
==18498==    by 0x400C89: main (speller.c:112)
==18498==  Uninitialised value was created by a heap allocation
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401071: check (dictionary.c:32)
==18498==    by 0x400C89: main (speller.c:112)
==18498== 
==18498== Use of uninitialised value of size 8
==18498==    at 0x520A623: tolower (ctype.c:46)
==18498==    by 0x4C33629: strcasecmp (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4010D9: check (dictionary.c:45)
==18498==    by 0x400C89: main (speller.c:112)
==18498==  Uninitialised value was created by a heap allocation
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401071: check (dictionary.c:32)
==18498==    by 0x400C89: main (speller.c:112)
==18498== 
==18498== Invalid read of size 8
==18498==    at 0x4010EB: check (dictionary.c:47)
==18498==    by 0x400C89: main (speller.c:112)
==18498==  Address 0x6743c70 is 48 bytes inside a block of size 56 free'd
==18498==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4012CE: load (dictionary.c:123)
==18498==    by 0x400964: main (speller.c:40)
==18498==  Block was alloc'd at
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401234: load (dictionary.c:99)
==18498==    by 0x400964: main (speller.c:40)
==18498== 
A
cat
is
not
a
caterpillar
==18498== Invalid read of size 8
==18498==    at 0x401344: unload (dictionary.c:147)
==18498==    by 0x400E09: main (speller.c:152)
==18498==  Address 0x6742ef0 is 48 bytes inside a block of size 56 free'd
==18498==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4012CE: load (dictionary.c:123)
==18498==    by 0x400964: main (speller.c:40)
==18498==  Block was alloc'd at
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401234: load (dictionary.c:99)
==18498==    by 0x400964: main (speller.c:40)
==18498== 
==18498== Invalid read of size 8
==18498==    at 0x401363: unload (dictionary.c:151)
==18498==    by 0x400E09: main (speller.c:152)
==18498==  Address 0x6742df0 is 48 bytes inside a block of size 56 free'd
==18498==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x4012CE: load (dictionary.c:123)
==18498==    by 0x400964: main (speller.c:40)
==18498==  Block was alloc'd at
==18498==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18498==    by 0x401234: load (dictionary.c:99)
==18498==    by 0x400964: main (speller.c:40)
==18498== 

WORDS MISSPELLED:     6
WORDS IN DICTIONARY:  143091
WORDS IN TEXT:        6
TIME IN load:         1.77
TIME IN check:        0.04
TIME IN size:         0.00
TIME IN unload:       0.06
TIME IN TOTAL:        1.87

==18498== 
==18498== HEAP SUMMARY:
==18498==     in use at exit: 0 bytes in 0 blocks
==18498==   total heap usage: 143,102 allocs, 143,102 frees, 8,023,752 bytes allocated
==18498== 
==18498== All heap blocks were freed -- no leaks are possible
==18498== 
==18498== For counts of detected and suppressed errors, rerun with: -v
==18498== ERROR SUMMARY: 195546 errors from 5 contexts (suppressed: 0 from 0)

Asking for help...

==18498== Conditional jump or move depends on uninitialised value(s)

Looks like you're trying to use a variable that might not have a value? Take a closer look at line 45 of dictionary.c.

// Implements a dictionary's functionality

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

// 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; //change variable as needed

// Hash table
node *table[N];

int wordcount[1];

// Returns true if word is in dictionary else false
bool check(const char *word)
{
    int hcheck = hash(word);
    node *checktemp = malloc(sizeof(node));
    checktemp->next = NULL;

    if(table[hcheck] == NULL)
    {
        free(checktemp);
        return false;
    }
    else
    {
        checktemp->next = table[hcheck];
        while(checktemp->next != NULL)
        {
            if(strcasecmp(word, checktemp->word) != 0)
            {
                checktemp->next = checktemp->next->next;
            }
            else
            {
                return true;
            }
        }

    }
    
    free(checktemp);
    return false;
}

// Hashes word to a number
unsigned int hash(const char *word)
{
    // Hashes word to a number
    // Source: djib2 by Dan Bernstein (http://www.cse.yorku.ca/~oz/hash.html)
    
    unsigned long hash = 5381;
    int c = *word;
    c = tolower(c);

    while (*word != 0)

    {
        hash = ((hash << 5) + hash) + c;
        c = *word++;
        c = tolower(c);
    }

    return hash % N;
}

// Loads dictionary into memory, returning true if successful else false
bool load(const char *dictionary)
{
    // TODO

    FILE *file = fopen(dictionary, "r");
    if (file == NULL)
    {
        unload();
        return false;
    }

    char wordcopy[LENGTH+1];
    int count = 0;

    while(fscanf(file, "%s", wordcopy) != EOF)
    {
        node *node1 = malloc(sizeof(node));
        
        if(node1 == NULL)
        {
            unload();
            return false;
        }
        else
        {
            
            strncpy(node1->word, wordcopy, sizeof(wordcopy));

            int hash1 = hash(wordcopy);

            if(table[hash1] == NULL)
            {
                table[hash1] = node1;
            }
            else
            {
                node1->next = table[hash1];
                table[hash1] = node1;
            }
            
            free(node1);
            count++;
        }
    }

    fclose(file);
    wordcount[0] = count;
    return true;
}

// Returns number of words in dictionary if loaded else 0 if not yet loaded
unsigned int size(void)
{
    // TODO
    return wordcount[0];
}

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

Answer 1

if(strcasecmp(word, checktemp->word) !=0)

This means that you are checking that if both words are not equal,

 // do something

Infact you should try to look if both words are matched,

 // do something

If you wanna prefer first style (mentioned above) then you need to return false so that function won't proceed further and word is not in the dictionary

If you wanna perfere second style (mentioned above) then you need to check if,

 if(strcasecmp(words_to_be_matched) == 0)

{
// do something return true; }

If this helps be sure to tick the answer to keep other smiling too :)

Answer 2

So, couple of bugs I noted.

• Why are you declaring array of word count? You just need to update the global word_count when a new word from the dictionary has been successfully hashed in the linked list
• Why are you calling unload() function inside your load function? Suppose that no list is created then unload function will fail to execute and return NULL
• Why are you freeing the node after a successful hash? If you free the node then linked list won't be updated with new words and unload function can unload from the memory because everytime you hashed the node in the table, you are also freeing the node
• Why are you mallocing the space for temporary cursor when its work is only to point at head of linked list to check the word? It should be a node type pointer to point to the head of the hash table