Segmentation fault accessing struct member

Question

I've been learning C and am having problems using linked lists. When looping over a pointer to a linked list I run into segmentation faults and I'm not sure why.

Looking at similar questions the suggestion is to allocate the memory, but I find this answer confusing. Do you have to use heap memory for linked lists, and if so why?

Here is my code:

#include <stdio.h>

typedef struct Node {
  char *name;
  struct Node *next;
} Node;

typedef struct Thing {
  Node *node;
} Thing;

Thing make_thing()
{
  Thing t = {
    .node = NULL
  };
  return t;
}

Thing * add_node(Thing *t, char *name)
{
  Node node = {
    .name = name,
    .next = t->node
  };

  t->node = &node;

  return t;
}

void print_nodes(Thing *t)
{
  Node *n = t->node;

  while(n != NULL) {
    printf("Node: %s\n", n->name);
    n = n->next;
  }
}

int main()
{
  printf("Start\n");

  Thing t = make_thing();
  add_node(&t, "one");

  printf("First %s\n", t.node->name);

  print_nodes(&t);

  return 0;
}

Where is the error message? The cryptic text can be very helpful in pinpointing the problem cause. — donjuedo, Dec 04 '19 at 12:55
regarding: `t->node = &node;` this is setting the `t->node` with the address of some local variable. That local variable goes out-of-scope when the function exits. I.E, it does not exist anymore — user3629249, Dec 05 '19 at 03:49

score 7 · Accepted Answer · answered Dec 04 '19 at 12:57

7

You are using objects with automatic storage out of their scope:

Node node = {
  .name = name,
  .next = t->node
};

t->node = &node;

return t;

Here you leak the pointer &node, which is invalid (out of scope) after the return, to the caller and use it here:

 printf("First %s\n", t.node->name);

You have to allocate memory by using malloc() for your Node structure.

Example:

 Node *node = malloc(sizeof *node);
 node->name = name;
 node->next = t->node;
 t->node = node;

 return t;

You have to care about freeing the memory when it is no longer used to prevent memory leaks.

answered Dec 04 '19 at 12:57

Ctx

18,090
24
36
51

I think you have not answered actual question.`(Do you have to use heap memory for linked lists, and if so why?)`. – kiran Biradar Dec 04 '19 at 13:07
1

@kiranBiradar Well, the question was: "Do you have to use heap memory for linked lists, and if so why?" and the response is: "You are using objects with automatic storage out of their scope" and " have to allocate memory by using malloc() for your Node structure". How does this not answer the question? – Ctx Dec 04 '19 at 13:09
Thank you. Does this mean I have to change the *design* of this type of code? Meaning I can't have a function like add_node which mutates a struct. I must design it differently so that this memory can be later freed. – Matthew Dec 04 '19 at 13:10
1

@Matthew You can use the function add_node() as it is, you just have to make sure, that you free each memory block, before the _last_ reference to it vanishes. I.e. you can iterate over the linked list and free each item. – Ctx Dec 04 '19 at 13:11
@Ctx I'm a little confused by what the scope is here. I can understand what you are saying that the new Node in add_node is scoped to add_node. But why isn't Thing created in make_thing scoped to make_thing? – Matthew Dec 04 '19 at 13:11
1

@Matthew Because you return the struct itself by value there, not a reference to it. – Ctx Dec 04 '19 at 13:12
Ok, I think I understand a little bit better now. Thank you so much for your help. – Matthew Dec 04 '19 at 13:13
@Ctx if you don't mind one more question, why is it the `sizeof *node` and not `sizeof Node` ? – Matthew Dec 04 '19 at 15:51
1

@Matthew `*node` has the type `Node`, so both values are equivalent. It is a matter of style, which to use. The way I chose has the advantage, that you can change the type in only one place (e.g. `Node` to `Node2`), in the latter case, two occurrences have to be replaced. But you are free to use the other version, of course – Ctx Dec 04 '19 at 15:58

Segmentation fault accessing struct member

1 Answers1