Why is reading from file function crashing?

Question

Trying to read multiple line from file to store them in a structure made up of the string elements, however when I run the program it simply crashes and I haven't the faintest idea why.

function in question:

Hashtbl* loadfromfile(Hashtbl* hashtbl, char *path){
    int i = 0;
    char line[100];
    char* string[40];

    FILE *f = fopen(path, "r");

    if(f == NULL){
        printf("FILE NO FOUND!");
    }else{
        while(fgets(line, sizeof(line), f)!=NULL){
           strcpy(string[i],line);
           i++;
       }
       fclose(f);
       for(i = 0; i<(SIZE*2); i++){
            strcpy(hashtbl[i].subscript, string[i]);
            i++;
       }

       for(i = 1; i<(SIZE*2); i++){
           strcpy(hashtbl[i].value, string[i]);
           i++;
       }

       return hashtbl;
       }
}

main.c:

#include <stdio.h>
#include <stdlib.h>
#include "hashtable.h"

int main() {

    Hashtbl* numbers;
    numbers = init_hashtbl(); //init_hashtable initialises numbers
    loadfromfile(numbers, "test.txt");
    for(int i = 0; i<SIZE; i++) {
        printf("%s1", numbers[i].subscript);
        printf("%s2\n", numbers[i].value);
    }
}

Hashtable structure:

typedef struct Hashtbls{
    char *subscript;
    char *value;
} Hashtbl;

init_hasthable function:

   Hashtbl* init_hashtbl(){
       Hashtbl* hashtbl;
       hashtbl = calloc(SIZE, sizeof(Hashtbl));
       for(int i = 0; i<SIZE; i++){
           hashtbl[i].subscript = "ZERO";
           hashtbl[i].value = "ZERO";
       }
       return hashtbl;
   }

Answer 1

You have quite a few problems here:

if(f == NULL){
    printf("FILE NO FOUND!");
}

If the file cannot be opened, then you cannot continue. Also the message might be printed way later, use printf("FILE NOT FOUND!\n"); instead.

char* string[40];
...
while(fgets(line, sizeof(line), f)!=NULL){
    strcpy(string[i],line);
    i++;
}

string is an array of uninitialized pointers, you cannot write anything there. You should do

while(fgets(line, sizeof line, f))
{
    string[i] = malloc(strlen(line) + 1);
    if(string[i] == NULL)
    {
        // error handling is needed
    }
    strcpy(string[i], line);
    i++;
    if(i == sizeof string / sizeof *string)
        break;
}

// or if your system has strdup

while(fgets(line, sizeof line, f))
{
    string[i] = strdup(line);
    if(string[i] == NULL)
    {
        // error handling is needed
    }
    i++;
    if(i == sizeof string / sizeof *string)
        break;
}

Also you are not checking whether you read more than 40 lines. I did that with the the last if. sizeof array / sizeof *array returns the number of elements that an array can hold. Note that this only works for arrays, not pointers, since in general sizeof array != sizeof pointer. Also don't forget to free the allocated memory afterwards.

strcpy(hashtbl[i].subscript, string[i]);
...
strcpy(hashtbl[i].value, string[i]);

Are the subscript and value parameters here initialized in some way? Check your init_hashtbl().

---

EDIT

Now that you posted init_hashtbl:

for(i = 0; i<(SIZE*2); i++){
    strcpy(hashtbl[i].subscript, string[i]);
    i++;
}

You are initializing subscript and value with string literals, they are pointing to read-only memory location, strcpy is going to fail. You have to either allocate memory with malloc or change your structure with arrays.

Option 1

Keep the structure, change init_hashtbl

Hashtbl* init_hashtbl(){
   Hashtbl* hashtbl;
   hashtbl = calloc(SIZE, sizeof(Hashtbl));
   for(int i = 0; i<SIZE; i++){
       hashtbl[i].subscript = malloc(SOME_MAXIMAL_LENGTH + 1);
       strcpy(hashtbl[i].subscript, "ZERO");

       hashtbl[i].value = malloc(SOME_MAXIMAL_LENGTH + 1);
       strcpy(hashtbl[i].value, "ZERO");
   }
   return hashtbl;
}

You should always check the return value of malloc/calloc. Also the problem here is that if you want to copy a string that is longer than SOME_MAXIMAL_LENGTH, you are going to have a buffer overflow. So you should use realloc in the reading routine:

for(i = 0; i<(SIZE*2); i++){
    char *tmp = realloc(hashtbl[i].subscript, strlen(string[i]) + 1);
    if(tmp == NULL)
    {
        // error handling
    }

    hashtbl[i].subscript = tmp;
    strcpy(hashtbl[i].subscript, string[i]);
    i++;
}

If you don't want to deal with realloc here, you have to make sure, that no string[i] is longer than SOME_MAXIMAL_LENGTH.

Option 2

Change you structure and init:

typedef struct Hashtbls{
    char subscript[SOME_MAXIMAL_LENGTH];
    char value[SOME_MAXIMAL_LENGTH];
} Hashtbl;


Hashtbl* init_hashtbl(){
   Hashtbl* hashtbl;
   hashtbl = calloc(SIZE, sizeof(Hashtbl));
   for(int i = 0; i<SIZE; i++){
       strcpy(hashtbl[i].subscript, "ZERO");
       strcpy(hashtbl[i].value, "ZERO");
   }
   return hashtbl;
}

Then in loadfromfile you don't have to deal with the realloc as shown above, you can keep your code. However, you have to check that no string[i] is longer than SOME_MAXIMAL_LENGTH - 1, otherwise buffer overflow.

One last thing, fgets reads a whole line, assuming that the length of the line is lesser than sizeof line, the newline character will be added to the line. You most likely don't want to have that. One way of getting rid of the newline is:

fgets(line, sizeof line, f);
int len = strlen(line);
if(line[len - 1] == '\n')
    line[len - 1] = 0;