C Programming: Reading data from a file, dynamically allocating memory, placing contents in struct array

Question

This is my very first post on stackoverflow. I am a CS student learning C, and I am having some issues with the problem I'm working on. Additionally, I should mention that I know very little, so if anything I put here comes off as foolish or ignorant, it is absolutely not my intention

I am aware that there are other posts similar to this one, however so far I feel that I have tried making a lot of amendments that all end with the same result.

I am given a text file in which each line contains studentName(tab)gpa. The total size of the file is unknown, this I must use dynamic memory allocation.

Example of text file format

Jordan  4.0
Bhupesh 2.51

General steps for program

Many details will be left out to save myself from embarrassment, however I will give a high-level overview of the process I am struggling with:

 1.) Create dynamic memory array to hold struct for each line
 2.) Start looping through file
 3.) check the current size of the array to see if reallocation is necessary
 4.) Create dynamic array to hold name
 5.) Place name and gpa into struct
 6.) rinse & repeat

Finally, one last thing. The error occurs when my initial allocated memory limit is reached and the program attempts to reallocate more memory from the heap.

Screenshot of error being thrown in clion debugger

My code is shown below:

#define EXIT_CODE_FAIL 1
#define ROW_COUNT 10
#define BUFFER_SIZE 255
#define VALID_ARG_COUNT 2

struct Student {
    float gpa;
    char * name;
};

// read the file, pack contents into struct array
struct Student * readFileContents(char *filename, int *rowCounter) {

    // setup for loop
    int maxDataSize = ROW_COUNT;
    float currentStudentGpa = 0;
    char studentNameBuffer[BUFFER_SIZE];

    // initial structArray pre-loop
    struct Student * structArray = calloc(maxDataSize, sizeof(*structArray));

    FILE *pFile = fopen(filename, "r");
    validateOpenFile(pFile);


    // loop through, get contents, of eaach line, place them in struct
    while (fscanf(pFile, "%s\t%f", studentNameBuffer, &currentStudentGpa) > 0) {
        structArray = checkArraySizeIncrease(*rowCounter, &maxDataSize, &structArray);
        structArray->name = trimStringFromBuffer(studentNameBuffer);
        structArray->gpa = currentStudentGpa;
        (*rowCounter)++, structArray++;
    }

    fclose(pFile);

    return structArray;
}

// resize array if needed
struct Student * checkArraySizeIncrease(int rowCount, int * maxDataSize, struct Student ** structArray) {

    if (rowCount == *maxDataSize) {
        *maxDataSize += ROW_COUNT;
        
        **// line below is where the error occurs** 
        struct Student * newStructArray = realloc(*structArray, *maxDataSize * sizeof(*newStructArray));
        validateMalloc(newStructArray);

        return newStructArray;
    }
    return *structArray;
}

// resize string from initial data buffer
char *trimStringFromBuffer(char *dataBuffer) {

    char *string = (char *) calloc(strlen(dataBuffer), sizeof(char));
    validateMalloc(string);
    strcpy(string, dataBuffer);

    return string;
}

