Segmentation Fault in C using fgets(), strtok(), strcpy()

Question

I am getting a seg fault from the code shown below. I am trying to create a simple database that reads a stdin from a bin file chops it up by commas, throws each value into an array and throws it in a struct. I want to do this for every line in the standard input and then write the struct to a file at the end.

I am calling this loadDatabase function from main.

The data looks like the following in one line and is about 14 lines long:

34156155,MILES,NORMA,TAMMY,n/a,9/16/1964,FEMALE,123-45-6789,LAGUARDIA RD,SHLT,10915,n/a,n/a,CHESTER,NY,848-896-8296,n/a,NMILES@AMGGT.COM,n/a

Here is my current code. Please excuse me if my C is really bad. First time...:

struct _record {
    char ID[25];
    char lname[25]; // last name
    char fname[25]; // first name
    char mname[25]; // middle name
    char suffix[25];    
    char bday[25];  
    char gender[25];    
    char SSN[25];   
    char add1[25]; //address 1
    char add2[25]; //address 2 
    char zip[25];
    char maiden[25];
    char MRN[25];
    char city[25];
    char state[25];
    char phone1[25];
    char phone2[25];
    char email[25];
    char alias[25];
};

bool loadDatabase(char *db_name) {
    printf("Loading Database...");
    char buffer[400];
    FILE *fp;
    int x;
    fp = fopen(db_name, "wb"); //write & binary option      
    if (fp == NULL) {
        puts(" ERROR: FILE CANNOT BE OPENED");
        return false; 
    } else {
        struct _record record; 
        while (fgets(rec, sizeof(rec), stdin) != NULL) {
            value = strtok(NULL, ",");
            flds[0] = strdup(value);

            //load lname
            value = strdup(NULL, ",");
            flds[1] = strdup(value);

            // load fname
            value = strdup(NULL, ",");
            flds[2] = strdup(value);

            // load mname
            value = strtok(NULL, "\n");
            flds[3] = strdup(value);
            // did not write the rest bc of the seg fault

            strcpy(record.ID, flds[0]);
            strcpy(record.lname, flds[1]);
            strcpy(record.fname, flds[2]);
            strcpy(record.mname, flds[3]);
            strcpy(record.suffix, flds[4]);
            strcpy(record.bday, flds[5]);  
            strcpy(record.gender, flds[6]);  
            strcpy(record.SSN, flds[7]);  
            strcpy(record.add1, flds[8]);  
            strcpy(record.add2, flds[9]); 
            strcpy(record.zip, flds[10]);
            strcpy(record.maiden, flds[11]);
            strcpy(record.MRN, flds[12]);
            strcpy(record.city, flds[13]);
            strcpy(record.state,  flds[14]);
            strcpy(record.phone1, flds[15]);
            strcpy(record.phone2, flds[16]);
            strcpy(record.email, flds[17]);
            strcpy(record.alias, flds[18]);
        }
        printf("ID: %s", record.ID);
        fwrite(record, sizeof(struct _record), 1, fp);
        fclose(fp);
    }
    return true;
}

Answer 1

There are multiple problems in your code:

• the definition of fld is not provided. It should be defined as a local array of 19 char *:

  char *fld[19];
  

• you have some cut+paste bugs: value = strdup(NULL, ","); instead of value = strtok(NULL, ",");

• many lines are missing.

• you never test if strtok() returns NULL. Invalid input will cause undefined behavior

• memory for the strings is unnecessary: you can just copy the string directly into the record field.

• you do not check the length of the strings before copying them with strcpy. Invalid input may cause buffer overflows.

• the argument to fwrite should be the address of the record, not its value:

  fwrite(&record, sizeof(struct _record), 1, fp);
  

• using strtok() (or sscanf() with %[^,] conversion specifiers) does not handle empty fields correctly: strtok() will consider any sequence of , as a single separator (%[^,] will not match an empty field either). I suggest using a function for this.

• the record structure should be cleared before each line to avoid storing uninitialized contents into the database file.

To avoid some of these problems, you should raise the warning level and let the compiler produce diagnostics for common programming errors: gcc -Wall -Wextra -Werror or clang -Weverything or cl /W4.

Here is an improved version:

#include <errno.h>
#include <stdio.h>
#include <string.h>
#include <stdbool.h>

struct _record {
    char ID[25];
    char lname[25]; // last name
    char fname[25]; // first name
    char mname[25]; // middle name
    char suffix[25];
    char bday[25];
    char gender[25];
    char SSN[25];
    char add1[25]; //address 1
    char add2[25]; //address 2
    char zip[25];
    char maiden[25];
    char MRN[25];
    char city[25];
    char state[25];
    char phone1[25];
    char phone2[25];
    char email[25];
    char alias[25];
};

bool loadField(char *dest, int size, char **cursorp) {
    bool truncated = false;
    int i = 0;
    char *p;
    for (p = *cursorp; *p != '\0' && *p != '\n'; p++) {
        if (*p == ',') {
            p++;  // skip the comma separator
            break;
        }
        if (i + 1 < size) {
            dest[i] = *p;
        } else {
            truncated = 1;
        }
        i++;
    }
    // pad the field with null bytes
    while (i < size) {
        dest[i++] = '\0';
    }
    *cursorp = p;
    if (truncated) {
        fprintf(stderr, "field too long: %.*s\n", i, *cursorp);
        return false;
    } else {
        return true;
    }
}

bool loadDatabase(const char *db_name) {
    char buffer[1000];
    FILE *fp;

    printf("Loading Database...");
    fp = fopen(db_name, "wb"); //write & binary option
    if (fp == NULL) {
        fprintf(stderr, "error: cannot open file %s: %s\n", db_name, strerror(errno));
        return false;
    } else {
        struct _record record;    // clear the record
        while (fgets(buffer, sizeof(buffer), stdin) != NULL) {
            char *cursor = buffer;

            memset(&record, 0, sizeof(record));    // clear the record
            loadField(record.ID, sizeof(record.ID), &cursor);
            loadField(record.lname, sizeof(record.lname), &cursor);
            loadField(record.fname, sizeof(record.fname), &cursor);
            loadField(record.mname, sizeof(record.mname), &cursor);
            loadField(record.suffix, sizeof(record.suffix), &cursor);
            loadField(record.bday, sizeof(record.bday), &cursor);
            loadField(record.gender, sizeof(record.gender), &cursor);
            loadField(record.SSN, sizeof(record.SSN), &cursor);
            loadField(record.add1, sizeof(record.add1), &cursor);
            loadField(record.add2, sizeof(record.add2), &cursor);
            loadField(record.zip, sizeof(record.zip), &cursor);
            loadField(record.maiden, sizeof(record.maiden), &cursor);
            loadField(record.MRN, sizeof(record.MRN), &cursor);
            loadField(record.city, sizeof(record.city), &cursor);
            loadField(record.state, sizeof(record.state), &cursor);
            loadField(record.phone1, sizeof(record.phone1), &cursor);
            loadField(record.phone2, sizeof(record.phone2), &cursor);
            loadField(record.email, sizeof(record.email), &cursor);
            loadField(record.alias, sizeof(record.alias), &cursor);
            printf("ID: %s\n", record.ID);
            if (fwrite(&record, sizeof(record), 1, fp) != 1) {
                fprintf(stderr, "error: cannot write record: %s\n", strerror(errno));
                break;
            }
        }
        fclose(fp);
    }
    return true;
}

int main(void) {
    if (loadDatabase("database.bin"))
        return 1;
    return 0;
}