Singly Linked List in C incorrect output

Question

So I'm doing some linked list revison and Im trying to just load a list with some numbers and then print it out. Below is my code:

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

typedef struct stack {
   int data;
   struct stack *next;
}*stack;

stack create_s(void){
   stack s = (void*)malloc(sizeof(stack));
   s->next = NULL;

   return s;
}

void push_s(stack s, int data) {
   while (s->next != NULL) {
      s = s->next;
      }

   s->next = (void*)malloc(sizeof(stack));
   s=s->next;
   s->data = data;
   s->next = NULL;
}

void print_s(stack s) {
   if (s==NULL) {
      return;
   } 
   else {
      while (s->next != NULL) {
         printf("%d\n",s->data);
         s=s->next;
      }
   }
}

int main (void) {
   stack s = create_s();

   push_s(s,2);
   push_s(s,4);
   push_s(s,6);
   push_s(s,8);

   print_s(s);

   return 0;
}

My output is however:

-1853045587
2
4
6

when it should be

2
4
6
8

Is it printing the address of my struct at the beginning? Also, why is it not printing my last element?

Thanks

Answer 1

The code contains several errors, but the first thing that catches the eye is that your memory allocation is already obviously broken

stack s = (void*)malloc(sizeof(stack));

You defined stack as a pointer type. This means that sizeof(stack) evaluates to pointer size and the above malloc allocates enough space to store a single pointer, not enough for the entire struct stack object. The same memory allocation error is present in push_s as well.

Here's some advice

1. Don't hide pointer types behind typedef names. Define your stack as

   typedef struct stack{
     int data;
     struct stack *next;
   } stack;
   

   and use stack * wherever you need a pointer. I.e. make that * visible instead of hiding it "inside" a typedef name. This will make your code easier to read.

2. Don't cast the result of malloc. Anyway, what is the point of casting it to void * when it is void * already???

3. Don't use sizeof with types unless you really really have to. Prefer to use sizeof with expressions. Learn to use the following malloc idiom

   T *p = malloc(sizeof *p);
   

   or, in your case

   struct stack *s = malloc(sizeof *s);
   

   This will allocate a memory block of appropriate size.

Also, as @WhozCraig noted in the comments, the very first node in your list is apparently supposed to serve as a "sentinel" head node (with undefined data value). In your code you never initialize the data value in that head node. Yet in your print_s function you attempt to print data value from the head node. No wonder you get garbage (-1853045587) as the first line in your output. Don't print the very first node. Skip it, if it really is supposed to serve as a sentinel.

Also, the cycle termination condition in print_s looks strange

while (s->next != NULL)

Why are you checking s->next for NULL instead of checking s itself? This condition will terminate the cycle prematurely, without attempting to print the very last node in the list. This is the reason why you don't see the last element (8) in your output.

Answer 2

The actual cause of the given output can be fixed by changing:

s=s->next;
s->data = data;

to

s->data = data;
s=s->next;