7
\$\begingroup\$

I'm using these 2 functions to handle file input/output and would like to know if there's anything that should be changed.

For content retrieval, enough space is allocated and the file content is copied. To make sure saving a file either saves all or nothing, I'm writing to a buffer then renaming it after all content is successfully written.

I'm particularly interested in knowing whether these functions are safe or not since tmpnam is considered dangerous. I read the main risk is a file with the name returned by tmpnam being created before we are able to do it, so I'm using wbxto make sure it doesn't happen. Is this enough or there are other security concerns that I'm not aware of?

files.h

#ifndef FILES_H
#define FILES_H

#include <stdlib.h>

#define FILE_SUCCESS 0
#define FILE_ERROR 1

void *file_get_content(const char *file_name, size_t *file_size_fill);

//Save all or nothing
int file_save_content(  const char *file_name, 
                        const void *content, 
                        size_t size             );

#endif

files.c

#include <stdio.h>
#include <sys/stat.h>
#include "files.h"

#define BYTE 1
#define READ_MODE "rb"

//Use the new "x" subspecifier so fopen will fail if a file with the name
//returned by tmpnam exists
#define SAFE_WRITE "wbx"


void *file_get_content(const char *file_name, size_t *file_size_fill)
{
    //Get file size, SEEK_END is not portable
    struct stat file_stat;
    if(stat(file_name, &file_stat) != 0)
        return NULL;

    //Check if the file actually contains anything, since malloc can return
    //values other than NULL for malloc(0)
    if(file_stat.st_size == 0)
        return NULL;

    //Space to save the contents
    void *content = malloc(file_stat.st_size);
    if(content == NULL)
        return NULL;

    //Open file
    FILE *file = fopen(file_name, READ_MODE);
    if(file == NULL){
        free(content);
        return NULL;
    }

    //Read it all
    if(fread(content, BYTE, file_stat.st_size, file) != file_stat.st_size){
        free(content);
        fclose(file);
        return NULL;
    }

    fclose(file);

    *file_size_fill = file_stat.st_size;
    return content;
}

//Save the file content safely, it either saves all or nothing
int file_save_content(  const char *file_name,
                        const void *content,
                        size_t size             )
{
    //Get a name for the temp file
    char temp_file_name[L_tmpnam];
    if(tmpnam(temp_file_name) == NULL)
        return FILE_ERROR;

    //Create it
    FILE *temp_file = fopen(temp_file_name, SAFE_WRITE);
    if(temp_file == NULL)
        return FILE_ERROR;

    //Write
    if(fwrite(content, BYTE, size, temp_file) != size){
        fclose(temp_file);
        remove(temp_file_name);
        return FILE_ERROR;
    }

    //Replace old file
    fclose(temp_file);
    if(rename(temp_file_name, file_name) != 0){
        remove(temp_file_name);
        return FILE_ERROR;
    }

    return FILE_SUCCESS;
}

file_save_content() after following the suggestions given by 200_success

int file_save_content(const char *file_name, const void *content, size_t size)
{
    /* Create temp file */
    char tmp[] = {'t','m','p','X','X','X','X','X','X', '\0'};
    int error;
    int fd = mkstemp(tmp);

    if(fd == -1)
        return FILE_ERROR;

    if(write(fd, content, size) != (ssize_t)size){
        error = errno;
        close(fd);
        errno = error;
        return FILE_ERROR;
    }

    /* Replace old file */
    if(rename(tmp, file_name) != 0){
        error = errno;
        unlink(tmp);
        close(fd);
        errno = error;
        return FILE_ERROR;
    }

    close(fd);

    return FILE_SUCCESS;
}
\$\endgroup\$
5
\$\begingroup\$

I think that a more idiomatic C interface would be

ssize_t file_get_content(const char *file_name, char *content[]);

Returning the number of bytes actually read would be analogous to read(2) and printf(3). On error, the return value could be -1, and the reason would be in errno. For consistency, I would have file_save_content() return the number of bytes written, or -1 on error.

It would be convenient if you allocated an extra byte and appended a \0 terminator to the result. Then the caller has the option to treat the buffer as a null-terminated string, if it believes the file to contain text. It would still be able to treat it as binary data (possibly containing NUL bytes) based on the return value.

When getting the contents of an empty file, I would expect it to produce an "empty" buffer, not a NULL pointer.

There is a possibility of a race condition: the file could be truncated between the stat() and the fread(). You could actually handle that case a bit more gracefully.

int file_get_content(const char *file_name, char *buf[]) {
    struct stat file_stat;
    if (0 != stat(file_name, &file_stat)) {
        return -1;
    }

    if (NULL == (*buf = 1 + malloc(file_stat.st_size))) {
        return -1;
    }

    FILE *file;
    if (NULL == (file = fopen(file_name, "rb"))) {
        free(*buf);
        return -1;
    }

    ssize_t bytes_read = fread(*buf, 1, file_stat.st_size, file);
    (*buf)[bytes_read] = '\0';
    fclose(file);
    return bytes_read;
}

In file_save_content(), avoid tmpnam(), which allows a race condition between the time you call tmpnam() and fopen(). Also, rename() would fail unless the temporary file and the destination are on the same filesystem. As an alternative, I would recommend mkstemp() with the destination filename plus a ".XXXXXX" suffix, followed by fdopen().

As a nitpick, I'd prefer unlink() over remove(), since the former refuses to remove directories.

In the error handlers of file_save_content(), save and restore errno, because it would be more useful for the caller to see the original reason for the failure rather than a problem with the subsequent cleanup.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1, nice answer. Note that (*buf)[1 + bytes_read] = '\0'; should drop the 1 \$\endgroup\$ – William Morris Jan 24 '14 at 2:04
  • \$\begingroup\$ @WilliamMorris Thanks! Fixed in Rev 2. \$\endgroup\$ – 200_success Jan 24 '14 at 2:21
  • \$\begingroup\$ Thank you for reviewing the code. Do you mind explaining how could a race condition happen when calling fopen with the x subspecifier since it will only be opened if the file doesn't exist? \$\endgroup\$ – 2013Asker Jan 24 '14 at 16:17
  • \$\begingroup\$ There is a slim chance that a colliding file could be created between tmpnam() and fopen(). Fortunately, there is no security hole, since you call fopen() with the "x" flag, but an attacker might be able to cause grief by triggering failures. It's a farfetched scenario, but an avoidable one. \$\endgroup\$ – 200_success Jan 24 '14 at 22:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.