Once again, I apologize if similar questions have been asked, but please know I have tried most of the recommendations that I have found on stack overflow with no success (of which I'm well aware is the result of my poor programming skill level in C).

I will now promptly prepare myself for my obligatory "first post on stackoverflow" roasting. Cheers!

Answer 1

You are reusing structArray as both the base of the array and a pointer to the current element. This won't work. We need two variables.

There are a number of "loose" variables related to the dynamic array. It's cleaner to define a struct (e.g. dynarr_t below) to contain them and pass just the struct pointer around.

When you're duplicating the string, you must allocate strlen + 1 [not just strlen]. But, the entire function does what strdup already does.

I tried to save as much as possible, but I've had to refactor the code a fair bit to incorporate all the necessary changes.

By passing sizeof(*structArray) to the arrnew function, this allows the struct to be used for arbitrary size array elements.

Anyway, here's the code:

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

#define sysfault(_fmt...) \
    do { \
        printf(_fmt); \
        exit(1); \
    } while (0)

#define EXIT_CODE_FAIL 1
#define ROW_COUNT 10
#define BUFFER_SIZE 255
#define VALID_ARG_COUNT 2

struct Student {
    float gpa;
    char *name;
};

// general dynamic array control
typedef struct {
    void *base;                         // base address
    size_t size;                        // bytes in array element
    size_t count;                       // current number of used entries
    size_t max;                         // maximum number of entries
    size_t grow;                        // number of entries to grow
} dynarr_t;

// arrfind -- return pointer to array element
void *
arrfind(dynarr_t *arr,size_t idx)
{
    void *ptr;

    ptr = arr->base;
    idx *= arr->size;
    ptr += idx;

    return ptr;
}

// arrnew -- create new array control
dynarr_t *
arrnew(size_t siz,size_t grow)
// siz -- sizeof of array element
// grow -- number of elements to grow
{
    dynarr_t *arr;

    arr = calloc(1,sizeof(*arr));
    if (arr == NULL)
        sysfault("arrnew: calloc fail -- %s\n",strerror(errno));

    arr->size = siz;
    arr->grow = grow;

    return arr;
}

// arrgrow -- grow array [if necessary]
// RETURNS: pointer to element to fill
void *
arrgrow(dynarr_t *arr)
{
    void *ptr;

    // grow array if necessary
    // NOTE: use of a separate "max" from "count" reduces the number of realloc
    // calls
    if (arr->count >= arr->max) {
        arr->max += arr->grow;
        arr->base = realloc(arr->base,arr->size * arr->max);
        if (arr->base == NULL)
            sysfault("arrgrow: realloc failure -- %s\n",strerror(errno));
    }

    // point to current element
    ptr = arrfind(arr,arr->count);

    // advance count of elements
    ++arr->count;

    return ptr;
}

// arrtrim -- trim array to actual number of elements used
void
arrtrim(dynarr_t *arr)
{

    arr->base = realloc(arr->base,arr->size * arr->count);
    if (arr->base == NULL)
        sysfault("arrtrim: realloc failure -- %s\n",strerror(errno));

    arr->max = arr->count;
}

void
validateMalloc(void *ptr)
{

    if (ptr == NULL) {
        perror("validateMalloc");
        exit(1);
    }
}

void
validateOpenFile(FILE *ptr)
{

    if (ptr == NULL) {
        perror("validateOpenFile");
        exit(1);
    }
}

// resize string from initial data buffer
char *
trimStringFromBuffer(char *dataBuffer)
{

#if 0
#if 0
    char *string = calloc(1,strlen(dataBuffer));
#else
    char *string = calloc(1,strlen(dataBuffer) + 1);
#endif
    validateMalloc(string);
    strcpy(string, dataBuffer);
#else
    char *string = strdup(dataBuffer);
    validateMalloc(string);
#endif

    return string;
}

// read the file, pack contents into struct array
dynarr_t *
readFileContents(char *filename)
{
    dynarr_t *arr;

    // setup for loop
    float currentStudentGpa = 0;
    char studentNameBuffer[BUFFER_SIZE];
    struct Student *structArray;

    arr = arrnew(sizeof(*structArray),10);

    FILE *pFile = fopen(filename, "r");
    validateOpenFile(pFile);

    // loop through, get contents, of eaach line, place them in struct
    while (fscanf(pFile, "%s\t%f", studentNameBuffer, &currentStudentGpa) > 0) {
        structArray = arrgrow(arr);
        structArray->name = trimStringFromBuffer(studentNameBuffer);
        structArray->gpa = currentStudentGpa;
    }

    fclose(pFile);

    arrtrim(arr);

    return arr;
}

Answer 2

I think your issue is with the calculation of the size of the realloc. Rather than using sizeof(*newStructArray), shouldn't you really be using the size of your pointer type? I would have written this as realloc(*structArray, *maxDataSize * sizeof(struct Student *))

There's a lot of other stuff in here I would never do - passing all those variables in to checkArraySizeIncrease as pointers is generally a bad idea because it can mask the fact that things are getting changed, for instance.

Answer 3

There is an issue in allocation of the buffer for string

char *string = (char *) calloc(strlen(dataBuffer), sizeof(char));

it should be:

char *string = (char *) calloc(1 + strlen(dataBuffer), sizeof(char));

as C-strings require extra 0-byte at the end. Without it, the following operation:

strcpy(string, dataBuffer);

may damage data after the buffer, likely messing malloc() metadata.