Hashtable valgrind memory leak

Question

Hi I've been working on school project and need your help with the code. I use valgrind to work out what is wrong and need to get rid of the awful errors what exacly means by your thoughts

Function inserts new element into table

This is one of the errors i get

Invalid write of size 1 at 0x4C29B32: strcpy (vg_replace_strmem.c:458) by 0x401CE9: HTab_insert (ial.c:65) by 0x4019B5: main (main.c:82) Address 0x5449785 is 0 bytes after a block of size 5 alloc'd at 0x4C28FA4: malloc (vg_replace_malloc.c:296) by 0x401CC9: HTab_insert (ial.c:64) by 0x4019B5: main (main.c:82)

HTab_listitem* HTab_insert(HTab_t* ptrht, Ttoken token) {
    unsigned ind = hash_function(ptrht->htable_size,token);
    HTab_listitem* item_ptr = NULL;
    HTab_listitem* item = ptrht->list[ind];
    HTab_listitem* nextitem;

    if(item == NULL) {
        nextitem = malloc(sizeof(HTab_listitem)+sizeof(char)*(strlen(token.data)+1));

        if(nextitem == NULL)
            /*allocation error*/
            return NULL;
        else {
            //printf("HERE\n");
            //printf("%s\n", token.data);
            //memcpy(nextitem->token.data,token.data,strlen(token.data)+1);
            int length = strlen(token.data);
            nextitem->token.data = malloc(length * sizeof((char) +2));
            strcpy(nextitem->token.data,token.data);
            nextitem->token.data[length] = '\0';
            nextitem->token.stav = token.stav;
            //printf("HERE AFTER\n");
            nextitem->ptrnext = NULL;

            item = ptrht->list[ind] = nextitem;

            nextitem = NULL;
            if(item == NULL)
                return NULL;
        }
    }
    else {
        while(item != NULL) {
            if(strcmp(item->token.data,token.data) == 0) {
                //if found
                item_ptr = item;
                break;
            }
            else {
                //next item
                item_ptr = item;
                item = item->ptrnext;
            }
        }
        if(item_ptr != NULL && item != item_ptr) {
            //not found insert next item
            nextitem = malloc(sizeof(HTab_listitem*)+sizeof(char)*(strlen(token.data)+1));
            if(nextitem == NULL)
                /*allocation error*/
                return NULL;
            else {
                //memcpy(nextitem->token.data,token.data,strlen(token.data)+1);
                int length = strlen(token.data);
                nextitem->token.data = malloc(length * sizeof((char) +2));
                strcpy(nextitem->token.data,token.data);
                nextitem->token.data[length] = '\0';
                nextitem->token.stav = token.stav;

                nextitem->ptrnext = NULL;
                item = nextitem;
                if(item == NULL)
                    return NULL;
                item_ptr->ptrnext = item;
            }
        }
    }
    return item;
}

You use strcpy for copying token.data. Are you sure token.data is null terminated? — GMichael
This is incorrect (+2 should not be inside parenthesis): malloc(length * sizeof((char) +2)). Do you mean malloc(length +2)? (Side note: sizeof(char) is 1 and thus can be omitted) — Klas Lindbäck
How can we reproduce your valgrind runtime warnings when we can't compile your code? Try again... see the MCVE comment. — autistic
@KlasLindbäck did you see the extra parantheses in sizeof((char) +2))? So the number +2 will be casted to a char. So malloc will be called with length, which makes the next line invoking undefined behaviour. — mch

Tezirg Tezirg · Accepted Answer · 2016-10-27T13:02:14

You have a few allocation errors that could lead to undefined behavior. I will go into the code from top to bottom

First, you allocate to much memory

nextitem = malloc(sizeof(HTab_listitem)+sizeof(char)*(strlen(token.data)+1));

Where the following should be enough, since the nextitem->token.data is allocated afterwards.

nextitem = malloc(sizeof(HTab_listitem));

Also, when allocating token.data, use the following :

int length = strlen(token.data);
nextitem->token.data = malloc( sizeof(char) * (length + 1) );
nextitem->token.data[length] = 0;

Your second item allocation is again not the right size. You allocate the sizeof of a pointer (8 bytes) instead of the size of your struct and adding the token.data is still not usefull.

nextitem = malloc(sizeof(HTab_listitem*)+sizeof(char)*(strlen(token.data)+1));
//Error here (HTab_listitem*)

That should be :

 nextitem = malloc(sizeof(HTab_listitem));

Then again allocate token.data as done previousely.

Hashtable valgrind memory leak

1 Answers