Initializing struct pointer gives segmentation fault-CodePudding

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!

CodePudding user response：

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;
    }
}

CodePudding user response：

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.

CodePudding user response：

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;
}