Double free error Stack pop Implementation

0
votes
typedef struct StackElement{    

    struct StackElement* below;    
    float value;

} sStackElement;

typedef struct StackElement* stack_top; 

float stack_pop(stack_top *stacktop)                                   
{
     sStackElement* temp = (sStackElement *) stacktop; 
     if(stacktop==NULL){
         return NAN;
     }    
     float temp2 = temp->value;                                
     stacktop = &(temp->below);                      

     free(temp);                                     

     return temp2;
}

I am trying to implement a stack pop function, but somehow I get a double free error.

In the function, I am creating a temporary pointer which points to the current stack:

  • then I check if the pointer is Null.
  • then I point top to the 2nd Element in the Stack.
  • then I free temp (which points to the element on the top of the stack)
  • then I get the double free error

What am I doing wrong?

3
cstack
Isn't temp->below a pointer sStackElement* ? If so, you have an extra reference operator here stacktop = &(temp->below); - Eugene Sh.
So you mean I should remove the "&" ? If I do that I get an Error which says that I am assigning an incompatible pointer type - Christian Kasim Loan
So show us the definition of the sStackElement and stack_top - Eugene Sh.
This doesn't solve your problem, but I would recommend structuring your code so there is only one return. Then use an If clause to handle the null case and an else to handle the non-null. Its always a little easier to follow when there is one exist point in a function. - Nick
The extra & is a problem, see my answer - Eugene Sh.

3 Answers

2
votes

Presumably you're calling this as:

float value = stack_pop( &realstacktop );

// do more with stacktop

The problem is that this:

float stack_pop(stack_top *stacktop)                                   
{
   // ...
        stacktop = &(temp->below);                      
   // ...
}

does nothing to change the realstacktop you passed in. Call stack_pop() again, and you'll dereference - and free - the same top item.

You want to say:

*stacktop = temp->below;
1
votes

Your stack_top is the same type as temp->below - they both are pointers to the sStackElement. So the line 

stacktop = &(temp->below);

is actually assigning an sStackElement ** to sStackElement *, which is not what is intended. So remove the reference operator &, and perform a cast if the compiler complains.

0
votes
// called via: float value = stack_pop( &realstacktop );

#include <stdlib.h>
#define  NAN (0.0f) // only here for my compiler

struct StackElement
{
    struct StackElement* below;
    float value;
};



float stack_pop(struct StackElement **ppStacktop)
{
     if(*ppStacktop==NULL)
     { // then stack empty
         return NAN;
     }

     // implied else, stack not empty

        // save ptr to current stack top
     struct StackElement* temp = *ppStacktop;
        // retrieve value from current stack top
     float temp2 = temp->value;
        // pop current stack top entry
     *ppStacktop = temp->below;

     free(temp); // release 'old' stack top

     return temp2; // return retrieved value
} // end function: stack_pop