Two implementations of file-reading

Question

I work quite a lot with file reading and writing in C/Win32 and I started wondering if my code is completely ugly and how I can improve it. I wrote two simple examples of reading a file, and I wonder how these functions could be improved. Are these methods bad - and how they can be improved? I use MSVC so the code might contain MS-specific macros or keywords.

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    DWORD check = 0;
    BOOL success = FALSE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile != INVALID_HANDLE_VALUE)
    {
        if((*outsize = GetFileSize(hFile, NULL)) != INVALID_FILE_SIZE)
        {
            if((*outdata = malloc(*outsize)) != NULL)
            {
                if(ReadFile(hFile, *outdata, *outsize, &check, NULL) && check == *outsize)
                {
                    success = TRUE;
                }
            }
        }
    }

    if(hFile != INVALID_HANDLE_VALUE)
    {
        CloseHandle(hFile);
    }

    return success;
}

BOOL read_file2(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    DWORD check = 0;
    BOOL success = TRUE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile == INVALID_HANDLE_VALUE)
    {
        success = FALSE; goto _end;
    }

    if((*outsize = GetFileSize(hFile, NULL)) == INVALID_FILE_SIZE)
    {
        success = FALSE; goto _end;
    }

    if((*outdata = malloc(*outsize)) == NULL)
    {
        success = FALSE; goto _end;
    }

    if(!ReadFile(hFile, *outdata, *outsize, &check, NULL) || check != *outsize)
    {
        success = FALSE; goto _end;
    }

_end:

    if(hFile != INVALID_HANDLE_VALUE)
    {
        CloseHandle(hFile);
    }

    return success;
}

Questions:

1. Which one is better, and why?
2. What is bad about the above examples?
3. Can you show a better example or how you would do it?

Answer 1

While there's nothing inherently wrong with either approach, your code does not follow usual Windows conventions for C APIs. Windows APIs will almost never allocate a buffer that the caller must free; those that do will almost always do so with a custom cleanup function. Secondly, by using ANSI characters and APIs instead of Wide characters and APIs, you limit the sets of files you can work with.

Now there are certainly tradeoffs in the question of whether caller or callee allocates the buffer. When the callee allocates it, there's little chance of buffer overruns. However there's an increased chance of either a memory leak or attempting to free the memory with the wrong function, although static analysis can help with either of these concerns. This concerns are especially relevant at library interfaces.

There's even technically a tradeoff on the ANSI vs Wide character question, but unless you're trying to support Windows 9x systems (now all firmly outdated), the fact that the Wide APIs do not work on those systems should no longer be relevant. If you don't know whether you need to support such cases, it's possible to use 'T' character typedefs and API macros that are dependent on the definition of UNICODE.

Finally there's a small concern that you're excluding possible return values from GetFileSize. This is probably okay, because if a file exceeds 4GB, you are less likely to be able to allocate the buffer and read it into memory anyway. But it's worth commenting that this is intentional so that the function can document its limits.

To sum up, I would suggest changing most of the above to improve your code. Add comments. Use the Unicode types and APIs. Especially if you are writing a library, have the caller allocate the buffer. Consider supporting static analysis (_In_, _Out_, etc.). And consider checking the high half of GetFileSize's result:

BOOL ReadFile_3(_In_ LPCWSTR szFilename, _Out_ LPBYTE lpBuffer, _In_ cbBuffer)
{
    : : :
    HANDLE hFile = CreateFileW(szFileName, ...);
    : : :
    LARGE_INTEGER liFileSize;
    liFileSize.LowPart = GetFileSize(hFile, &liFileSize.HighPart);
    if (liFileSize.HighPart > 0 || liFileSize.LowPart > cbBuffer)
        success = FALSE;
    : : :
}

Or, if your code must support Windows 9x, but you can compile it twice to also better support Windows NT:

BOOL ReadFile_4(_In_ LPCTSTR szFileName, ...) // LPCSTR or LPCWSTR depending on UNICODE
{
    : : :
    HANDLE hFile = CreateFile(szFileName, ...); // CreateFileA or CreateFileW similarly
    : : :
}

Answer 2

You could improve your first example by returning early and moving variable definitions nearer to first use. I prefer this to the use of gotos but maybe I've just been conditioned against their use for too long:

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    HANDLE h = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
    if(h == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    BOOL success = FALSE;
    if((*outsize = GetFileSize(h, NULL)) != INVALID_FILE_SIZE)
    {
        if((*outdata = malloc(*outsize)) != NULL)
        {
            DWORD check = 0;
            if(ReadFile(h, *outdata, *outsize, &check, NULL) && check == *outsize)
            {
                success = TRUE;
            }
            else { free(*outdata); }
        }
    }
    CloseHandle(h);
    return success;
}

Another approach is to split into two functions, thereby removing the need for nesting and gotos. The compiler can optimize away the function call (eg by inlining) if read_file_data is made static, but this inner function might possibly be useful in itself:

static BOOL read_file_data(HANDLE h, DWORD size, PBYTE *outdata)
{
    *outdata = malloc(size);
    if (*outdata == NULL) {
        return FALSE;
    }
    DWORD size_read = 0;
    if (ReadFile(h, *outdata, size, &size_read, NULL) && (size_read == size)) {
        return TRUE;
    }
    free(*outdata);
    return FALSE;
}

BOOL read_file(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    HANDLE h = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
    if (h == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    BOOL success = FALSE;
    *outsize = GetFileSize(h, NULL);
    if (size != INVALID_FILE_SIZE) {
        success = read_file_data(h, *outsize, outdata);
    }
    CloseHandle(hFile);
    return success;
}

You can also improve the goto method a little by returning early and changing the default success to FALSE (and freeing the buffer on error).

BOOL read_file2(PCHAR filename, PBYTE *outdata, DWORD *outsize)
{
    BOOL success = FALSE;
    HANDLE hFile = CreateFileA(filename, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if(hFile == INVALID_HANDLE_VALUE) {
        return FALSE;
    }
    if((*outsize = GetFileSize(hFile, NULL)) == INVALID_FILE_SIZE) {
        goto _end;
    }
    if((*outdata = malloc(*outsize)) == NULL) {
        goto _end;
    }
    DWORD check = 0;
    if(!ReadFile(hFile, *outdata, *outsize, &check, NULL) || (check != *outsize)) {
        free(*outdata);
    }
    else success = TRUE;

_end:
    CloseHandle(hFile);
    return success;
}

Answer 3

• You (intentionally?) don't deallocate the value from malloc in your error path.
• I'm always for read_file2. You have multiple points of failure, so you can properly organise the error handling in the same fashion, step by step. Oh yeah, and less nesting is a good thing IMO. Normally I'd expect multiple labels though, since in the failure case you need to reverse all the steps which happened before.

Why switch the return code handling in read_file2? I'd say

BOOL success = FALSE;
...
if (error) goto _end;
...
success = TRUE;
...
_end: ...

removes potential coding errors.

In general, why not take a look at some C code, or e.g. the Linux kernel coding style, the problem isn't new, although you can find both styles.

Edit: For multiple labels, I mean something like:

if (allocation1 fails)
  goto deallocate1;
if (allocation2 fails)
  goto deallocate2;
return result;
deallocate2: ...
deallocate1: ...
error:
  return error_code;

So you do your steps one by one, then, if any of those fail, you jump into the first label and undo everything in the reverse order.