Why does free() of a struct result in segfault (wrong usage of pointers)?

Question

When I try to free my struct, the program crashes because of a segfault. Inspecting the program with valgrind I have found:

==9761== Invalid free() / delete / delete[] / realloc()
==9761==    at 0x484827F: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9761==    by 0x109242: destroyHashTable (hashtable.c:38)
==9761==    by 0x10942E: main (hashtable_main.c:17)
==9761==  Address 0x1ffefffa70 is on thread 1's stack
==9761==  in frame #2, created by main (hashtable_main.c:7)

I cannot really say anything more useful than having no idea, how to solve it. The crash happens during the free(ht) in destroyHashTable(ht) in hashtable.c. What am I doing wrong?

Below the code hashTable_main.c:

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


#include "hashtable.h"

int main() {

    hashTable* ht = NULL;

    initHashTable(&ht);

    int totalColCount = 0;

    totalColCount += addHashTableEntry(&ht, "PRPR2");

    destroyHashTable(&ht);

    return EXIT_SUCCESS;
}

hashtable.c:

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


#include "hashtable.h"

/* private internal API */
int hash_funktion(char *string);
hashtableEntry* createTableEntry(char* newKey) ;
/* end of private internal API */


int hash_funktion(char *string) {
    unsigned int hash_adresse;
    unsigned char *pointer;
    hash_adresse = 0;
    pointer = (unsigned char *) string;
    while(*pointer != '\0') {
        hash_adresse = 19 * hash_adresse + *pointer;
        pointer++;
    }
    return hash_adresse % MAX_HASH;
}

hashtableEntry* createTableEntry(char* newKey) {
     hashtableEntry* e = (hashtableEntry*) malloc (sizeof(hashtableEntry));
     e->hashKey = newKey;
     return e;
}

void initHashTable(hashTable* ht) {
    ht = (hashTable*) malloc (sizeof (struct hashTable));
    ht->table = (hashtableEntry*) malloc (MAX_HASH * sizeof (hashtableEntry));
}

void destroyHashTable(hashTable* ht) {
    if (ht) {
        free(ht);
        ht = NULL;
    }
}

int  addHashTableEntry(hashtableEntry* ht, char* keyValue) {
    hashtableEntry *e = createTableEntry(keyValue);

    int colCounter = 0;

    int hashValue = hash_funktion(keyValue);

    if (ht[hashValue].hashKey == NULL) {
        ht[hashValue] = *e;
        return 0;
    } else {
        int newVal = (hashValue + 1) % MAX_HASH;
        colCounter++;
        while (ht[newVal].hashKey != NULL && newVal != hashValue ) {
            newVal = (newVal + 1) % MAX_HASH;
            colCounter++;
        }
        if (newVal != hashValue) {
            ht[newVal] = *e;  
            return colCounter;      
        } else {
            return -1;
        }
    }
}

bool searchValue(hashtableEntry* ht, char* searchValue) {    
    for (int i = 0; i < MAX_HASH; i++)
    {
        if(ht[i].hashKey == searchValue) {
            return true;
        }
    }
    return false;
}

and hashtable.h:

#pragma once

#define MAX_HASH 20
#include <stdbool.h>

typedef struct hashtableEntry {
    char* hashKey;
} hashtableEntry;

typedef struct hashTable {
    hashtableEntry* table;
    int elemCount;
} hashTable;

void initHashTable(hashTable* ht);

void destroyHashTable(hashTable* ht);

int  addHashTableEntry(hashtableEntry* ht, char* keyValue);

bool searchValue(hashtableEntry* ht, char* searchValue);

Answer 1

There never was a hashtable to begin with. The issue lies in initHashTable. It should be accepting a double pointer since it is given a pointer to a pointer it should initialize. The reason it can segfault despite the check in destroyHashTable is that the pointer is left uninitialized and may be non-zero at the start of program execution.

void initHashTable(hashTable** ht) {
    *ht = (hashTable*) malloc (sizeof (struct hashTable));
    (*ht)->table = (hashtableEntry*) malloc (MAX_HASH * sizeof (hashtableEntry));
}

You may find it easier to instead return the newly created hash table. This better expresses that initHashTable is giving you a new hashTable * value.

hashTable *initHashTable() {
    hashTable *ht = (hashTable *) malloc (sizeof (struct hashTable));
    ht.table = (hashtableEntry *) malloc (MAX_HASH * sizeof (hashtableEntry));
    return ht;
}

There are also a bunch of other places where pointers are not handled correctly.

void doThing(Foo *foo) {
    // This changes foo, but not the data foo points to.
    foo = something;
    // This changes the data foo points to
    *foo = someOtherThing;
}

void doStuff() {
    Foo *foo;

    // This is incorrect since it creates a double pointer. doThing would need to
    // be defined as "void doThing(Foo **foo)" to be correct.
    doThing(&foo);

    // Instead we can just pass the existing pointer
    doThing(foo);


    // We only need to create a reference if the value does not start out as a pointer
    Foo bar;
    doThing(&bar);
}