Crash when freeing up dynamic array of char

Question

When I try to free the char array for the name field of Student in dequeue() function, the program crashes. The memory was previously allocated in the createStudent() function.

I really can't see what I miss.

#include "iostream"
#include "stdio.h"
using namespace std;

struct Student
{
    int id;
    char* name;
};

struct QueueNode
{
    Student* info;
    QueueNode* next;
};

Student* createStudent(int id, char* name)
{
    Student* s = (Student*)malloc(sizeof(Student));
    s->id = id;
    s->name = (char*)malloc(sizeof(name) + 1);
    strcpy(s->name, name);

    return s;
}

QueueNode* createQueueNode(Student *s)
{
    QueueNode *queue = (QueueNode*)malloc(sizeof(QueueNode));
    queue->info = s;
    queue->next = nullptr;

    return queue;
}

void enqueueNode(QueueNode* &root, QueueNode* node)
{
    if (root == nullptr)
        root = node;
    else
    {
        QueueNode* tmp = root;
        while (tmp->next)
            tmp = tmp->next;
        tmp->next = node;
    }
}

Student* dequeueNode(QueueNode* &root)
{
    Student* s = nullptr;
    if (root != nullptr)
    {
        QueueNode* tmp = root;
        s = tmp->info;
        root = root->next;
        free(tmp->info->name);    ----->>program crashes
        free(tmp->info);
        free(tmp);
    }
    return s;
}

void printqueue(QueueNode* root)
{
    if (root)
    {
        printf("Id:%d Name:%s\n", root->info->id, root->info->name);
        printqueue(root->next);
    }
}

void main()
{

    FILE* file = fopen("Text.txt", "r");
    int id; char buffer[50];
    QueueNode* queue = nullptr;

    fscanf(file, "%d", &id);
    while (!feof(file))
    {
        fgetc(file);
        fscanf(file, "%[^\n]s", buffer);
        Student* s = createStudent(id, buffer);
        QueueNode* node = createQueueNode(s);
        enqueueNode(queue, node);
        fscanf(file, "%d", &id);
    }
    dequeueNode(queue);
    printqueue(queue);
}

this is a very weird way to write C++ I must say, interpolating C and C++ when you could have simpler, one or the other — cat, Jun 09 '17 at 21:18
Possible duplicate of [What is a segmentation fault?](https://stackoverflow.com/questions/2346806/what-is-a-segmentation-fault) — ivan_pozdeev, Aug 18 '18 at 17:14

Jean-François Fabre · Accepted Answer · 2017-06-09T20:52:30.787

in createStudent this line:

s->name = (char*)malloc(sizeof(name) + 1);

allocates the size of the pointer + 1, not the size of the string pointed by name (sizeof is misused here). So you're corrupting the memory list, and you see that when freeing it, because you triggered undefined behaviour before by copying a string too long for the allocated buffer (it may appear to work with short strings, though).

Fix:

s->name = malloc(strlen(name) + 1);

or replace those 2 lines:

s->name = (char*)malloc(sizeof(name) + 1);
strcpy(s->name, name);

by

s->name = strdup(name);

that handles the allocation & the copy properly.

Note: since it's C++ code you'd be better off with std::string to handle the strings. You'll save a lot of time hunting down crashes & memory leaks.

I didn't realized that I wrote sizeof instead of strlen, thank you very much — CanciuCostin, Jun 09 '17 at 21:06

Crash when freeing up dynamic array of char

1 Answers1