Why does malloc fail to allocate more than once?

Question

I coded a simple source. It contains a queue and some of the function a queue needs but for some reason malloc() only works once.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>



#define QUEUE       sizeof(Queue)

Definition of the Node, which is an element of the list, and the queue.

typedef struct node {
    char * value;
    struct node * next;
} Node;

typedef struct queue {
    Node * head;
    Node * tail;
} Queue;



int initialization(void ** list, int type){
    int code = -1;
    //create an empty list. 
    //if queue dynamically allocate memory and assign NULL to both properties head and tail. 


    return code;    
}

enqueue() add one element in the queue at a time. but for some reason it can only add one element then the program crashes.

int enqueue(Queue * q, char * instruction){
    int code = -1;
    if(q != NULL){
        printf("Prepare to enqueue!\n");
        Node * n = NULL;
        n = (Node*)malloc(sizeof(Node));
        if(n != NULL){
            printf("Node created!\n");
            strcpy(n->value, instruction);
            n->next = NULL;

            //if first value
            if(q->head == NULL){
                q->head = n;
                q->tail = n;

                printf("Enqueue first Node\n");
            }
            else {
                q->tail->next = n;
                q->tail = n;
                printf("Enqueue another Node\n");
            }
            code = 0;
            printf("Node \"%s\" Enqueued\n", instruction);
        }
    }
    return code;
}

int dequeue(Queue * q){
    int code = -1;
    //dequeuing code here.
    return code;
}


int isEmpty(void * list, int type){
    int code = 0;
    //check if the list is empty

    return code;
}

the for loop in the main() function never reaches 3

int main(int argc, char * argv[]){

    Queue * queue = NULL;

    initialization((void*)&queue, QUEUE);

    int i = 0;

    for(i = 0; i < 3; i++){
        if(enqueue(queue, "some value") != 0){
            printf("couldn't add more Node\n");
            break;
        }
    }

    while(!isEmpty(queue, QUEUE)){
        dequeue(queue);
    }

    return 0;
}

The initialization function is written this way because it should also be able to initialize stacks (I removed the stack code to reduce the source but even without it the bug persist). I also put printfs to debug the code. And I have more than enough memory to make this simple code run how it should.

Thanks in Advance!

In C, as long as you're not trying to drop `const` (which is very rare), you never need to cast to/from `void *`. Thus, [don't cast the return value of `malloc()`](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc), either. — unwind, Dec 17 '14 at 14:36
Side note: using `sizeof(Queue)` as a type-identifier is a very bad idea (because a type-identifier is basically supposed to be **unique**). — barak manos, Dec 17 '14 at 14:37
So far this is just a bunch of code with a vague description telling us it has a bug. What does "malloc() only works once" mean ? What happens ? What should happen ? — nos, Dec 17 '14 at 14:39
@nos I Agree, Nass S, please break this question down a bit and provide a shorter example so we do not have to analyze 50 lines or so of code. — Nick, Dec 17 '14 at 14:42
I get errors if I don't cast it to `void *` (in the `main`) and if I replace the `void **` by `Queue **` I won't be able to use `int initialization()` with other type (such as Stack). But even if I replace every single `void` by `Queue` I still get a runtime error somehow. — Nass S, Dec 17 '14 at 14:46
Leaving this out of the answer (below), since it's *irrelevant to the actual problem* (though people love to pounce on it), but are you compiling with a C++ compiler? That would explain the errors if you don't cast `malloc()`. In C, as opposed to C++, there's no need to do that. — Paul Roub, Dec 17 '14 at 15:09

score 4 · Accepted Answer · answered Dec 17 '14 at 14:43

4

Running this, I crash with a segmentation fault, as I'd expect:

n = (Node*)malloc(sizeof(Node));

n is allocated, it's contents uninitialized and effectively random

if(n != NULL){

n is not NULL, so...

  strcpy(n->value, instruction);

And we crash.

See the problem? n->value is a pointer to nowhere. Or, to somewhere, but nowhere known. Nowhere good. And we're just dumping a string into that space.

Either change the Node struct so that value is a char [SOME_SIZE], or use strdup() instead of strcpy(), to actually allocate some memory for the poor thing.

n->value = strdup(instruction);

answered Dec 17 '14 at 14:43

Paul Roub

36,322
27
84
93

yes but isn't `malloc` suppose to return NULL in case it fails to allocate memory? That's why I don't initilialize n. – Nass S Dec 17 '14 at 15:01
2

No one said it failed to allocate memory. It allocated all the memory it was asked for; it just doesn't *initialize* that memory. The pointer itself (`value`) was allocated, but it's not *pointing* to anything. Allocating the 8 or 16 bytes (or whatever) needed to hold the struct doesn't magically create space somewhere else for the string `value` will point to. – Paul Roub Dec 17 '14 at 15:03
Look at it this way: if `value` were an `int` instead, after `n = malloc(sizeof(Node));`, what would that `int` value be? Answer: you have no idea. `malloc()` does *not* initialize the memory it allocates. Same problem here. `value` is an uninitialized pointer until you set it. – Paul Roub Dec 17 '14 at 15:06
OK, I always though `strcpy()` (and `sprintf` for that matter) allocated memory for the pointer by itself. – Nass S Dec 17 '14 at 15:14
Nope. Nor do `memcpy()`, `memset()`, `strcat()`... basically anything that *takes* a `char *`, `void *` etc. destination parameter, rather than *returning* one. – Paul Roub Dec 17 '14 at 15:16

Why does malloc fail to allocate more than once?

1 Answers1