Standard way of reading file contents to a buffer

Question

I have been trying to write a sample code block to read file contents into a buffer.

I am assuming that:

1. Code must be platform independent
2. Should work in all cases(or report proper error)
3. It should be readable and maintainable
4. It shouldn't be over-complex and shouldn't have errors

Do you think the return of fseek should be tested as well?

void
die( int error_code, char* message )
{
    fprintf( stderr, message );
    exit( error_code );
}

char *
read_certificate_file ( char *file_location ) 
{
    FILE *file_descriptor = NULL; 
    size_t file_size = 0;
    size_t bytes_read = 0;
    char *certificate_content = NULL;
    file_descriptor = fopen ( file_location, "r" );
    if ( file_descriptor == NULL ) {
        perror ( "Opening certificate file" );
        die ( EXIT_FAILURE, "Error opening certificate file." );
    }
    fseek (file_descriptor, 0, SEEK_END);
    file_size = ftell ( file_descriptor );
    if ( file_size == 0 ) {
        fclose ( file_descriptor );
        die ( EXIT_FAILURE, "Certificate file is empty." );
    }       
    certificate_content = (char *) malloc ( file_size+1 );
    if ( certificate_content == NULL ) {
        fclose( file_descriptor );
        die( EXIT_FAILURE, "Error allocating memory for " \
                          " certificate." );
    }
    certificate_content[ file_size ] = '\0';
    fseek ( file_descriptor, 0, SEEK_SET ); 
    bytes_read = fread ( certificate_content, 1, file_size, 
                         file_descriptor );
    if ( bytes_read != file_size ) {
            perror ( "Reading Certificate file" );
            fclose( file_descriptor );
            die ( EXIT_FAILURE, "Error reading "
                                "certificate file." );
    }
    fclose ( file_descriptor );
    return certificate_content;
}


int main()
{
    printf( "%s", read_certificate_file ( "./certificate.pem" ));
    return EXIT_SUCCESS;
}

Answer 1

A few things stand out...

Function declarations

I'm not a huge fan of specify return type on previous line style:

void
die( int error_code, char* message )

However, if you're going to use it, be consistent (your main is defined differently).

should work in all cases

You're assuming that the file you're reading does't contain any nulls. This may be OK, as you're not opening the file in binary mode, however if you decide to reuse the code for binary files, returning a null terminated buffer without a size field is going to cause issues.

Main is leaking

I know your main is just a test harness to demonstrate the code, however it's best to always use appropriate memory management. Your function is allocating a buffer using malloc, you should be freeing it in your main. By including this in your example it demonstrates that you recognise responsibility for the returned buffer is sitting with the caller.

Answer 2

Separate means from policies

A decision to exit is very important, and it shall not be delegated to an utility function. Consider returning an error code instead.

Printing error messages from inside an utility function is less critical, but is also dubious. For example, the function cannot be used in a GUI application.

Answer 3

Your code allocates memory with malloc(), which the user is required release with free(). This prevents you using a different allocator in future, or for specific platforms (e.g. you might use mmap() on platforms that have it). You should either

• provide a matching function to release the memory (currently a minimal wrapper of free()), or
• allow the caller to provide their own allocator (and they can use the corresponding release)

so that you have an interface that's not tied to malloc/free forever.

---

Your function doesn't return the number of characters read. It might not matter for you, if your files are known not to contain NUL characters, but for a general file reader you can't depend on that. The fact that you open it text mode suggests you're expecting printable characters, but I can't tell that from the name or comments.

---

You open the file in text mode, but you attempt to use ftell() to measure the size. But the meaning of this result is not specified for a text stream, only that it can be used as input to fseek() to return to the same place. On systems where text and binary files have different representations, this could cause your code to fail.

If you were to open the file in binary mode (i.e. with "rb"), you would have a different problem; a binary stream is not required to support SEEK_END. So you're between a rock and a hard place here - in fact, CERT FIO19-C says

Do not use fseek() and ftell() to compute the size of a regular file

(And you didn't even check the result of fseek in case it's not a regular file...)

