C, Getting segmentation fault

Question

I have a file called islands.txt with the contents:

islandone
islandtwo
islandthree

And this my code:

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

typedef struct island{
    char *name;
    struct island *previous;
} island;

void printIsland(island is){
    printf("%s", is.name);
    if(is.previous && is.previous->name[0] != '\0'){
        printf("%s", is.previous->name);
    }
}

int main(){

    // the file to be read.
    FILE *islandsFile = fopen("islands.txt","r");

    // temporary location to store the name read from the file.
    char name[40];

    // temporary pointer to an island which has been already read for linking.
    island *previousIsland;

    while(fscanf(islandsFile,"%s",name) != EOF){
        // allocate space for a new island and point to it with (*newIsland) pointer
        island *newIsland =malloc(sizeof(island));

        // assign name
        newIsland->name = name;

        // if previousIsland pointer is not null
        // it means there is an island that was read before newIsland in the file

        if(previousIsland){
            // newIsland.previous should hold the address of this previously read island..
            newIsland->previous = previousIsland;
        }
        // now previousIsland is the newIsland..
        previousIsland = newIsland;
        printIsland(*newIsland);
        puts("");
    }

    fclose(islandsFile);
}

My expectation of output is:

islandone
islandtwoislandone
islandthreeislandtwo

Instead all I am getting is segmentation fault. I have tried everything but I am stuck. Where am I getting the segmentation fault here? I am pretty new to C and I have no idea how to debug.

Answer 1

There are several problems here. In addition to those mentioned by CyberSpock, you have the following code:

island *previousIsland; 

while(fscanf(islandsFile,"%s",name) != EOF){
   /* some code omitted */

   if(previousIsland){
        newIsland->previous = previousIsland;
   }

The previousIsland variable is uninitialized, and the if may be true the first time, so that the previous pointer points to invalid memory. Then when you get to the end in printIsland, it will continue following the uninitialized pointer, getting to invalid memory. I also see that you don't free() any memory, but that may be becuase you don't care for such a small example.

To debug C programs, a debugger is you friend. Now you don't tell which OS and compiler you use, but if you use gcc, gdb is the matching debugger.

Answer 2

Yes you need to allocate memory for the name as well. You only allocate for the structure

typedef struct island{
    char *name;
    struct island *previous;
} island;

so this

// assign name
newIsland->name = name;

will set the pointer to your array which you have on the stack, but by every loop iteration it will be the same address.

instead do something like

newIsland->name = strdup(name);

or if you prefer

newIsland->name = malloc( strlen( name ) + 1 );
strcpy( newIsland->name, name );