Search code examples
cmemory-managementlinked-listreallocstrcpy

strcpy corrupts char array (string value)


The function below tries to order strings on a linked list in ascending order. When it returns the new list, it becomes corrupted.

void* order( void *ptr){
    struct wordlist *head;
    head = (struct wordlist *) ptr;

    struct wordlist *first = (struct wordlist*)malloc(sizeof(struct wordlist));
    struct wordlist *second = (struct wordlist*)malloc(sizeof(struct wordlist));
    struct wordlist *temp = (struct wordlist*)malloc(sizeof(struct wordlist));

    first = head;

    int j = 1;
    while( first != NULL){
        second = first->next;

        while( second != NULL){
            if( strcmp( first->word, second->word) > 0){
                if( temp->word == NULL){
                    temp->word = malloc( sizeof(first->word));
                }
                else{
                    if( realloc( temp->word, sizeof( first->word)) != NULL){
                        strcpy( temp->word, first->word);
                    }
                }

                if( realloc( first->word, sizeof(second->word)) != NULL){
                    strcpy( first->word, second->word);
                }

                if( realloc( second->word, sizeof(temp->word)) != NULL){
                    strcpy( second->word, temp->word);
                }

                free(temp);
            }
            second = second->next;
        }
        j++;
        first = first->next;
    }
}

For example, if input would be

piero
ronaldo
messi

then the output looks like

messi
ŽŽŽ
ronaldo

The above example is not tried on the code, but it will give you a clue. I believe there is something with the allocation of the memory but I could not manage to find it. By the way, sometimes the words come empty too.

Also, the wordlist is as follows:

struct wordlist{
    char *word;
    struct wordlist *next;
};

Solution

  • You don't copy the string into your temporary the first time around.

                if( temp->word == NULL){
                    temp->word = malloc( sizeof(first->word));
                    // You forgot to copy!!
                }
                else{
                    if( realloc( temp->word, sizeof( first->word)) != NULL){
                        strcpy( temp->word, first->word);
                    }
                }
    

    See, if temp->word is NULL, which it ought to be the first time (note that you don't actually clear the temp struct so already you will get undefined behaviour), then you don't copy it. The quick fix is to do a strcpy after you malloc.

    Your realloc calls are all wrong. You cannot use sizeof to get the size of a string. Use strlen for that, and don't forget to add an extra byte for the string terminator.

    Furthermore, you should not allocate first and second. They are iterators for your data structure. The first thing you do is discard their value so you leak memory. Don't forget to free your temp structure as well as temp->word afterwards.

    After you get that working, please stop all this malloc and strcpy business!!!

    To move the strings around you only need to move the pointers. No reallocation or copies required. This will simplify your code down to a handful of lines.

    Oh, and did you also forget to return a value from your function?