Getting Segmentation Fault in LinkedList

Question

I m getting Segmentation fault in this code but i don't see any reason why I m checking if head is NULL then it will be intitialized to temp.

void insertFirst(int id,char *name){
    struct Student *temp;
    temp->id=id;
    strcpy(temp->name,name);
    temp->next=NULL;
    temp->prev=NULL;
    if(head==NULL){
        head=temp;
    } else{
        temp->next=head;
        head->prev=temp;
        head=temp;
    }
}
void append(int id, char *name){
    struct Student* temp;
    temp->id=id;
    strcpy(temp->name,name);
    temp->next=NULL;
    temp->prev=NULL;
    if(head==NULL){
        head=temp;
    } else{
        while(head->next!=NULL){
            head=head->next;
        }
        head->next=temp;
        temp->prev=head;
    }
}

Please try to create a [Minimal, Complete, and Verifiable Example](http://stackoverflow.com/help/mcve) and show us. Also please take some time to read [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) by Eric Lippert. — Some programmer dude, Jun 29 '17 at 07:32
There are also many other problems with your code. Like if `Student::name` is a pointer and not an array? Like you modifying `head` in the loop in the `append` function? Like you not making `temp` actually point anywhere! — Some programmer dude, Jun 29 '17 at 07:36
Proper indentation will go a long way towards making your code readable and debuggable. — Sebastian Lenartowicz, Jun 29 '17 at 12:27
This really looks more like C code than C++ (for starters you don't need to write `struct Student` in C++, just `Student` is enough) — UnholySheep, Jun 29 '17 at 12:28
So your code is full of *undefined behavior* (as your `temp` pointer doesn't point to anything valid) - it might be time to read [a good C++ book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) — UnholySheep, Jun 29 '17 at 12:48

score 0 · Accepted Answer · answered Jun 29 '17 at 14:14

There're many problems with this code.

Just a working example

Complete example

Errors with explanations

Assumptions

So far we've no complete problem definition, so I've made several assumtions about your code.

Student definition

struct Student {
  int id;
  char* name;
  Student* next;
  Student* prev;
};

head is global variable

Fatal errors

Uninitialized pointers

Pointer is just a integer of some sort. So when you write:

struct Student* temp;

You just create integer value without initialization. It's better to always initialize variables properly, for instance with NULL value:

struct Student* temp = NULL;

Struct instantiation

Following code doesn't create intsance of Student

struct Student* temp;

Rather, it just creates a pointer, as shown above. To actually instantiate Student you've to write something like this:

struct Student* temp = NULL;
temp = new Student;

or shorlty:

struct Student* temp = new Student;

Here new instance of Student will be created.

Copy string using unitialized pointer

This problem is similar to first one. After instantiation of Student, Student::name points nowhere(actually it points somewhere in memory). Before copy a name you have to allocate memory for that copy & points Student::name there. To do this we've to:

Get amout of memory to be allocated(count of chars in that case).
```
size_t chars_count = strlen(name);
```

Allocate required memory.

char* pointer_to_string = new char[chars_count];

Assign pointer value to Struct::name.
```
temp->name = pointer_to_string;
```

or shortly:

temp->name = new char[strlen(name)];

Moving head in append function

head is global variable that have to point to the beginning of the students list. But in append function, this code:

while(head->next != NULL) {
  head = head->next;
}

will reassign head on every iteration. So after first loop execution head will point to the last list entry. To avoid this we've to use additional variable, points to current element.

struct Student* pCurrent = head;
while(pCurrent->next != NULL) {
  pCurrent = pCurrent->next;
}

Following code will be changed relatively:

pCurrent->next=temp;
temp->prev=pCurrent;

P.S. this is not the complete list of errors in this code, but enought to make it work.

Recomendations

As addition few styles recomendations from my side. Complete code example created according theme.

Code duplication. Each function in this sample creates new Student instance, using exactly same code. To avoid this we've to add new function:

Student* createStudent(const int id, const char* name, Student* pNext, 
                       Student* pPrev) {
  struct Student* pStudent = new Student;
  pStudent->id = id;
  pStudent->name = new char[strlen(name)];
  strcpy(pStudent->name, name);
  temp->next = pNext;
  temp->prev = pPrev;
  return pStudent;
}

Use const whenever possible.

Signatures of both functions contains:

functionName(int id, char* name)

But most likely you wont change provided id or name so replace it with:

functionName(const int id, const char* name)

Pass head as an argument.

We've to avoid usage of global variables to have more control.