-1

Why do I keep getting a stack overflow error from the code below in C? Any kind of insight would be much appreciated.

This code was created to add a node to the end of a linked list.

Can someone please review this code and tell me the problem with my insertion process. I am a fairly new programmer and would appreciate any insight.

struct linked_list
{
    int y;
    int x;

    struct linked_list *next;
};

struct linked_list **node = NULL;

void list_build(int y, int x)
{
    circular_node *list = (struct linked_list *)calloc(1, sizeof(struct linked_list));
    list -> y = y;
    list -> x = x;
    list -> next = NULL;

    if(*node == NULL)
    {
        *node = list;
    }
    else
    {
        circular_node *array = *node;
        while(array->next != NULL)
        {
          array = array -> next;
        }
        array->next = list;
    }
}
Ken Y-N
  • 14,644
  • 21
  • 71
  • 114
Sage Kase
  • 7
  • 3
  • 2
    You have a pointer `node`, which you initialize to point to `NULL` (i.e. nowhere). That means you can't dereference the pointer (i.e. `*null` is invalid and leads to *undefined behavior*). What is the reason for the variable to be a pointer to a pointer? – Some programmer dude Apr 13 '22 at 07:09
  • 2
    `circular_node *list = (struct linked_list *)calloc(1, sizeof(struct linked_list));` is a problem to review as questionable type. For better code, use `circular_node *list = calloc(1, sizeof list[0]);` and avoid wrong type sizing. – chux - Reinstate Monica Apr 13 '22 at 07:11
  • 1
    Use your debugger, single step through your code. – the busybee Apr 13 '22 at 07:13
  • Also, I'd [use `malloc()` instead of `calloc()`](https://stackoverflow.com/q/8106782/1270789) here. – Ken Y-N Apr 13 '22 at 07:15
  • @Some programmer dude, I am trying to append a node to the end of a linked list, sir. – Sage Kase Apr 13 '22 at 07:59
  • 1
    You don't need a pointer to a pointer for that. Especially since you don't make the pointer actually point somewhere valid. – Some programmer dude Apr 13 '22 at 08:11
  • Technically, you haven't provided a [mre] so the above is not running. `circular_node` is not declared so doesn't even compile. You don't check return value of calloc. – Allan Wind Apr 13 '22 at 08:57
  • @Allan Wind. I am sorry. I will take this into consideration. Thank you. – Sage Kase Apr 13 '22 at 09:44
  • A pointer to pointer is only useful when you wish to allocate an array of pointers (not applicable for linked lists) or in case you wish to return a pointer to the caller through one of the parameters of a function. – Lundin Apr 13 '22 at 10:30
  • @SageKase No need to be sorry. If you write a better question as outlined in that article, you will get better answers. – Allan Wind Apr 13 '22 at 18:28

1 Answers1

0

@Someprogrammerdude already explained the issue with the **node. A suitable fix would be to use a regular pointer (instead of pointer to pointer). Here is a working example (also swapped x and y because it unnecessary confusing otherwise):

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

struct linked_list {
    int x;
    int y;
    struct linked_list *next;
};

struct linked_list *node = NULL;

void list_build(int x, int y) {
    struct linked_list *list = malloc(sizeof(struct linked_list));
    if(!list) {
       printf("malloc failed\n");
       return;
    }
    list->x = x;
    list->y = y;
    list->next = NULL;

    if(!node) {
        node = list;
        return;
    }

    struct linked_list *n;
    for(n = node; n->next; n = n->next);
    n->next = list;
}

void list_print() {
    for(struct linked_list *n = node; n; n = n->next) {
        printf("x = %d, y = %d\n", n->x, n->y);
    }
    printf("\n");
}


int main() {
    list_build(1, 2);
    list_print();
    list_build(3, 4);
    list_print();
}

Consider making node a local variable and pass it to the two functions. Or at least make it static to make it file local so you don't accidentally export that generic symbol name.

Allan Wind
  • 23,068
  • 5
  • 28
  • 38