HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Memory-safe file-reading in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
safereadingfilememory

Problem

I am trying to write a single function that will read a certain number of bytes from a C file, or the entire file, based on the arguments. I want it to be memory-safe of course, and deal with NULL terminators and the like. Not only that, but it should be efficient.

#include 
#include 
#include 

char *read(FILE *file, long bytes) {
    /* This function takes a FILE pointer  and reads  bytes from it. These
     * bytes are returned as a char* back to the calling function and should be freed
     * since they were malloc'd. This function was an attempt to recreate the
     * .read( [bytes] ) method on python file objects */

    /**********************************
     Set bytes to 0 to read entire file
     **********************************/

    long fileSize = 0; // File size to zero before byte check

    if (bytes == 0) {
        fseek(file, 0L, SEEK_END); // seek to end
        fileSize = ftell(file); // find size of file by checking end
        rewind(file); // back to the beginning for reading
    } else {
        fileSize = ++bytes; // extra byte for NULL 
    }

    char buffer[16]; // We will read the file 16 bytes at a time
    char *file_out = malloc(fileSize + 1);
    if (file_out == NULL) {
         fputs("Could not allocate enough memory", stderr);
         exit(EXIT_FAILURE);
    }

    while (fgets(buffer, 16, file) != NULL) {
        strncat(file_out, buffer, 16);
    }

    return file_out;
}


Is this efficient enough for large files and fringe cases?

Solution

Instead of fgets, you can use fread to read directly into the file_out:

int totalRead = 0;
int size;
while( fileSize > total_read && (size = fread(file_out + totalRead, 1, fileSize - totalRead, file) != 0) {
    totalRead+=size;
}
if(totalRead < fileSize){
    //couldn't read entire file
}
file_out[total_read] = '\0';

return file_out;


This avoids the \$O(n^2)\$ complexity of doing strncat on a growing string.

Doing an exit from within a helper function is not recommended because the calling code cannot recover from it. Instead, return a special value that specifies something went wrong.

Also, make up your mind about the naming convention of camelCase vs. snake_case.

Code Snippets

int totalRead = 0;
int size;
while( fileSize > total_read && (size = fread(file_out + totalRead, 1, fileSize - totalRead, file) != 0) {
    totalRead+=size;
}
if(totalRead < fileSize){
    //couldn't read entire file
}
file_out[total_read] = '\0';

return file_out;

Context

StackExchange Code Review Q#112613, answer score: 9

Revisions (0)

No revisions yet.