---

Minor niggles:

• Don't cast the return value from malloc(). Include <stdlib.h> instead.
• if (foo == NULL) is more idiomatically written if (!foo). Certainly subjective, but I find the latter easier to read.

---

Other issues as addressed in different answers; I've upvoted rather than re-writing them here.

Answer 4

Welcome to code review, and nice first question.

The code is readable at first glance.

It helps us review the code when the full is the full program, the includes are missing.

Reduce Function Complexity
The code would be much more readable and maintainable if the function read_certificate_file() was broken up into additional functions. Each of the variable assignments in this function should be a sub-function. This is known as the Single Responsibility Principle (SRP).

    file_descriptor = open_input_file(char *file_location);
    file_size = get_file_size(file_descriptor);
    certificate_content = fill_buffer(file_descriptor, file_size);

Check ALL Possible Errors
The function read_certificate_file() should make sure char *file_location is not empty before using it. Never depend on input being the proper value.

This function May Not Work for Binary Files As forsvarir points out fseek() and ftell() may not give the results expected in all cases. See this reference.

Answer 5

1. Bug: The end offset of a text file does not necessarily equate to the number of characters that will be read. 2 notable exceptions: CR/LF pairs are often changed to a single LF. Many text files, even "8-bit" ones, may begin with a Byte-order-Mark (BOM), which takes up space in a file, yet is not returned as part of a fread(). So either read the file as a binary or check for file error instead and use buf[bytes_read] = '\0';

   // if ( bytes_read != file_size ) {
   if (ferror(file_descriptor)) {
   

2. Architecture design weakness. Reading is a whole file into memory at once is rarely a robust solution. Rarely do programs need the entire contents before processing. Also large files may not fit in memory. Knowing the higher code level goal would help in suggesting alternatives.

3. "to read file contents into a buffer." is the goal, then why all the "certificate" naming? This function certainly have many applications and should not be written with only a single use case in mind.

4. The following code test is short-sighted. 0-length files are common. IAC, better to pass back the file length information to the caller

   // 0-length file test not needed _here_.
   if ( file_size == 0 ) {
     fclose ( file_descriptor );
     die ( EXIT_FAILURE, "Certificate file is empty." );
   

5. Mixing integer types without regard to range and error value checking. ftell() returns type long which differs from type size_t.

   // weak code
   size_t file_size = 0;
   ...
   file_size = ftell(file_descriptor);
   
   // better
   read_certificate_file (const char *file_location, size_t *file_size ) {
     ...
     long tell_result = ftell(file_descriptor);
     if (tell_result == -1L) {
       Handle_Error(errno); // Use implementation-defined positive value in errno.
     }
     if (tell_result < 0 || tell_result + ((size_t) 0) > SIZE_MAX - 1) {
       Handle_RangeError();
     }
     *file_size = (size_t) tell_result;
   

6. For all IO functions, best to check return value.

   // fseek (file_descriptor, 0, SEEK_SET);     
   if (fseek (file_descriptor, 0, SEEK_SET)) {
     Handle_Error();
   }
   

7. Use const for pointers to data that are not modified. Function then has wider application and greater optimization potentials.

   // char *read_certificate_file ( char *file_location ) 
   char *read_certificate_file (const char *file_location ) 
   

8. Memory leak with if ( bytes_read != file_size ) path. certificate_content is not free'd.

9. Simplify. Remove unneeded cast.

   // certificate_content = (char *) malloc ( file_size+1 );
   certificate_content = malloc ( file_size+1 );
   

Sample code (minus error messages)

char *read_file(FILE *file, size_t *length) {
  if (fseek(file, 0, SEEK_END)) {
    return NULL; // fseek error
  }
  long tell = ftell(file);
  if (tell < 0 || tell + ((size_t) 0) > SIZE_MAX - 1) {
    if (tell == -1L) return NULL;  // ftell error
    return NULL; // range error
  }
  size_t file_length = (size_t) tell;
  if (fseek(file, 0, SEEK_SET)) {
    return NULL; // fseek error
  }
  char *buf = malloc(file_length + 1);
  if (buf == NULL) {
    return NULL; // alloc error
  }
  size_t bytes_read = fread(buf, 1, file_length, file);
  if (ferror(file)) {
    free(buf);
    return NULL; // fread error
  }
  buf[bytes_read] = '\0';
  if (length) *length = bytes_read;
  return buf;
}

Detail on if (tell < 0 || tell + ((size_t) 0) > SIZE_MAX - 1) {

If tell < 0 code has a problem - need to return NULL. Otherwise, code needs to test if tell, which is long can fit in a size_t (some unsigned type.) It is not known which type has a wider positive range. Using tell + ((size_t) 0) converts the value to the wider of the two types. Should not use tell + ((size_t) 1) > SIZE_MAX as tell + ((size_t) 1) may overflow. Use SIZE_MAX - 1 rather than SIZE_MAX to allow for the +1 later in code.

Answer 6

The most obvious problem I see with your code is that it will fail for non-seekable files (e.g. pipes, sockets, stdin, etc.). Also, as pacmaninbw notes in their answer, using fseek() / ftell() to determine the size of a file has a number of other pitfalls.

The simple and generic solution is to not try to determine the size of the buffer in advance at all, but simply to expand the buffer as necessary:

1. malloc() a reasonable-sized buffer (e.g. 1 kb to 1 Mb, depending on how large files you expect to be reading).
2. fread() as many bytes as will fit into the buffer.
3. If the buffer got full, double its size with realloc() and read more bytes into the newly allocated area. Repeat until fread() returns less than the requested size.
4. At this point, you've either reached the end if the file, or there was an error. Use ferror() to check which.
5. Finally, if there was no error, truncate the buffer with realloc() to the actual size of the file (plus one byte, if you want to append a null terminator).

This approach can handle any kind of input stream — even e.g. stdin connected to the user's terminal — without having to determine its length in advance. While it's possible for the realloc()s to require some unnecessary data copying, this should not happen often unless your heap is badly fragmented. In any case, the cost of such copying should be negligible compared to that of disk access.

Also, given that files can contain null bytes (even if you don't expect them to), a more robust interface would have your function return the actual number of bytes read to the caller. There are several ways to do this, but one option would be to let the caller pass in an optional pointer to a variable in which to store the length, like this:

#include <stdlib.h>
#include <stdio.h>
/*
 * Reads the entire contents of a file or a stream into memory.
 * Returns a pointer to a buffer containing the file contents, or NULL if there was an I/O error or insufficient memory. (The ferror() function can be used to determine which of these was the case.)
 * The caller is responsible for free()ing the buffer after use!
 * If the optional lenptr argument is not NULL, the actual number of bytes read into the buffer will be stored at the address it points to.
 * The returned buffer will always be padded with a null byte; however, since the file being read may itself contain null bytes, calling strlen() on the buffer may return a value less than the actual length of the file (as returned via the lenptr argument).
 */
char *slurp (FILE *fh, size_t *lenptr) {
    size_t flen = 0, buflen = (1 << 12); /* actual initial size is twice this, i.e. 8 kb */
    char *buf = NULL, *newbuf;
    do {
        buflen *= 2;
        if (buflen <= flen) break; /* overflow */
        newbuf = realloc(buf, buflen);
        if (newbuf == NULL) {
            free(buf);
            return NULL;
        }
        buf = newbuf;
        flen += fread(buf + flen, buflen - flen, 1, fh);
    } while (flen == buflen);

    if (buflen <= flen || ferror(fh)) {
        free(buf);
        return NULL;
    }

    newbuf = realloc(buf, flen + 1);
    if (newbuf == NULL) {
        /* this should never happen, but better safe than sorry */
        free(buf);
        return NULL;
    }
    buf = newbuf;

    buf[flen] = '\0';
    if (lenptr != NULL) *lenptr = flen;
    return buf;
}

(Note: The code above is completely untested. I believe it should work, unless I made some silly typo, but use it at your own risk!)