Why does heap corruption occur in my stack program implemented with linked list? And how to fix it?

Question

I've created a C header file for a data structure course which I'm taking in school. I've only limited coding experience in C and C++. It contains code which builds a stack using linked list for data storage. When I try to run a driver program with Visual Studio 2013 to test if the implementation works, it throws the following error:

HEAP CORRUPTION DETECTED: after Normal block (#68) at 0x006F8178. CRT detected that the application wrote to memory after end of heap buffer.

The code in the said header file is listed below:

#include <stdlib.h>
#include <stdbool.h>

//type definition for a single stack data node
typedef struct node
{
    void *dataPtr; //create void pointer to user data
    struct node *link; //create pointer to next node in stack
}STACK_NODE;

//type definition for stack head structure
typedef struct stack
{
    int count; //location to hold number of entries in stack
    STACK_NODE *top; //create pointer to top of stack
}STACK;

//function to create empty stack
STACK* createStack()
{
    STACK *stack; //create a stack head node

    stack = (STACK*)malloc(sizeof(STACK));
    if (stack){
        stack->count = 0; //set stack count to zero
        stack->top = NULL; //initialize top pointer to null
    }
    return stack; //return address of node in dynamic memory
}

//function to push data onto stack
bool pushStack(STACK *stack, void *dataInPtr)
{
    STACK_NODE *newPtr;

    newPtr = (STACK_NODE*)malloc(sizeof(STACK_NODE*));
    //if out of memory
    if (!newPtr)
        return false;

    newPtr->dataPtr = dataInPtr; //assign data pointer to node
    newPtr->link = stack->top; //set link to point to node currently indicated as stack top
    stack->top = newPtr; //set top node to point to data in new node

    ++(stack->count); //add one to stack count
    return true;
}

//function to pop data off the stack and recycle node
void* popStack(STACK *stack)
{
    void *dataOutPtr;
    STACK_NODE *temp;

    //if stack is empty, return NULL
    if (stack->count == 0)
        dataOutPtr = NULL;
    else{
        temp = stack->top; //set temp to point to top node to be recycled
        dataOutPtr = stack->top->dataPtr; //set dataOutPtr to point to value currently stored in the top node
        stack->top = stack->top->link; //set top pointer to point to next node in stack
        free(temp); //delete top node
        --(stack->count);
    }
    return dataOutPtr; //return address of popped data
}

//function to retrieve data in top node
void* stackTop(STACK *stack)
{
    //if stack is empty, return NULL
    if (stack->count == 0)
        return NULL;
    //if top node contains data, return dataPtr
    else
        return stack->top->dataPtr;
}

//function to test if stack contains data
bool emptyStack(STACK *stack)
{
    return (stack->count == 0);
}

//function to delete nodes in stack
STACK* destroyStack(STACK *stack)
{
    STACK_NODE *temp;

    //if stack is not empty
    if (stack){
        //delete all nodes in stack
        while (stack->top != NULL){
            //delete data entry in top node
            free(stack->top->dataPtr);

            temp = stack->top; //set temp to point to top node to be recycled
            stack->top = stack->top->link; //set top node to point to next node
            free(temp); //destroy top node
        }
        //stack now empty, destroy stack head node
        free(stack);
    }
    return NULL;
}

The code for the driver program is listed below:

#include <stdio.h>
#include <stdlib.h>
#include "stackADT.h"

int main()
{
    int a = 4;
    int *dataPtr, *result, *popped;

    STACK *stack1;

    //create stack
    stack1 = createStack();

    //push value in a onto stack and output value on stack top
    dataPtr = malloc(sizeof(int));
    *dataPtr = a;
    pushStack(stack1, dataPtr);
    result = (int*)stackTop(stack1);
    printf("Value in stack is: %d\n", *result);

    //pop stack and output popped value
    popped = (int*)popStack(stack1);
    printf("Value popped off is: %d\n", *popped);

    destroyStack(stack1);
    return 0;
}

TBH, when I first saw the error message I have no idea what it meant. After doing some rudimentary research I now understand that it is caused by writing data to the heap buffer without having allocated enough memory in the first place.

While I'm not entirely certain, I believe the error occurs both in the popStack and destroyStack function at the line:

temp = stack->top;

And it is detected and reported at the line:

free(temp);

My idea is to pass the address contained in the current top node (stack->top) to a temporary node and then call free() to release the memory in the temp node. Since both pointers are of the same type (i.e. both are of STACK_NODE type), I do not understand why the assignment action would trigger a heap corruption error.

Any help in resolving the issue will be greatly appreciated!

Standard Warning : Please [do not cast](http://stackoverflow.com/q/605845/2173917) the return value of `malloc()` and family. — Sourav Ghosh, Feb 18 '15 at 14:00
Please provide a ___[MCVE](http://stackoverflow.com/help/mcve)___ — Sourav Ghosh, Feb 18 '15 at 14:01
1) `newPtr = (STACK_NODE*)malloc(sizeof(STACK_NODE*));` --> `newPtr = (STACK_NODE*)malloc(sizeof(STACK_NODE));` — BLUEPIXY, Feb 18 '15 at 14:04
@SouravGhosh, I've included a small driver program with which the error can be reproduced. — sircactus77, Feb 18 '15 at 14:31

score 2 · Accepted Answer · answered Feb 18 '15 at 14:32

2

  newPtr = (STACK_NODE*)malloc(sizeof(STACK_NODE*));

Is allocating enough storage for a "STACK_NODE Pointer" - normally 4 bytes - but sizeof (STACK_NODE) is 8 bytes, so when you use newPtr you are overwriting memory.

The correct form is

  newPtr = (STACK_NODE*)malloc(sizeof(STACK_NODE));

answered Feb 18 '15 at 14:32

Anonymouse

935
9
20

I think it's enough to say that `sizeof(STACK_NODE*) < sizeof(STACK_NODE);` without emphasizing the actual value that `sizeof()` is returning. – PaulMcKenzie Feb 18 '15 at 14:35
disagree, was trying to make it crystal clear what the problem is... Just saying that "sizeof(STACK_NODE*) < sizeof(STACK_NODE);" would confuse the hell out of me for answer, and I know what the problem is. – Anonymouse Feb 18 '15 at 14:37
You add that to the answer you gave. The OP used the wrong type in the call to malloc, which is the main point. If the struct were only 4 bytes, their code would still be wrong but the error would be masked. – PaulMcKenzie Feb 18 '15 at 14:41
@Anonymouse, that indeed fixes the problem. Thank you very much! I've been agonizing over this problem for hours now on account of failing to catch that extra asterisk after STACK_NODE... It was a careless typo on my part. – sircactus77 Feb 18 '15 at 14:52
@bastille77 For your reference the "Setting Breakpoints on a Memory Allocation Number" section on https://msdn.microsoft.com/en-us/library/x98tx3cf.aspx is VERY useful for tracking down such bugs... – Anonymouse Feb 18 '15 at 15:19

Why does heap corruption occur in my stack program implemented with linked list? And how to fix it?

1 Answers1