CODEWITHSUNDEEP
cwinapi
Johannes Meyer
Johannes Meyer

Reputation: 44

GetFileAttributesW causes Access violation reading location x (GetLastError returns 8)

I am currently trying to implement a program that loads multiple files into memory, for further processing. To see if the file exists I made a function that uses GetFileAttributesW to check if the file indeed exists.

The first file gets loaded correctly, but once I try to load the second file, I get the access violation (GetLastError returns 8; ERROR_NOT_ENOUGH_MEMORY). I can safely rule out that I don't have enough RAM, as the files are max. 500kb in size and I am never loading more than 20 (I have 16GB of RAM). I also have rights to access he file, etc.

inline BOOL FileExists(const TCHAR* szPath)
{
    DWORD dwAttrib = GetFileAttributesW(szPath); // ERROR here (1st iteration everything is fine) 

    return (dwAttrib != INVALID_FILE_ATTRIBUTES && !(dwAttrib & FILE_ATTRIBUTE_DIRECTORY));
}

.
.
.

FILE_DATA LoadFileIntoMemory(const TCHAR* FileName)
{
    PTCHAR FinalPath = VirtualAlloc(NULL, MAX_PATH, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);

    PTCHAR FilePath = L"C:\\Users\\invasi0nZ\\"; // 

    concat(FinalPath, FilePath, MAX_PATH);
    concat(FinalPath, FileName, MAX_PATH);

    if (!FileExists(FinalPath))
    {
        memset(FinalPath, 0, MAX_PATH);

        FilePath = L"C:\\Users\\invasi0nZ\\Documents\\";

        concat(FinalPath, FilePath, MAX_PATH);
        concat(FinalPath, FileName, MAX_PATH);
    }

    HANDLE File = CreateFileW(FinalPath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

    if (File == INVALID_HANDLE_VALUE)
    {
        return (FILE_DATA){ NULL, NULL };
    }

    int FileSize = GetFileSize(File, NULL);

    PBYTE RawFile = VirtualAlloc(NULL, FileSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);

    DWORD dwRead;
    size_t FileSize = GetFileSize(File, NULL);

    ReadFile(File, RawFile, FileSize, &dwRead, NULL);

    CloseHandle(File);
    VirtualFree(FinalPath, sizeof(FinalPath), MEM_FREE);

    return (FILE_DATA) { RawFile, FileSize };
}

.
.
.

void LoadAllFiles(Array FileNames)
{
    for (int i = 0; i < FileNames.used; i++)
    {
        FILE_DATA file_data = LoadFileIntoMemory(FileNames.array[i].file_name);
        // Store file_data, etc.
    }
    // Do stuff with files here
}

As far as I see I am closing all required handles and freeing everything that I can without breaking the program.

Thank you very much in advance!

Upvotes: 0

Views: 169

Answers (1)

Remy Lebeau
Remy Lebeau

Reputation: 597036

You are not allocating enough memory with VirtualAlloc(), thus your concat() function can cause a buffer overflow. Since you are compiling with UNICODE enabled, TCHAR is wchar_t, which is 2 bytes in size. You need to take that size into account when allocating memory for FinalPath, as well as when clearing FinalPath with memset().

Change this:

PTCHAR FinalPath = VirtualAlloc(NULL, MAX_PATH, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
...
memset(FinalPath, 0, MAX_PATH);

To this:

PTCHAR FinalPath = VirtualAlloc(NULL, sizeof(TCHAR) * MAX_PATH, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
...
memset(FinalPath, 0, sizeof(TCHAR) * MAX_PATH);

There are other problems with your code, too. You are not checking if VirtualAlloc() is successful before using the returned pointer. You are leaking the allocated memory if CreateFileW() fails. And you are declaring FileSize twice.

Try this instead:

FILE_DATA LoadFileIntoMemory(const TCHAR* FileName)
{
    PTCHAR FinalPath = VirtualAlloc(NULL, sizeof(TCHAR) * MAX_PATH, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
    if (!FinalPath)
        return (FILE_DATA){ NULL, 0 };

    PTCHAR FilePath = TEXT("C:\\Users\\invasi0nZ\\"); // 

    concat(FinalPath, FilePath, MAX_PATH);
    concat(FinalPath, FileName, MAX_PATH);

    if (!FileExists(FinalPath))
    {
        memset(FinalPath, 0, sizeof(TCHAR) * MAX_PATH);

        FilePath = TEXT("C:\\Users\\invasi0nZ\\Documents\\");

        concat(FinalPath, FilePath, MAX_PATH);
        concat(FinalPath, FileName, MAX_PATH);
    }

    HANDLE File = CreateFile(FinalPath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
    if (File == INVALID_HANDLE_VALUE)
    {
        VirtualFree(FinalPath, 0, MEM_RELEASE);
        return (FILE_DATA){ NULL, 0 };
    }

    VirtualFree(FinalPath, 0, MEM_RELEASE);

    DWORD FileSize = GetFileSize(File, NULL);
    if (FileSize == INVALID_FILE_SIZE)
    {
        CloseHandle(File);
        return (FILE_DATA){ NULL, 0 };
    }

    PBYTE RawFile = VirtualAlloc(NULL, FileSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
    if (!RawFile)
    {
        CloseHandle(File);
        return (FILE_DATA){ NULL, 0 };
    }

    DWORD dwRead;
    if (!ReadFile(File, RawFile, FileSize, &dwRead, NULL))
    {
        VirtualFree(RawFile, 0, MEM_RELEASE);
        RawFile = NULL;
        dwRead = 0;
    }

    CloseHandle(File);

    return (FILE_DATA) { RawFile, dwRead };
}

That being said, you don't really need FileExists() at all, as CreateFile() can tell you whether the file exists or not, thus avoiding a race condition in your code if another process creates/deletes the file after you check for its existence but before you open it.

You should also get rid of VirtualAlloc() for FinalPath, you don't need to allocate that string dynamically. Just declare the array statically instead.

And the Win32 API has functions for concatenating path segments together, so you don't need to write your own. It also has functions for querying the paths of the user's profile and Documents folders, you should not hard-code those paths.

Try something more like this instead:

#include <windows.h>
#include <pathcch.h>
#include <shlobj.h>

HANDLE OpenFileInFolder(CSIDL FolderID, const TCHAR *FileName)
{
    TCHAR FilePath[MAX_PATH];

    HRESULT Res = SHGetFolderPath(NULL, FolderID, NULL, SHGFP_TYPE_CURRENT, FilePath);
    if (Res != S_OK)
    {
        SetLastError(Res);
        return INVALID_HANDLE_VALUE;
    }

    Res = PathCchCombine(FilePath, MAX_PATH, FilePath, FileName);
    if (Res != S_OK)
    {
        SetLastError(Res);
        return INVALID_HANDLE_VALUE;
    }

    return CreateFile(FilePath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
}

FILE_DATA LoadFileIntoMemory(const TCHAR* FileName)
{
    HANDLE File = OpenFileInFolder(CSIDL_PROFILE, FileName);
    if ((File == INVALID_HANDLE_VALUE) && (GetLastError() == ERROR_FILE_NOT_FOUND))
        File = OpenFileInFolder(CSIDL_MYDOCUMENTS, FileName);

    if (File == INVALID_HANDLE_VALUE)
        return (FILE_DATA){ NULL, 0 };

    DWORD FileSize = GetFileSize(File, NULL);
    if (FileSize == INVALID_FILE_SIZE)
    {
        CloseHandle(File);
        return (FILE_DATA){ NULL, 0 };
    }

    PBYTE RawFile = VirtualAlloc(NULL, FileSize, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
    if (!RawFile)
    {
        CloseHandle(File);
        return (FILE_DATA){ NULL, 0 };
    }

    DWORD dwRead;
    if (!ReadFile(File, RawFile, FileSize, &dwRead, NULL))
    {
        VirtualFree(RawFile, 0, MEM_RELEASE);
        RawFile = NULL;
        dwRead = 0;
    }

    CloseHandle(File);

    return (FILE_DATA) { RawFile, dwRead };
}

Upvotes: 1

Related Questions