CODEWITHSUNDEEP
cpointerscharmallocfree
Grant Upson
Grant Upson

Reputation: 65

Freeing char* data not working

I have a List, each node in the list holds a char *data field which has to be malloc'd with the node itself, but when I try to free that data the console just stops, it doesn't crash, their is no error, nothing. But if I comment it out and only free the list node it works fine.

Am I doing this right?

void removeSpecificData(List *list, char *course)
{
    ListNodePtr currentNode = list->head;
    ListNodePtr previousNode = NULL;

    while (currentNode != NULL)
    {
        if (strcmp(currentNode->data, course) == 0)
        {
            ListNodePtr nodeToFree = currentNode;

            if (previousNode == NULL)
            {
                list->head = currentNode->nextNode;
                currentNode = list->head;
            }
            else
            {
                previousNode->nextNode = currentNode->nextNode;
                currentNode = previousNode->nextNode;
            }

            printf("Free Data");
            free(nodeToFree->data);
            printf("Free Node");
            free(nodeToFree);
        }
        else
        {
            previousNode = currentNode;
            currentNode = currentNode->nextNode;
        }
    }
}

Here's my createListNode function incase it's necessary to view.

ListNodePtr createListNode(char *newCourse)
{
    ListNodePtr newNode = (ListNodePtr)malloc(sizeof(struct ListNode));
    newNode->data = (char *)malloc(sizeof(strlen(newCourse) + 1));
    strcpy(newNode->data, newCourse);
    newNode->nextNode = NULL;

    return newNode;
}

Thanks in advance.

Upvotes: 1

Views: 155

Answers (3)

Vlad from Moscow
Vlad from Moscow

Reputation: 311126

For starters the function removeSpecificData at least has a typo or a bug because there is absent a declaration of the variable nodeToFree and neither value was assigned to the variable.

So this statement

free(nodeToFree);

does not make sense.

Also in this else block the second expression statement does not make sense.

    else
    {
        previousNode->nextNode = currentNode->nextNode;
        currentNode = previousNode->nextNode;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 
    }

The function is too complicated. It can be written much simpler. For example

void removeSpecificData(List *list, const char *course)
{
    ListNodePtr *currentNode = &list->head;

    while ( *currentNode != NULL && strcmp( ( *currentNode )->data, course) != 0 )
    {
        currentNode = &( *currentNode )->nextNode;
    }

    if ( *currentNode != NULL )
    {
        ListNodePtr tmp = *currentNode;
        *currentNode = ( *currentNode )->nextNode;

        free( tmp->data );
        free( tmp );
    }
}

As for the function createListNode then the expression

strlen(newCourse) + 1

has the type size_t. So the value of the expression that by the way is not evaluated:) 

sizeof(strlen(newCourse) + 1)

is equivalent to the expression

sizeof( size_t )

and is equal to 4 or 8 depending on the definition of the type size_t.

You nedd to write just

newNode->data = (char *)malloc( strlen(newCourse) + 1 );

Upvotes: 0

R Sahu
R Sahu

Reputation: 206727

The following line is incorrect.

newNode->data = (char *)malloc(sizeof(strlen(newCourse) + 1));

The sizeof() part is not right. If the length of the string is greater than sizeof(size_t), you end up allocating less memory than you need. The call to strcpy following it will use memory beyond what you allocated. As a consequence, your program will have undefined behavior.

Use

newNode->data = malloc(strlen(newCourse) + 1);

See Do I cast the result of malloc?

Upvotes: 3

Jean-François Fabre
Jean-François Fabre

Reputation: 140297

The allocation size is incorrect. It's detected by the system only when attempting to free the memory (corrupt memory list).

Why not just:

newNode->data = strdup(newCourse);

it would allocate the proper size (unlike your attempt) and would copy the string at the same time (no need for malloc and strcpy)

Upvotes: 2

Related Questions