Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common/FileUtil: Fix incorrect (32-bit) stat struct being used on Windows, which was hidden by a define in CommonFuncs.h. #10183

Merged
merged 1 commit into from Oct 23, 2021

Conversation

AdmiralCurtiss
Copy link
Contributor

tldr: https://godbolt.org/z/9YeK6fToa

This is a really silly name clash between the stat struct, the _stat64 struct, and a #define that tries to make the stat function work on Windows by mapping the _stat64 function to it.

Over in FileUtil.h we have a member of FileInfo that is defined as follows:

struct stat m_stat;

And over in CommonFuncs.h we have this define:

#define stat _stat64

Additionally, sys/stat.h on Windows defines the following structs:

struct _stat64
{
    _dev_t         st_dev;
    _ino_t         st_ino;
    unsigned short st_mode;
    short          st_nlink;
    short          st_uid;
    short          st_gid;
    _dev_t         st_rdev;
    __int64        st_size;
    __time64_t     st_atime;
    __time64_t     st_mtime;
    __time64_t     st_ctime;
};

#define __stat64 _stat64 // For legacy compatibility

#if defined(_CRT_INTERNAL_NONSTDC_NAMES) && _CRT_INTERNAL_NONSTDC_NAMES && !defined _CRT_NO_TIME_T
    struct stat
    {
        _dev_t         st_dev;
        _ino_t         st_ino;
        unsigned short st_mode;
        short          st_nlink;
        short          st_uid;
        short          st_gid;
        _dev_t         st_rdev;
        _off_t         st_size;
        time_t         st_atime;
        time_t         st_mtime;
        time_t         st_ctime;
    };
#endif

This is a problem. If you include FileUtil.h without including CommonFuncs.h, you get the second struct but if you include CommonFuncs.h you instead get the first struct. FileUtil.cpp does the latter, so it gets the _stat64 struct. This means that different compilation units may have a different idea about the length of File::FileInfo, leading to potential stack corruption.

Note that I'm using __stat64 here instead of _stat64 because that's how the _wstat64 function (which we use) is defined in the MSVC docs: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/stat-functions?view=msvc-160

...I love code that works by accident.

…dows, which was hidden by a define in CommonFuncs.h.
@lioncash
Copy link
Member

Oof this one's a doozy. Nice catch!

@lioncash lioncash merged commit 52823c6 into dolphin-emu:master Oct 23, 2021
9 of 10 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the stat64 branch October 23, 2021 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants