CODEWITHSUNDEEP
cpointershashtableresize
pori
pori

Reputation: 83

Freeing memory of used data leads to Segmentation Fault

I wrote a hashtable and it basically consists of these two structures:

typedef struct dictEntry {
    void *key;
    void *value;
    struct dictEntry *next;
} dictEntry;

typedef struct dict {
    dictEntry **table;
    unsigned long size;
    unsigned long items;
} dict;

dict.table is a multidimensional array, which contains all the stored key/value pair, which again are a linked list.

If half of the hashtable is full, I expand it by doubling the size and rehashing it:

dict *_dictRehash(dict *d) {
    int i;
    dict *_d;
    dictEntry *dit;

    _d = dictCreate(d->size * 2);

    for (i = 0; i < d->size; i++) {
        for (dit = d->table[i]; dit != NULL; dit = dit->next) {
            _dictAddRaw(_d, dit);
        }
    }

    /* FIXME memory leak because the old dict can never be freed */
    free(d); // seg fault

    return _d;
}

The function above uses the pointers from the old hash table and stores it in the newly created one. When freeing the old dict d a Segmentation Fault occurs.

How am I able to free the old hashtable struct without having to allocate the memory for the key/value pairs again?

Edit, for completness:

dict *dictCreate(unsigned long size) {
    dict *d;

    d = malloc(sizeof(dict));
    d->size  = size;
    d->items = 0;
    d->table = calloc(size, sizeof(dictEntry*));

    return d;
}

void dictAdd(dict *d, void *key, void *value) {
    dictEntry *entry;

    entry = malloc(sizeof *entry);

    entry->key   = key;
    entry->value = value;
    entry->next  = '\0';

    if ((((float)d->items) / d->size) > 0.5) d = _dictRehash(d);

    _dictAddRaw(d, entry);
}

void _dictAddRaw(dict *d, dictEntry *entry) {
    int index = (hash(entry->key) & (d->size - 1));

    if (d->table[index]) {
        dictEntry *next, *prev;

        for (next = d->table[index]; next != NULL; next = next->next) {
            prev = next;

        }

        prev->next = entry;
    } else {
        d->table[index] = entry;
    }
    d->items++;
}

Upvotes: 4

Views: 211

Answers (4)

kjw0188
kjw0188

Reputation: 3685

To clarify Graham's point, you need to pay attention to how memory is being accessed in this library. The user has one pointer to their dictionary. When you rehash, you free the memory referenced by that pointer. Although you allocated a new dictionary for them, the new pointer is never returned to them, so they don't know not to use the old one. When they try to access their dictionary again, it's pointing to freed memory.

One possibility is not to throw away the old dictionary entirely, but only the dictEntry table you allocated within the dictionary. That way your users will never have to update their pointer, but you can rescale the table to accomodate more efficient access. Try something like this:

void _dictRehash(dict *d) {
    printf("rehashing!\n");
    int i;
    dictEntry *dit;

    int old_size = d->size;
    dictEntry** old_table = d->table;
    int size = old_size * 2;

    d->table = calloc(size, sizeof(dictEntry*));
    d->size = size;
    d->items = 0;

    for (i = 0; i < old_size; i++) {
        for (dit = old_table[i]; dit != NULL; dit = dit->next) {
            _dictAddRaw(d, dit);
        }
    }

    free(old_table);
    return;

}

As a side note, I'm not sure what your hash function does, but it seems to me that the line

int index = (hash(entry->key) & (d->size - 1));

is a little unorthodox. You get a hash value and do a bitwise and with the size of the table, which I guess works in the sense that it will be guaranteed to be within (I think?) [0, max_size), I think you might mean % for modulus.

Upvotes: 3

Jay D
Jay D

Reputation: 3307

  1. best way to debug this is to run your code against valgrind .

But to you give some perspective :

  1. when you free(d) you are expecting more of a destructor call on your struct dict which would internally free the memory allocated to the pointer to pointer to dictEntry

  2. why do you have to delete the entire has table to expand it ? you have a next pointer anyways why not just append new hash entries to it ?

Solution is not to free the d rather just expand the d by allocating more struct dictEntry and assigning them to appropriate next.

When contracting the d you will have to iterate over next to reach the end and then start freeing the memory for struct dictEntrys inside of your d.

Upvotes: 3

Roddy
Roddy

Reputation: 68033

What does dictCreate actually do?

I think you're getting confused between the (fixed size) dict object, and the (presumably variable sized) array of pointers to dictEntries in dict.table.

Maybe you could just realloc() the memory pointed to by dict.table, rather than creating a new 'dict' object and freeing the old one (which incidentally, isn't freeing the table of dictentries anyway!)

Upvotes: 1

Graham Borland
Graham Borland

Reputation: 60681

You are freeing a pointer which is passed in to your function. This is only safe if you know that whoever's calling your function isn't still trying to use the old value of d. Check all the code which calls _dictRehash() and make sure nothing's hanging on to the old pointer.

Upvotes: 1

Related Questions