Error in stack with linked list implementation

Question

I'm trying to implement stack using linked list implementation. Its giving me "Segmentation Error". Please help me finding the error. This is my code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX 100

struct NODE {
    char word;
    struct NODE *next;
};

struct STACK {
    struct NODE *head;
    int size;
};

void pushStack(struct STACK *stack, char s);
void makeStack(struct STACK *stack, char *s);
void printStack(struct STACK *stack);

int main(){
    char *s;
    fgets(s,100,stdin);
    struct STACK stack;
    stack.head = NULL;
    makeStack(&stack,s);
    printStack(&stack);
    return 0;
}

void pushStack(struct STACK *stack, char s){
    struct NODE temp;
    temp.word = s;
    temp.next = stack->head;
    stack->head = &temp;
}

void makeStack(struct STACK *stack, char *s){
    char temp[MAX];
    strcpy(temp,s);
    for(int i=0; i<MAX; i++){
        if(temp[i]=='\0') break;
        pushStack(stack,temp[i]);
    }
}

void printStack(struct STACK *stack){
    struct NODE *trav = stack->head;
    while (trav != NULL){
        printf("%c", trav->word);
        trav = trav->next; 
    }
}

MAX=100 is the limit I'm taking for string input. I haven't also added increasing the size because I'm just ignoring the increment of size for now. Before I could perfect the implementation

`s` points nowhere: `char* s` -> `char s[100];`. There might be more problems though. — Jabberwocky, Mar 17 '21 at 08:28
After using fgets it does print the string. Doesn't that mean that it pointing somwhere? — Karnik Kanojia, Mar 17 '21 at 08:34
How can it print the string after `fgets`? First of all it is very likely that you get a Segmentation Error during `fgets` and then you don't print the string anyway. — Jabberwocky, Mar 17 '21 at 08:40
Thanks you were right actually. I tried the fgets and its actually giving segmentation error. I tried to print the string by initializing string using 'char s[100]' and then changed to char *s, thinking it's same. I'm sorry for leading wrong — Karnik Kanojia, Mar 17 '21 at 09:29

Jabberwocky · Accepted Answer · 2021-03-17T09:03:04.490

In main the s pointer is not initialized and it points nowhere.

int main(){
   char *s;   // <<< this is wrong, you want 'char s[100]' instead
   fgets(s,100,stdin);
   ...

However the safest option is this:

 int main(){
   char s[100];                 // declare array of 100 chars
   fgets(s, sizeof(s), stdin);  // sizeof(s) is the actual size of s (100 here)
   ...

This is wrong too: you store the pointer to the local variable temp, but that variables ceases to exist once you return from the pushStask function.

void pushStack(struct STACK* stack, char s) {
  struct NODE temp;
  temp.word = s;
  temp.next = stack->head;
  stack->head = &temp;
}

Instead you need to create a new struct NODE like this:

void pushStack(struct STACK* stack, char s) {
  struct NODE* temp = malloc(sizeof *temp);
  temp->word = s;
  temp->next = stack->head;
  stack->head = temp;
}

Instead of malloc(sizeof *temp) you could write sizeof(struct NODE), it's the same, but it's less fool proof because you could mistakenly write sizeof(struct STACK) which would compile fine, but the size of the allocated memory would be wrong.

Another problem: you don't assign the size field of the struct STACK, this is not a problem now, but it might become a problem later.

Shouldn't it be `(struct NODE*) malloc(sizeof(struct NODE))`? — Andy Sukowski-Bang, Mar 17 '21 at 08:41

Vlad from Moscow · Answer 2 · 2021-03-17T11:39:59.430

There are several drawbacks in your implementation of a stack.

The first one is that you are using a pointer with an indeterminate value to read a string

char *s;
fgets(s,100,stdin);

So the call of fgets invokes undefined behavior.

Moreover there is used a magic number 100.

You need to allocate a character array and use it to read a string.

#define MAX 100

//...

char s[MAX];
fgets( s, MAX, stdin );

Pay attention to that the name word for an object of the type char is confusing

struct NODE {
    char word;
    struct NODE *next;
};

You could define the structure like for example

struct NODE {
    char c;
    struct NODE *next;
};

or

struct NODE {
    char item;
    struct NODE *next;
};

Instead of separating the declaration and the initialization as you did

struct STACK stack;
stack.head = NULL;

forgetting to initialize the data member size (that by the way should have an unsigned integer type as for example size_t) you could just write for example

struct STACK stack = { NULL, 0 };

or

struct STACK stack = { .head = NULL, .size = 0 };

In the declaration of the function makeStack the second parameter should have the qualifier const because the passed string is not being changed within the function. And as a memory allocation in general can fail the function should report whether all characters of the string were pushed successfully. So the function declaration should look like

int makeStack( struct STACK *stack, const char *s );

It does not make a sense to declare a local array temp within the function

void makeStack(struct STACK *stack, char *s){
    char temp[MAX];
    //...

using the index variable i is redundant. Also the function fgets can append the new line character '\n' to the input string that you should not push on stack.

The function can be defined the following way

int makeStack( struct STACK *stack, const char *s )
{
    int success = 1;

    for ( ; *s && success; ++s )
    {
        if ( *s != '\n' )
        {
            success = pushStack( stack, *s );
        }
    }

    return success;
}

Another approach is to remove the new line character from the input string before passing it to the function makeStack.

For example

s[ strcspn( s, "\n" ) ] = '\0';
makeStack( &stack, s );

If it is the user that is responsible whether to push the new line character on stack or not then the function makeStack can be simplified

int makeStack( struct STACK *stack, const char *s )
{
    int success = 1;

    for ( ; *s && success; ++s )
    {
        success = pushStack( stack, *s );
    }

    return success;
}

Correspondingly the function pushStack also should be redefined.

For starters it shall dynamically allocate a new node. Otherwise you will try to add nodes that are local to the function and will not be alive after exiting the function that again results in undefined behavior.

The function pushStack can be defined the following way.

int pushStack( struct STACK *stack, char c )
{
    struct NODE *temp = malloc( sizeof( struct NODE ) );
    int success = temp != NULL;

    if ( success )
    {
        temp->word = c;
        temp->next = stack->head;

        stack->head = temp;
        ++stack->size;
    }

    return success;
}

The parameter of the function printStack should have the qualifier const because the stack itself within the function is not being changed.

The function can be defined at least the following way

void printStack( const struct STACK *stack )
{
    for ( const struct NODE *trav = stack->head; trav != NULL; trav = trav->next )
    {
        printf( "%c", trav->word );
    }
}

I really learned a lot from this. Thanks it is working now – Karnik Kanojia Mar 17 '21 at 10:13 — Karnik Kanojia, Mar 17 '21 at 10:13

Error in stack with linked list implementation

2 Answers2

Linked