-1

I made this program in c to reverse stack. but it is crashing. please help me figure out what is wrong. the program is working fine till taking inputs. but when reverse is called it crashes. i am not able to find the fault. all the memory is being allocated properly. so i dont think there is segmentation fault.

#include<stdio.h>
#include<stdlib.h>
typedef struct Node{
    int data;
    struct Node *next, *prev;
}SNode;

typedef struct{
     SNode *top;
     int count;
 }Stack;

 int isEmpty(Stack *s){
     return (s->count==0);
  }
 void push(Stack *s,int x){
        SNode *temp = (SNode *)malloc(sizeof(SNode));
        temp->next = s->top;
        temp->prev = NULL;
        s->top->prev = temp;
        temp->data = x;
        s->top = temp;
        s->count++;
   }

   int pop(Stack *s){
     if(isEmpty(s)){
     printf("Underflow");
      return;
    }
   SNode *temp = s->top;
   s->top = s->top->next;
   s->top->prev = NULL;
   int a = temp->data;
   free(temp);
   s->count--;
   return a;
   }

   void reverse(Stack *s,Stack *rs){
      while(!isEmpty(s)){
      int p = pop(s);
      push(rs,p);
    }
   }

   int main(){
       Stack *s = (Stack *)malloc(sizeof(Stack));
       Stack *rs = (Stack *)malloc(sizeof(Stack));
       char p='y';
       while(p=='y'){
           int pu;
           printf("Enter data to be pushed: ");
           scanf(" %d",&pu);
           push(s,pu);
           printf("Do you want to push another number? y/n:");
           scanf(" %c",&p);
         }
        reverse(s,rs);
        printf("Top of reversed stack: %d",rs->top->data);
        return 0;
        }

I dont know what i changed. i rewrote the code, this time using singly linked list, now it works really fine. dont know how !!??

#include<stdio.h>
#include<stdlib.h>
typedef struct node{
 int data;
 struct node *next;
}SNode;

typedef struct{
 int count;
 SNode *top;
}stack;

int isEmpty(stack *s){
 return (s->count==0);
}

void push(stack *s,int x){
 SNode *temp = (SNode *)malloc(sizeof(SNode));
 temp->data = x;
 temp->next = s->top;
 s->top=temp;
 s->count++;
}

int pop(stack *s){
    if(isEmpty(s)){
        printf("Underflow");
        return -1;
    }
 SNode *temp = s->top;
 s->top = temp->next;
 int a = temp->data;
 free(temp);
 s->count--;
 return a;
}

void reverseStack(stack *s, stack *rs){
 while(!isEmpty(s)){
    push(rs,pop(s));
 }
}

int main(){
 stack *s = (stack *)malloc(sizeof(stack));
 stack *rs = (stack *)malloc(sizeof(stack));
 s->count = rs->count =0;
 char p ='y';
 while(p=='y'){
    int x;
    printf("Enter data to push: ");
    scanf("%d",&x);
    push(s,x);
    printf("Do you want to push more data? : y/n");
    scanf(" %c",&p);
 }
 reverseStack(s,rs);
 printf("Top of reversed stack: %d",rs->top->data);
 return 0;
}
Gameatro
  • 143
  • 1
  • 1
  • 8

4 Answers4

0

malloc doesn't initialize the struct members to zero and NULL. You have to do that yourself or use calloc instead. So change your malloc lines in your program to something like this:

Stack *s = calloc(1, sizeof(Stack));
Stack *rs = calloc(1, sizeof(Stack));
bruceg
  • 2,433
  • 1
  • 22
  • 29
  • Why should they be null? – Gameatro Mar 28 '18 at 00:45
  • 1
    What happens here: `temp->next = s->top;`? What is the value of `s->top;`?? (hint: *Undefined Behavior* invoked by attempting to access an indeterminate value) – David C. Rankin Mar 28 '18 at 00:47
  • your count field needs to be zero, and the top pointer needs to be NULL to indicate the end of list. If it's not NULL it is just a random number and will cause problems when you de-reference it. – bruceg Mar 28 '18 at 00:47
  • @DavidC.Rankin s->top is the top of the stack; – Gameatro Mar 28 '18 at 01:38
  • It's also an *Uninitialized Pointer* which invoked *Undefined Behavior* when you attempt to assign it to `temp->next` `:)` – David C. Rankin Mar 28 '18 at 02:07
  • https://googleweblight.com/i?u=https://www.geeksforgeeks.org/design-a-stack-with-find-middle-operation/&hl=en-IN – Gameatro Mar 28 '18 at 02:25
  • See the link. They have implemented stack similar to me. Thst code had no error. They have not done all that you told. So why is mine crashing? – Gameatro Mar 28 '18 at 02:27
0

I see at least two problems.

When you malloc() a new Stack structure, you don't initialize the *top and count fields, and their contents is likely to be garbage.

In pop():

SNode *temp = s->top;
s->top = s->top->next;
s->top->prev = NULL;

What happens if the initial value of s->top->next is NULL?

Jim Lewis
  • 43,505
  • 7
  • 82
  • 96
0

You have a number of problems in your implementation of your stack. The first of which is in push(), when you call:

