malloc fails catastrophically

Question

I am trying to implement a Queue in C. Coming from Java and other managed languages, I am really struggling with memory management. Here is the enqueue() function:

int enqueue(Queue q, int value) {

    Node newNode = malloc(sizeof(Node));
    /*newNode->value = value;

    if (q->size == 0)
        q->head = newNode;
    else
        q->head->next = &newNode;

    q->size++;*/
}

I am getting this error :

malloc.c:3096: sYSMALLOc: Assertion `(old_top == (((mbinptr) (((char *) &((av)->bins[((1) - 1) * 2])) - __builtin_offsetof (struct malloc_chunk, fd)))) && old_size == 0) || ((unsigned long) (old_size) >= (unsigned long)((((__builtin_offsetof (struct malloc_chunk, fd_nextsize))+((2 * (sizeof(size_t))) - 1)) & ~((2 * (sizeof(size_t))) - 1))) && ((old_top)->size & 0x1) && ((unsigned long)old_end & pagemask) == 0)' failed.

FWIW, here's the rest of the code (is this even right?):

typedef struct NodeStruct *Node;
struct NodeStruct {
    Node* prev;
    Node* next;
    int value;
};

typedef struct QueueStruct *Queue;
struct QueueStruct {
    Node* head;
    Node* tail;
    int size;
    int capacity;
};

Queue newQueue(int size) {
    Queue q = malloc(sizeof(Queue));

    q->capacity = size;
    q->size = 0;
    q->head = NULL;
    q->tail = NULL;

    return q;
}

void printQueue(Queue q) {
    printf("Queue of size %d, capacity %d", q->size, q->capacity);
}    

int main() {
    Queue myQ = newQueue(10);

    // this seems to work
    printQueue(myQ);
    // epic fail
    enqueue(myQ, 5);

    return 0;
}

Why is this happening?

Answer 1

you have trashed your heap

if you are on linux use electric fence or valgrind to find out where you went wrong

edit : you mean

Queue q = malloc(sizeof(QueueStruct));

and the same for node

Node n = malloc(sizeof(NodeStruct));

and I agree with others - its very misleading to call a pointer to a NodeStruct Node. Better to call it NodePtr or PNode and to call the struct Node.

Answer 2

The following line is probably giving you grief:

Node newNode = malloc(sizeof(Node));

Node is a pointer type, so you're only allocating enough space to hold a pointer, not an entire NodeStruct. I think what you want to do is:

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

or

Node newNode = malloc(sizeof(NodeStruct));

The same issue exists for Queue, you're only allocated space to hold a pointer, not a QueueStruct. Something else that I only just noticed, is that in your NodeStruct and QueueStruct, you are using the type Node*, which is actually NodeStruct **, which is probably not what you want, since Node is already a pointer.

Answer 3

It's often considered bad style in C to hide a pointer within a typedef. This is because you need to know that something is a pointer to use it properly, anyway. (For example, even the opaque type FILE in the standard library is used and passed around as a FILE *).

This seems to have led you astray - for example, your next and prev members are actually pointers-to-pointers, which is not really what you want. I suggest:

typedef struct NodeStruct Node;
typedef struct QueueStruct Queue;

struct NodeStruct {
    Node *prev;
    Node *next;
    int value;
};

struct QueueStruct {
    Node *head;
    Node *tail;
    int size;
    int capacity;
};

Queue *newQueue(int size) {
    Queue *q = malloc(sizeof(Queue));

    q->capacity = size;
    q->size = 0;
    q->head = NULL;
    q->tail = NULL;

    return q;
}

int enqueue(Queue *q, int value) {

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

    newNode->value = value;
    newNode->next = NULL;

    if (q->size == 0)
    {
        newNode->prev = NULL;
        q->tail = q->head = newNode;
    }
    else
    {
        newNode->prev = q->tail;
        q->tail->next = newNode;
        q->tail = newNode;
    }

    q->size++;
    return 0;
}

void printQueue(Queue *q) {
    printf("Queue of size %d, capacity %d\n", q->size, q->capacity);
}

int main() {
    Queue *myQ = newQueue(10);

    printQueue(myQ);
    enqueue(myQ, 5);

    return 0;
}