Secure file system utility functions

Question

For Khronos, I've had to develop these utility functions to help me deal with storing the .wav files. However, they could also be used in a variety of applications.

A description of what the three functions do are given in the header documentation comments.

---

util.h:

#ifndef UTIL_H
#define UTIL_H

/**
 * @return the temporary file directory for your system
 */
const char* getTmpDir(void);

/**
 * Generates a unique temporary filename given the fileroot, creates and opens the file
 * @return file descriptor to the open file
 */
int createSafeFileDescriptor(const char* fileRoot);

/**
 * Fetches the file path given the file descriptor
 * @return file path
 */
const char* getPathFromDescriptor(int fd);

#endif // UTIL_H

util.c:

#include <stdlib.h>
#include <stdio.h>
#include <stdint.h>

#ifndef _WIN32
#include <fcntl.h>
#include <unistd.h>
#endif

#include "util.h"

const char* getTmpDir(void)
{
    char *tmpdir = NULL;
    if ((tmpdir = getenv("TEMP"))) return tmpdir;
    else if ((tmpdir = getenv("TMP"))) return tmpdir;
    else if ((tmpdir = getenv("TMPDIR"))) return tmpdir;
    else return "/tmp/";
}

int createSafeFileDescriptor(const char* fileRoot)
{
    // Creates temporary file safely
    char flacFile[FILENAME_MAX] = "";
    snprintf(flacFile, FILENAME_MAX, "%sXXXXXX.wav", fileRoot);

    // the 5 is for the length of the suffix ".wav"
    return mkstemps(flacFile, 4);
}

const char* getPathFromDescriptor(int fd)
{
    char *filename = malloc(FILENAME_MAX);
#ifdef _WIN32
    intptr_t file = _get_osfhandle(fd);
    intptr_t fileMap = CreateFileMapping(file, NULL, PAGE_READONLY, 0, 1, NULL);

    if (hFileMap)
    {
        void* pMem = MapViewOfFile(hFileMap, FILE_MAP_READ, 0, 0, 1);
        if (pMem)
        {
            if (GetMappedFileName(GetCurrentProcess(), pMem, filename, FILENAME_MAX)) return filename;
        }
    }
    return NULL;
#endif

#ifdef __APPLE__
    if (fcntl(fd, F_GETPATH, filename) != -1) return filename;
    return NULL;
#endif

#ifdef __unix__
    char proclnk[PATH_MAX] = {0};
    ssize_t r = -1;
    snprintf(proclnk, sizeof(proclnk), "/proc/self/fd/%d", fd);
    if ((r = readlink(proclnk, filename, FILENAME_MAX)) < 0) return NULL;
    filename[r] = '\0';
    return filename;
#endif
}

Answer 1

Good set of code.

Observations

1. #ifdef structure lacks uniqueness and detection if none are true. Suggest #elif

   #ifdef _WIN32
   #elif __APPLE__
   #elif __unix__
   #else
   #error Error Message
   #endif
   

2. Rather than a differing #if structure in the .c file, suggest a matching one as in #1

   //#ifndef _WIN32
   //#endif
   #ifdef _WIN32
   #elif __APPLE__
   ...
   

3. Memory leak. Should free(filename) before returning NULL.

   if ((r = readlink(proclnk, filename, FILENAME_MAX)) < 0) return NULL;
   

4. Name-space. Prefer some naming convention that ties these 3 functions to their .h file. Maybe wavutil?

   wavutil.h
   wavutil_getTmpDir();
   wavutil_createSafeFileDescriptor()
   wavutil_getPathFromDescriptor();
   

5. Since function allocates data, make that clear in the function declaration. Also document candidate error returns

   * ALLOCATES the file path given the file descriptor
   * Returns NULL on ...
   ...
   const char* getPathFromDescriptor(int fd);
   

6. Suggest error return of NULL instead. Hoping "/tmp/" exists after failing 3 environment variables it a questionable fix. At least "./" is sure to exist. Also the environment variable might not end with /. May need to check that.

   // else return "/tmp/";
   else return NULL;
   // or 
   else return "./";
   

7. snprintf() is good to prevent overruns, yet if it fails, do not proceed with a runt string.

   int len = snprintf(proclnk, sizeof(proclnk), "/proc/self/fd/%d", fd);
   if (len < 0 || len >= sizeof(proclnk)) Handle_Error();
   if ((r = readlink(proclnk, filename, FILENAME_MAX)) < 0) return NULL;
   

Minor

1. Comment inconsistent with code 5 vs. 4? Could avoid magic number 4.

      // // the 5 is for the length of the suffix ".wav"
      // return mkstemps(flacFile, 4);
   
      char ext[] = ".wav";
      snprintf(flacFile, FILENAME_MAX, "%sXXXXXX%s", fileRoot, ext);
      return mkstemps(flacFile, sizeof ext - 1);
   

2. Prefer right-sizing allocations that start off large.

      char *filename = malloc(FILENAME_MAX);
      ...
      char *p = realloc(filename, strlen(filename) + 1);
      return p ? p : filename;
   

3. Curious style change. Why sizeof() in 2nd line, yet FILENAME_MAX instead of sizeof(flacFile). Suggest the latter. Further, not a fan of xxx_MAX when xxx_SIZE is meant. MAX to me means the maximum number so characters - which is 1 less than size.

   char proclnk[PATH_MAX] = {0};
   snprintf(proclnk, sizeof(proclnk), "/proc/self/fd/%d", fd);
   
   char flacFile[FILENAME_MAX] = "";
   snprintf(flacFile, FILENAME_MAX, "%sXXXXXX.wav", fileRoot);