Using fgets to get lines of text from a file and store them into an array

Question

I am struggling to get lines of text from a file and storing them into an array. I have used a debugger on my code and it seems to get the first couple lines of text but then there is a segmentation fault when it gets to the third line of text. I believe there is something wrong with my allocate_mem() function because the fault occurs when it is called during the 3rd iteration of the while loop in read_lines(). Help would be much appreciated!

My Code:

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

char* read_one_line(FILE* fp, char* line);
char* allocate_mem(FILE* fp, char* line);

void print_lines(char** lines, int num_lines){
    int i;
    for(i = 0 ; i < num_lines; ++i){
        printf("%d. %s", i+1, lines[i]);
    }
}

void free_lines(char** lines, int num_lines){
    int i;

    for(i = 0 ; i < num_lines; ++i){
        free(lines[i]);
    }

    if(lines != NULL && num_lines > 0){
        free(lines);
    }
}

FILE* validate_input(int argc, char* argv[]){

    FILE* fp = NULL;

    if(argc < 2){
        printf("Not enough arguments entered.\nEnding program.\n");
        exit(0);
    }
    else if(argc > 2){
        printf("Too many arguments entered.\nEnding program.\n");
        exit(0);
    }

    fp = fopen(argv[1], "r");

    if(fp == NULL){
        printf("Unable to open file: %s\nEnding program.\n", argv[1]);
        exit(0);
    }
    return fp;
}

void read_lines(FILE* fp, char*** lines, int* num_lines) {
  *num_lines = 0;
  do {
    *num_lines += 1;
    *lines = realloc((*lines), (*num_lines) * sizeof(*lines));
    (*lines)[*num_lines - 1] = read_one_line(fp, (*lines)[*num_lines - 1]);
  } while((*lines)[*num_lines - 1] != NULL);
  free((*lines)[*num_lines - 1]);
  *num_lines -= 1;
}

char* read_one_line(FILE* fp, char* line) {
  line = NULL;
  int str_len = 0;

  while (1) {
        // resize buffer to hold next char, or zero termination
        char *tmp = realloc(line, str_len + 1);
        if (!tmp) {
          free(line);
          return NULL;
        }
        line = tmp;
        // Get next character from file
        int ch = fgetc(fp);
        if (ch == EOF) {
          free(line);
          return NULL;
        }
        else if (ch == '\n') {
          line[str_len] = '\n';
          line[str_len + 1] = 0;
          return line;
        }
        else {
             line[str_len] = ch;
             str_len++;
        }
    }
}

int main(int argc, char* argv[]){
    char** lines = NULL;
    int num_lines = 0;
    FILE* fp = validate_input(argc, argv);

    read_lines(fp, &lines, &num_lines);
    print_lines(lines, num_lines);
    free_lines(lines, num_lines);
    fclose(fp);

    return 0;
}

Answer 1

When allocating memory for a line of text, before reading the line, we have to do some magic with ftell, reading and discarding, fseek, memory allocation, and actual reading. This is not that hard to get right, but not all streams support seeking and we have to account for the file changing during operations. Failure do account for file changes may or may not be a security issue.

The easiest way is not to preallocate the correct size, but to but to start with a small string buffer, and resize the buffer as we read more characters.

/* 
   Read a line of text from `file` and store it in a freshly allocated 
   buffer

   Return new string or NULL on error or premature EOF
 */
char *alloc_and_read_line(FILE *file)
{
   char *str = NULL; 
   size_t str_len = 0

   while (1)
     {
        /* resize buffer to hold next char, or zero termination */
        char *tmp = realloc(str, str_len + 1);
        if (!tmp)
          {
             free(str);
             return NULL;
          }
        str = tmp;

        /* Get next character from file */
        int ch = fgetc(file);
        if (ch == EOF)
          {
             /* 
               IO error in `file`, or last line was not correctly
               terminated with newline
              */ 
             free(str);
             return NULL;
          }
        else if (ch == '\n')
          {
             /* End of line */
             str[str_len] = 0;
             return str;
          }
        else
          {
             str[str_len] = ch;
             str_len++;
          }
     }
}

Some people prefer to resize the buffer in larger steps than 1, or double the size when it needs to grow. The theory is that it should be faster, but it is not really an issue on modern realloc implementations (I have measured). It would only add more complexities and chances for bugs.

EDIT:

About your new segmentation error, *foo[bar] is the same as *(foo[bar]) not (*foo)[bar]. When it comes to operator precedence, the compiler will happily have a different opinion than what you wished for. The liberal use of parentheses doesn't hurt.