Initializing struct pointer gives segmentation fault

Question

I've been trying to initialise a linked list in a for loop. Every iteration, I create a pointer to a node struct and point the last node in the list to it. However, something strange that happens is I keep getting a segmentation fault when trying to assign the value of the data field of the struct to 5 (second line in the for loop). Not sure why this is happening.

struct node{
  int data;
  node* next;
};

void CreateLinkedList(node* start, int numberOfNodes)
{
  int i = 0;
  node* tempo = start;

  for(i=0; i<numberOfNodes; i++){
      node* newNode;
      newNode->data = 5;
      newNode->next = NULL;
      while(tempo->next != NULL){
        tempo = tempo->next;
      }
      tempo->next = newNode;
  }
}

Something else I tried was using the "new" operator and it worked (again, not sure why):

void CreateLinkedList(node* start, int numberOfNodes)
{
  int i = 0;
  node* tempo = start;
  node* newNode;

  for(i=0; i<numberOfNodes; i++){
      newNode = new node;
      newNode->data = 5;
      newNode->next = NULL;
      while(tempo->next != NULL){
        tempo = tempo->next;
      }
      tempo->next = newNode;
  }
}

Any ideas?

PS: This is my very first post here!

Welcome to Stack Overflow. This seems a good start to make use of your debugger and a good C++ book - https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list. — Michael Chourdakis, May 16 '22 at 20:18
in your first example, `newNode` is uninitialized, so dereferencing it with `newNode->` invokes [undefined behavior](https://en.wikipedia.org/wiki/Undefined_behavior). The `new` operator creates a new `node` object, including allocating memory for it. And you should remove the C tag since this is C++ code, there is no `new` operator in C. — yano, May 16 '22 at 20:27
`node* newNode; newNode->data = 5;` creates an uninitialized pointer that can point absolutely anywhere. You then dereference it and start writing to that "random" memory location. Your program did not request memory to be allocated. You have no right to do this. With compiler warnings enabled, your compiler would scream at you when encountering this code. Calling `new` to actually allocate memory and store that location in the `newNode` pointer is the correct approach, although with modern C++ you may consider using smart pointers instead (e.g. `std::unique_ptr`). — paddy, May 16 '22 at 20:27
Single list linked list should eigther be created back to front or keep a pointer to the last item so you can just attach the next one directly without having to traverse the list every time. — Goswin von Brederlow, May 16 '22 at 20:52

Vlad from Moscow · Answer 1 · 2022-05-16T20:33:18.200

The both functions are incorrect. In the first function you are using at least the uninitialized pointer newNode with an indeterminate value to access memory that invokes undefined behavior

for(i=0; i<numberOfNodes; i++){
    node* newNode;
    newNode->data = 5;
    newNode->next = NULL;
    //..

Nevertheless even using the operator new in the second function

for(i=0; i<numberOfNodes; i++){
    newNode = new node;
    newNode->data = 5;
    newNode->next = NULL;
    //...

does not make the function correct.

The user can pass a null pointer to the function (when the list is empty). In this case you have the same problem of accessing memory using a null pointer

while(tempo->next != NULL){
      ^^^^^^^^^^

The function can be declared and defined the following way

void CreateLinkedList( node * &start, size_t numberOfNodes )
{
    node **current = &start;

    while (numberOfNodes--)
    {
        *current = new node{ 5, nullptr };
        current = &( *current )->next;
    }
}

So just to clarify, if I have a normal struct, I can initialize it using "myStruct.field=..." and going through all the fields. But when I have a pointer to a struct, I need to use the "new" operator to initialize it? — Hamza, May 16 '22 at 20:42
@Hamza It will be correctly to say that if you have an object of the structure then you can use the dot operator. If you have a pointer then the pointer shall point to a valid object to be able to access members of the object using the pointer. — Vlad from Moscow, May 16 '22 at 20:45

score 0 · Answer 2 · answered May 16 '22 at 20:46

I'm agree with Vlad. First of all when you are supposed to set data after construct the struct, and you just are creating a new pointer of that struct. that means that your memory is not already allocated and you are trying to access into a non-accessible region of memory. In order to solve that you need to create some constructor (or use the default), and then create the object with the new + the constructor. Some naïve version can be.

    struct node{
      int data;
      node* next;
      
      node()
      {
       data = 0;
       node* = nullptr;
      }
   };
      //.
      //.
      //.
   for(i=0; i<numberOfNodes; i++){
      node* new node();
      //.
      //.
      //.

Also as Vlad told you it's important to take care of the while clause since the function can receive some null value, so take care of that too.

score 0 · Answer 3 · answered May 16 '22 at 21:07

C++ has constructors to initialize structures so they are never in some undefined state. So use them.

struct Node {
    int data{};
    Node* next{nullptr};

    Node(Node **head, int data_) : data{data_} {
        while(*head != nullptr) head = &(*head)->next;
        *head = this;
    }
};

This then simplifies the function:

Node * CreateLinkedList1(int numberOfNodes) {
    Node *head{nullptr};
    for(int i=0; i < numberOfNodes; i++) {
        new Node(&head, 5);
    }
    return head;
}

It would be much faster though to remember the tail of the list so you could append to it directly.

Node * CreateLinkedList2(int numberOfNodes) {
    Node *head{nullptr};
    Node **tail{head};
    for(int i=0; i < numberOfNodes; i++) {
        tail = &(new Node(tail, 5))->next;
    }
    return head;
}

Initializing struct pointer gives segmentation fault

3 Answers3