temp->next = s->top;

s->top is an uninitialized pointer whose value is indeterminate. That invokes Undefined Behavior. Since this occurs on your very first call to push(s, pu), you can have zero confidence in the operation of the remainder of your code.

Next, both your push and pop function fail to test and handle in an appropriate manner whether the node being pushed or popped is the first or last node in the stack. These must be handled differently as the handling of your prev and next pointers will depend on whether other nodes exist. (you can't assign a next or prev node if you are pushing the first node, and you can't assign a new top on pop if no more nodes exist.

(note: you also seem to have used prev and next in a somewhat inconsistent manner. As long as you use them consistently, it doesn't matter whether you use prev for the existing top on push or next. I tend to have prev pointing down the stack and next pointing from the bottom up - but you are free to do it the other way)

To properly handle the first and last node in your stack, you can do a simple check on isempty() or simply check if (s->top == NULL) or if (s->count == 0).

You also need to initialize all pointers and count when you allocate storage for your stack and nodes. Failure to initialize all values on allocation will likely lead to an inadvertent attempted read from an uninitialized value (invoking additional instances of Undefined Behavior).

To eliminate that possibility, you can create simply helper functions, say create_node and create_stack to allocate, and validate the allocation, initialize the needed values, and then to provide a meaningful return indicating success or failure. You could do something simple like the following:

snode *create_node (int x)
{
    snode *tmp = malloc (sizeof *tmp);
    if (!tmp) {
        perror ("create_node: memory exhausted.");
        return NULL;
    }
    tmp->data = x;
    tmp->prev = tmp->next = NULL;

    return tmp;
}

stack *create_stack (void)
{
    stack *tmp = malloc (sizeof *tmp);
    if (!tmp) {
        perror ("create_stack: memory exhausted.");
        return NULL;
    }
    tmp->top = NULL;
    tmp->count = 0;

    return tmp;
}

Now there are no possibilities of an uninitialized value.

(Also note: while not an error, the standard coding style for C avoids the use of camelCase or MixedCase variable names in favor of all lower-case while reserving upper-case names for use with macros and constants. It is a matter of style -- so it is completely up to you, but failing to follow it can lead to the wrong first impression in some circles.)

With the helper functions defined, allocating and initializing your forward and reverse stacks in main() becomes a simple matter of:

    int pu;
    stack *s = create_stack();
    stack *rs = create_stack();

    if (!s || !rs)      /* validate the return from create_stack */
        return 1;

Having created your forward and reverse stack, you can then handle the first and last node cases in push() and pop() with the addition of a simple conditional as described above:

void push (stack * s, int x)
{
    snode *temp = create_node (x);
    if (!s->top)             /* is the stack empty? */
        s->top = temp;
    else {
        s->top->next = temp;
        temp->prev = s->top;
        s->top = temp;
    }
    s->count++;
}

and for pop(),

int pop (stack *s)
{
    int x;
    snode *temp;
    if (isempty (s)) {      /* checking empty as you did in original */
        printf ("underflow");
        return 0;
    }
    x = s->top->data;

    temp = s->top;
    if (s->top->prev)
        s->top = s->top->prev;
    s->top->next = NULL;
    free (temp);

    s->count--;
    return x;
}

Adding a short prn_stack() function to iterate over the stack without poping and freeing the nodes, you could put together a short example program to test the push, pop and reverse as follows:

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

typedef struct node {
    int data;
    struct node *next, *prev;
} snode;

typedef struct {
    snode *top;
    int count;
} stack;

snode *create_node (int x)
{
    snode *tmp = malloc (sizeof *tmp);
    if (!tmp) {
        perror ("create_node: memory exhausted.");
        return NULL;
    }
    tmp->data = x;
    tmp->prev = tmp->next = NULL;

    return tmp;
}

stack *create_stack (void)
{
    stack *tmp = malloc (sizeof *tmp);
    if (!tmp) {
        perror ("create_stack: memory exhausted.");
        return NULL;
    }
    tmp->top = NULL;
    tmp->count = 0;

    return tmp;
}

int isempty (stack *s)
{
    return (s->count == 0);
}

void prn_stack (stack *s)
{
    snode *iter;
    if (isempty(s)) {
        puts ("stack empty");
        return;
    }
    iter = s->top;
    for (; iter; iter = iter->prev)
        printf ("  %d\n", iter->data);
}

void push (stack * s, int x)
{
    snode *temp = create_node (x);
    if (!s->top)
        s->top = temp;
    else {
        s->top->next = temp;
        temp->prev = s->top;
        s->top = temp;
    }
    s->count++;
}

int pop (stack *s)
{
    int x;
    snode *temp;
    if (isempty (s)) {
        printf ("underflow");
        return 0;
    }
    x = s->top->data;

    temp = s->top;
    if (s->top->prev)
        s->top = s->top->prev;
    s->top->next = NULL;
    free (temp);

    s->count--;
    return x;
}

void reverse (stack * s, stack * rs)
{
    while (!isempty (s))
        push (rs, pop (s));
}

int main ()
{
    int pu;
    stack *s = create_stack();
    stack *rs = create_stack();

    if (!s || !rs)
        return 1;

    while (scanf (" %d", &pu) == 1)
        push (s, pu);

    printf ("stack:\n");
    prn_stack (s);

    reverse (s, rs);

    printf ("\nreversed stack:\n");
    while (!isempty (rs))
        printf ("  %d\n", pop (rs));

    free (s);
    free (rs);

    return 0;
}

(note: do not forget to free the memory you allocate for your forward and your reverse stacks after all the values have been popped, and always validate your memory use by using a memory-error checking program like valgrind on Linux. There are similar programs for all OS's)

Example Use/Output

$ echo "10 9 8 7 6 5 4 3 2 1" | ./bin/stack_rev
stack:
  1
  2
  3
  4
  5
  6
  7
  8
  9
  10

reversed stack:
  10
  9
  8
  7
  6
  5
  4
  3
  2
  1

To validate your memory use and that you have freed all memory you allocate and that there are no memory errors, just run your code through the memory checker, similar to the following using valgrind, e.g.

Memory Use/Error Check

$ echo "10 9 8 7 6 5 4 3 2 1" | valgrind ./bin/stack_rev
==18418== Memcheck, a memory error detector
==18418== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==18418== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==18418== Command: ./bin/stack_rev
==18418==
stack:
  1
  2
  3
  4
  5
  6
  7
  8
  9
  10

reversed stack:
  10
  9
  8
  7
  6
  5
  4
  3
  2
  1
==18418==
==18418== HEAP SUMMARY:
==18418==     in use at exit: 0 bytes in 0 blocks
==18418==   total heap usage: 22 allocs, 22 frees, 512 bytes allocated
==18418==
==18418== All heap blocks were freed -- no leaks are possible
==18418==
==18418== For counts of detected and suppressed errors, rerun with: -v
==18418== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Look things over and let me know if you have any further questions.

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
  • https://googleweblight.com/i?u=https://www.geeksforgeeks.org/design-a-stack-with-find-middle-operation/&hl=en-IN see the link. They have done similar to mine. But that don't have error. They have not done any of what you told. – Gameatro Mar 28 '18 at 02:29
  • Well, actually, it's virtually the same. The only differences are that code is interested in finding the middle of the stack - not so here Otherwise, the basic stack operations are identical. (there are only so many ways to do a stack based on a linked list). What part are you having trouble understanding? Be wary of "code" on the internet. That code fails to `free` its memory and see: [Do I cast the result of malloc?](http://stackoverflow.com/q/605845/995714) – David C. Rankin Mar 28 '18 at 03:07
  • well what i realized that my code is working sometimes actually now that i ran it several times. sometimes it works. other times it crashes. why is this hapenning? – Gameatro Mar 28 '18 at 03:40
  • If it is your original code, it is because you try and use variables that have not been assigned an initial value -- and by default have an indeterminate value. Only global variables or variables with *static or thread storage duration* are initialized by the compiler -- all others are indeterminate (meaning until they are assigned a value, just have some stray garbage value) This is specified in the [C11 Standard - § 6.7.9 Initialization (p10)](http://port70.net/~nsz/c/c11/n1570.html#6.7.9p10) In any pointer operation, you have to know what it is pointing to before using it. – David C. Rankin Mar 28 '18 at 03:44
  • i did change my code . i initialized count to zero. now my code works for 3 elements, then crashes. why is it so? – Gameatro Mar 29 '18 at 22:49
  • @Gameatro, If you posted your latest code, I'll take a look. – David C. Rankin Mar 30 '18 at 01:32
  • i just rewrote the code with singly linked list. now it is working fine. dont know why? can you take look at second code and tell what was wrong with first. it looks almost same except that first is dll and second is sll. – Gameatro Mar 30 '18 at 11:25
  • Sure. I won't have a chance until later this evening, but I'll pick through it and see what I find. – David C. Rankin Mar 30 '18 at 18:53
  • @Gameatro The difference is you now initialize `count` with `s->count = rs->count =0;` This sets both `s->count` and `rs->count` equal to zero and avoids the *Undefined Behavior* that occurred when you previously attempted `count++;` when `count` was not previously initialized. If my answer was helpful, don't forget to select the answer by checking the box at the top-left of the question. Otherwise this question will continue to float around in the unanswered->on-hold queues. – David C. Rankin Mar 30 '18 at 21:18
0

Your code crashes randomly because s->count is not 0 when you first declare it in main. If you're lucky, s->count is 0, then your reverse is fine because isEmpty() will work correctly. Otherwise, you will get segmentation fault.

tudatn
  • 102
  • 1
  • 6
  • I did that. Now my code works fine for 3 sized stack. But when the 4 the is being poped and pushed to reverse stack, code crashes – Gameatro Mar 28 '18 at 13:18
  • Because you don't initialize values of properties of either nodes or stacks, your program may behave unpredictably. It is a good habit to try to print out values of each steps in your functions. I can point out one of problem in your push: you initialize stack * s in main, let say if your s->top is null (randomly), so the line s->top->prev will cause segmentation fault. The similar problem can happen anywhere uninitialized value is used. – tudatn Mar 28 '18 at 22:26