Search code examples
cpointersgccmemory-leaksvalgrind

Proper way of deallocation of double pointer to struct


I am trying to add memory deallocations to old C code. I have a hash table of custom objects (HASHREC). After analysis of current code and reading other SO questions, I know that I need to provide three levels of deallocations. Fist - word member, next HASHREC*, and then HASHREC**.

My version of free_table() function frees mentioned objects. Unfortunately, Valgrind still complains that some bytes are lost.

I am not able to provide full code, it will be too long, but I am presenting how HASHREC **vocab_hash is filled inside inithashtable() and hashinsert(). Could you give me a suggestion how should I fix free_table()?

typedef struct hashrec {
    char *word;
    long long count;
    struct hashrec *next;
} HASHREC;

HASHREC ** inithashtable() {
    int i;
    HASHREC **ht;
    ht = (HASHREC **) malloc( sizeof(HASHREC *) * TSIZE );
    for (i = 0; i < TSIZE; i++) ht[i] = (HASHREC *) NULL;
    return ht;
}

void hashinsert(HASHREC **ht, char *w) {
    HASHREC     *htmp, *hprv;
    unsigned int hval = HASHFN(w, TSIZE, SEED);

    for (hprv = NULL, htmp = ht[hval]; htmp != NULL && scmp(htmp->word, w) != 0; hprv = htmp, htmp = htmp->next);
    if (htmp == NULL) {
        htmp = (HASHREC *) malloc( sizeof(HASHREC) );  //<-------- problematic allocation (Valgrind note)
        htmp->word = (char *) malloc( strlen(w) + 1 );
        strcpy(htmp->word, w);
        htmp->next = NULL;
        if ( hprv==NULL ) ht[hval] = htmp;
        else hprv->next = htmp;
    }
    else {/* new records are not moved to front */
        htmp->count++;
        if (hprv != NULL) { /* move to front on access */
            hprv->next = htmp->next;
            htmp->next = ht[hval];
            ht[hval] = htmp;
        }
    }
    return;
}

void free_table(HASHREC **ht) {
    int i;
    HASHREC* current;
    HASHREC* tmp;
    for (i = 0; i < TSIZE; i++){
        current = ht[i];
        while(current != NULL) {
            tmp = current;
            current = current->next;
            free(tmp->word);
        }
        free(ht[i]);
    }
    free(ht);
}

int main(int argc, char **argv) {
    HASHREC **vocab_hash = inithashtable();
    // ...
    hashinsert(vocab_hash, w);
    //....
    free_table(vocab_hash);
    return 0;
}

Solution

  • I assume the problem is here:

    current = ht[i];
    while(current != NULL) {
        tmp = current;
        current = current->next;
        free(tmp->word);
    }
    free(ht[i]);
    

    You release the word but you don’t release tmp. After you release the first item in the linked list but not the others which causes a leak.

    Free tmp in there and don’t free ht[i] after since it’s already freed here.

    current = ht[i];
    while(current != NULL) {
        tmp = current;
        current = current->next;
        free(tmp->word);
        free(tmp);
    }