"A heap has been corrupted" after using malloc twice

Question

I have a linked list of "words" that I'm trying to build, I made a function called "add_to_mem" which adds to the linked list the next word. I've made a couple of checks on the code, and found out that he works twice - once when the linked list is a NULL, and once when it's not - and it is does working, but in the third time I'm calling to the method - I'm getting an "A heap has been corrupted" error. The code:

    typedef struct { unsigned int val : 14; } word;

    typedef struct machine_m

    {

    word * current;
    int line_in_memo;
    char * sign_name;

    struct machine_m * next_line;
}Machine_Memo;

The function:

    /*Adding a word to the memory.*/
void add_to_mem(word * wrd, int line, char * sign_name)
{
    Machine_Memo * temp = NULL, *next = NULL;
    if (machine_code == NULL)
    {

        machine_code = (Machine_Memo *)malloc(sizeof(Machine_Memo));
        if (machine_code == NULL)
        {
            printf("Memory allocation has failed.");
            exit(1);
        }
        machine_code->current = wrd;
        machine_code->line_in_memo = line;
        machine_code->sign_name = sign_name;
        machine_code->next_line = NULL;

    }
    else
    {
        printf("token has been reached");
        temp = machine_code;
        next = (Machine_Memo *)malloc(sizeof(Machine_Memo)); //Line of error
        if (next == NULL)
        {
            printf("MEMORY ALLOCATION HAS FAILED. EXITING PROGRAM.\nThe problem has occured on code line %d", 775);
            exit(0);
        }
        next->current = wrd;
        next->line_in_memo = line;
        next->sign_name = sign_name;
        next->next_line = NULL;

        while (temp->next_line != NULL)
        {
            temp = temp->next_line;
            temp->next_line = next;

        }


    }
}

Will do, but just to learn from this - why not to use a global variable? — Ido H Levi, Aug 15 '18 at 09:03
see here: https://stackoverflow.com/questions/484635/are-global-variables-bad — pmg, Aug 15 '18 at 09:07
Plainly, global variables are bad because private encapsulation is good. Study _private encapsulation_. — Lundin, Aug 15 '18 at 09:13
Problem not reproduced. But anyway, you are leaking a lot of memory with each `malloc` inside your function. You don't have to keep the pointer to `word` inside your machine. — pptaszni, Aug 15 '18 at 09:21
@Lundin I would read about this, thank you And @pmg - I tried passing `machine_code` as an parameter, and changing `machine_code` to a local variable of "main" - and now it won't even run once, it crashes with the same error when it's trying to create `machine_code` for the first time — Ido H Levi, Aug 15 '18 at 09:26
@Ptaq666 I have to keep it as a pointer since in other parts of the code I need to use its pointer — Ido H Levi, Aug 15 '18 at 09:31
I doubt that the problem is in this code. You probably cause some trouble between those two function calls. It might be important to see how you provide `wrd` and `sign_name` to the function. — Gerhardh, Aug 15 '18 at 13:20
wrd is created by a couple of different ways: - by value, or creating taking another structure with 14 bits (such as: `typedef struct { unsigned int ERA : 2; unsigned int destination_ooperand : 2; unsigned int source_operand : 2; unsigned int opcode : 4; unsigned int param2 : 2; unsigned int param1 : 2; }start_word;`) and then using memcpy for 14 bits to copy them. — Ido H Levi, Aug 15 '18 at 13:24
`sign_name` is created mostly NULL - there is only one or two times out of 20 that it's not NULL. — Ido H Levi, Aug 15 '18 at 13:24

score 0 · Answer 1 · answered Aug 15 '18 at 09:10

0

Here

        while (temp->next_line != NULL)
        {
            temp = temp->next_line;
            temp->next_line = next; // move out of the loop
        }

you may want to move the last assignment outside of the loop.

answered Aug 15 '18 at 09:10

pmg

106,608
13
126
198

score 0 · Accepted Answer · answered Aug 15 '18 at 09:54

0

As far as I understand the code, it does not create a linked list. it creates nodes, but does not link them together.

at first call, the machine_code (head of list) is created. at next call, the node 'next' is created, however, the loop:

while (temp->next_line != NULL)
    {
        temp = temp->next_line;
        temp->next_line = next;
    }

does nothing as the 'machine_code->next' value is null. so code inside the loop is not executed. and we do not get here a linked list, but sporadic nodes not connected each to other. you may wanted (as pointed at the other post here) to have something like:

while (temp->next_line != NULL)
    {
        temp = temp->next_line;
    }
temp->next_line = next;

answered Aug 15 '18 at 09:54

Eliyahu Machluf

1,251
8
17

I accidentally placed `temp->next_line = next` inside of the loop, now when it's out again it's still not working, and I recive the same problem. – Ido H Levi Aug 15 '18 at 10:38
Can you add the calling code, so we get an entire program, we can examine. – Eliyahu Machluf Aug 15 '18 at 10:50
The entire program is over 900 lines, I can upload it to github if you like, but I don't think it has anything to do with the function's behavior – Ido H Levi Aug 15 '18 at 13:01
I think the problem of heap corruption is not at code you've posted here, but at other code of your program. I suggest you try to isolate the problem, by writing as small as possible program which cause the problem you'v encountered. BTW, there are tools for heap corruption detection. see https://stackoverflow.com/questions/2470131/visual-studio-how-to-find-source-of-heap-corruption-errors – Eliyahu Machluf Aug 15 '18 at 13:18
Allright, when I think about it more and more, the problem could be caused from an eternal source, such as its input, is copying a 14 bits struct to a `word` type is a good idea, and could it be what caused it? I'm going to try to debug it right now – Ido H Levi Aug 15 '18 at 13:27

"A heap has been corrupted" after using malloc twice

2 Answers2