From 1e0c75c3bd8a7b5c25189410f7ecbde643bb3873 Mon Sep 17 00:00:00 2001 From: ergo720 <45463469+ergo720@users.noreply.github.com> Date: Sun, 4 Apr 2021 17:55:02 +0200 Subject: [PATCH 1/5] Detect mismatch between cxbxr modules --- projects/cxbxr-ldr/CMakeLists.txt | 2 ++ src/CxbxVersion.h | 9 +++++++++ src/common/win32/EmuShared.cpp | 1 + src/common/win32/EmuShared.h | 20 +++++++++++++++++--- src/core/kernel/init/CxbxKrnl.cpp | 11 +++++++++++ src/core/kernel/init/CxbxKrnl.h | 3 --- src/emulator/cxbxr-emu.cpp | 9 +++++++++ src/loader/cxbxr-ldr.cpp | 9 +++++++++ 8 files changed, 58 insertions(+), 6 deletions(-) diff --git a/projects/cxbxr-ldr/CMakeLists.txt b/projects/cxbxr-ldr/CMakeLists.txt index 4bfc47151..1e629bea8 100644 --- a/projects/cxbxr-ldr/CMakeLists.txt +++ b/projects/cxbxr-ldr/CMakeLists.txt @@ -25,6 +25,8 @@ endif() file (GLOB HEADERS "${CXBXR_ROOT_DIR}/src/common/AddressRanges.h" "${CXBXR_ROOT_DIR}/src/common/ReserveAddressRanges.h" + "${CXBXR_ROOT_DIR}/src/CxbxVersion.h" + "${CXBXR_ROOT_DIR}/src/version.h" ) file (GLOB SOURCES diff --git a/src/CxbxVersion.h b/src/CxbxVersion.h index 17983832b..e4f1e1969 100644 --- a/src/CxbxVersion.h +++ b/src/CxbxVersion.h @@ -1,5 +1,14 @@ #pragma once #include "version.h" +#include + extern const char* CxbxVersionStr; + +// Note: GitVersionMaxLength should be large enough to accomodate the longest git version string we can practically expect to have. This is necessary +// to avoid possible mismatches in the string length which can happen if the user mixes different cxbxr versions +inline constexpr const char *CxbxGitVersion = _GIT_VERSION; +inline constexpr size_t GitVersionLength = std::char_traits::length(CxbxGitVersion); +inline constexpr size_t GitVersionMaxLength = 80; +static_assert(GitVersionLength < GitVersionMaxLength); diff --git a/src/common/win32/EmuShared.cpp b/src/common/win32/EmuShared.cpp index e3ecea7d8..1d8f88367 100644 --- a/src/common/win32/EmuShared.cpp +++ b/src/common/win32/EmuShared.cpp @@ -168,6 +168,7 @@ EmuShared::EmuShared() for (auto& i : m_DeviceType) { i = to_underlying(XBOX_INPUT_DEVICE::DEVICE_INVALID); } + std::strncpy(m_git_version, CxbxGitVersion, GitVersionLength); } // ****************************************************************** diff --git a/src/common/win32/EmuShared.h b/src/common/win32/EmuShared.h index 4fcd35b70..93da9711b 100644 --- a/src/common/win32/EmuShared.h +++ b/src/common/win32/EmuShared.h @@ -30,8 +30,8 @@ #include "Mutex.h" #include "common\IPCHybrid.hpp" #include "common\input\Button.h" +#include "CxbxVersion.h" #include "core/common/imgui/settings.h" - #include extern HMODULE hActiveModule; // Equals EXE Module handle in (GUI) Cxbx.exe / cxbxr.exe, equals DLL Module handle in cxbxr-emu.dll @@ -66,6 +66,11 @@ class EmuShared : public Mutex // ****************************************************************** unsigned int m_size; + // ****************************************************************** + // * Git version string of cxbx.exe + // ****************************************************************** + char m_git_version[GitVersionMaxLength]; + // ****************************************************************** // * Each process needs to call this to initialize shared memory // ****************************************************************** @@ -286,6 +291,16 @@ class EmuShared : public Mutex void GetOverlaySettings(overlay_settings *value) { Lock(); *value = m_imgui_overlay_settings; Unlock(); } void SetOverlaySettings(const overlay_settings* value) { Lock(); m_imgui_overlay_settings = *value; Unlock(); } + // ****************************************************************** + // * Git version Accessor (only the get method is provided because it should not be changed + // ****************************************************************** + void GetGitVersion(char *value) + { + Lock(); + std::strncpy(value, m_git_version, GitVersionLength + 1); + Unlock(); + } + // ****************************************************************** // * Reset specific variables to default for kernel mode. // ****************************************************************** @@ -329,9 +344,8 @@ class EmuShared : public Mutex bool m_bEmulating_status; #ifndef CXBX_LOADER // Temporary usage for cxbx.exe's emu unsigned int m_PreviousMmLayout; - int m_Reserved7[3]; #else - int m_Reserved7[4]; + unsigned int m_Reserved; #endif bool m_bFirstLaunch; bool m_bClipCursor; diff --git a/src/core/kernel/init/CxbxKrnl.cpp b/src/core/kernel/init/CxbxKrnl.cpp index b9e96f990..8c245c436 100644 --- a/src/core/kernel/init/CxbxKrnl.cpp +++ b/src/core/kernel/init/CxbxKrnl.cpp @@ -685,6 +685,17 @@ bool HandleFirstLaunch() void CxbxKrnlEmulate(unsigned int reserved_systems, blocks_reserved_t blocks_reserved) { +#ifdef CXBXR_EMU + // First of all, check if the emulation dll version matches the gui version and abort otherwise + char GitVersionGui[GitVersionMaxLength]; + g_EmuShared->GetGitVersion(GitVersionGui); + if (std::strncmp(GitVersionGui, CxbxGitVersion, GitVersionLength) != 0) { + PopupError(nullptr, "Mismatch detected between cxbx.exe and cxbxr-emu.dll, aborting."); + CxbxKrnlShutDown(); + return; + } +#endif + std::string tempStr; // NOTE: This is designated for standalone kernel mode launch without GUI diff --git a/src/core/kernel/init/CxbxKrnl.h b/src/core/kernel/init/CxbxKrnl.h index 293b8728e..f4df488e6 100644 --- a/src/core/kernel/init/CxbxKrnl.h +++ b/src/core/kernel/init/CxbxKrnl.h @@ -136,9 +136,6 @@ extern "C" { extern Xbe::Certificate *g_pCertificate; -/*! validate version string match */ -bool CxbxKrnlVerifyVersion(const char *szVersion); - extern bool g_bIsDebugKernel; bool CreateSettings(); diff --git a/src/emulator/cxbxr-emu.cpp b/src/emulator/cxbxr-emu.cpp index 44ba5f417..1a0d5d174 100644 --- a/src/emulator/cxbxr-emu.cpp +++ b/src/emulator/cxbxr-emu.cpp @@ -166,6 +166,15 @@ DWORD WINAPI Emulate(unsigned int reserved_systems, blocks_reserved_t blocks_res return EXIT_FAILURE; } + // Check if the loader version matches the gui version and abort otherwise + char GitVersionGui[GitVersionMaxLength]; + g_EmuShared->GetGitVersion(GitVersionGui); + if (std::strncmp(GitVersionGui, reinterpret_cast(PHYSICAL_MAP1_BASE + 0x1000), GitVersionLength) != 0) { + PopupError(nullptr, "Mismatch detected between cxbx.exe and cxbxr-ldr.exe, aborting."); + EmuShared::Cleanup(); + return EXIT_FAILURE; + } + if (!HandleFirstLaunch()) { PopupError(nullptr, "First launch failed!"); EmuShared::Cleanup(); diff --git a/src/loader/cxbxr-ldr.cpp b/src/loader/cxbxr-ldr.cpp index a967ec2f1..a24463ad3 100644 --- a/src/loader/cxbxr-ldr.cpp +++ b/src/loader/cxbxr-ldr.cpp @@ -28,7 +28,9 @@ #define WIN32_LEAN_AND_MEAN // Exclude rarely-used stuff from Windows headers #include // For LPTSTR, FormatMessage, GetSystemInfo, etc +#include "strsafe.h" // For StringCchCopy +#include "..\CxbxVersion.h" #include "..\Common\AddressRanges.h" #include "..\Common\ReserveAddressRanges.h" @@ -193,6 +195,13 @@ DWORD CALLBACK rawMain() return ERROR_RESOURCE_NOT_FOUND; } + // We cannot just pass the gui version of the loader via the Emulate function. This, because if the user mixes a version which does the check (and thus expects 3 arguments) + // with an old version which doesn't do the check (and thus only has 2 arguments), the behavior will be undefined since the new version will attempt to use a + // non-existent argument. We instead pass the version string in the contiguous memory, which must have been successfully reserved by now or else the loader would + // have already aborted execution. This memory is backed by the paging file, and thus its contents will always be initialized to zero. Thus, in the above scenerio + // the check will fail because a version string cannot be zero. + StringCchCopy(reinterpret_cast(PHYSICAL_MAP1_BASE + 0x1000), GitVersionLength + 1, CxbxGitVersion); + // Find the main emulation function in our DLL typedef void (WINAPI *Emulate_t)(unsigned int, blocks_reserved_t); Emulate_t pfnEmulate = (Emulate_t)GetProcAddress(hEmulationDLL, "Emulate"); From a8405c967e1b689570ed82d6df04b447c820c9ba Mon Sep 17 00:00:00 2001 From: ergo720 <45463469+ergo720@users.noreply.github.com> Date: Mon, 5 Apr 2021 13:14:57 +0200 Subject: [PATCH 2/5] Removed reserved variables in EmuShared --- src/common/win32/EmuShared.cpp | 6 ------ src/common/win32/EmuShared.h | 5 ----- 2 files changed, 11 deletions(-) diff --git a/src/common/win32/EmuShared.cpp b/src/common/win32/EmuShared.cpp index 1d8f88367..17b165f28 100644 --- a/src/common/win32/EmuShared.cpp +++ b/src/common/win32/EmuShared.cpp @@ -153,12 +153,6 @@ EmuShared::EmuShared() m_bFirstLaunch = false; m_bClipCursor = false; - // Reserve space (default to 0) - m_bReserved4 = false; - m_Reserved5 = 0; - m_Reserved6 = 0.0f; - std::memset(m_Reserved7, 0, sizeof(m_Reserved7)); - std::memset(m_Reserved99, 0, sizeof(m_Reserved99)); std::memset(m_DeviceControlNames, '\0', sizeof(m_DeviceControlNames)); std::memset(m_DeviceName, '\0', sizeof(m_DeviceName)); m_imgui_general.ini_size = IMGUI_INI_SIZE_MAX; diff --git a/src/common/win32/EmuShared.h b/src/common/win32/EmuShared.h index 93da9711b..e7e747a39 100644 --- a/src/common/win32/EmuShared.h +++ b/src/common/win32/EmuShared.h @@ -335,8 +335,6 @@ class EmuShared : public Mutex // * Shared configuration // ****************************************************************** int m_BootFlags_status; - unsigned int m_Reserved5; - float m_Reserved6; float m_FPS_status; // NOTE: If move into ipc_send_gui_update will spam GUI's message system (one message per frame) bool m_Krnl_Log_enabled; // Is require in order to preserve previous set for support multi-xbe. bool m_bDebugging; @@ -349,13 +347,10 @@ class EmuShared : public Mutex #endif bool m_bFirstLaunch; bool m_bClipCursor; - bool m_bReserved3; - bool m_bReserved4; unsigned int m_dwKrnlProcID; // Only used for kernel mode level. int m_DeviceType[4]; char m_DeviceControlNames[4][HIGHEST_NUM_BUTTONS][HOST_BUTTON_NAME_LENGTH]; char m_DeviceName[4][50]; - int m_Reserved99[28]; // Reserve space // Settings class in memory should not be tampered by third-party. // Third-party program should only be allow to edit settings.ini file. From 7030e1551dbf54832877f1d8bfd2b756716a625e Mon Sep 17 00:00:00 2001 From: ergo720 <45463469+ergo720@users.noreply.github.com> Date: Mon, 5 Apr 2021 15:01:47 +0200 Subject: [PATCH 3/5] Replaced StringCchCopy usage with a for loop --- src/loader/cxbxr-ldr.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/loader/cxbxr-ldr.cpp b/src/loader/cxbxr-ldr.cpp index a24463ad3..d62883462 100644 --- a/src/loader/cxbxr-ldr.cpp +++ b/src/loader/cxbxr-ldr.cpp @@ -28,7 +28,6 @@ #define WIN32_LEAN_AND_MEAN // Exclude rarely-used stuff from Windows headers #include // For LPTSTR, FormatMessage, GetSystemInfo, etc -#include "strsafe.h" // For StringCchCopy #include "..\CxbxVersion.h" #include "..\Common\AddressRanges.h" @@ -195,12 +194,14 @@ DWORD CALLBACK rawMain() return ERROR_RESOURCE_NOT_FOUND; } - // We cannot just pass the gui version of the loader via the Emulate function. This, because if the user mixes a version which does the check (and thus expects 3 arguments) + // We cannot just pass the git version of the loader via the Emulate function. This, because if the user mixes a version which does the check (and thus expects 3 arguments) // with an old version which doesn't do the check (and thus only has 2 arguments), the behavior will be undefined since the new version will attempt to use a // non-existent argument. We instead pass the version string in the contiguous memory, which must have been successfully reserved by now or else the loader would // have already aborted execution. This memory is backed by the paging file, and thus its contents will always be initialized to zero. Thus, in the above scenerio // the check will fail because a version string cannot be zero. - StringCchCopy(reinterpret_cast(PHYSICAL_MAP1_BASE + 0x1000), GitVersionLength + 1, CxbxGitVersion); + for (unsigned i = 0; i < GitVersionLength; ++i) { + *(reinterpret_cast(PHYSICAL_MAP1_BASE + 0x1000) + i) = CxbxGitVersion[i]; + } // Find the main emulation function in our DLL typedef void (WINAPI *Emulate_t)(unsigned int, blocks_reserved_t); From cb9bb62146b288f830b4920313b713e8c4ebf5a4 Mon Sep 17 00:00:00 2001 From: ergo720 <45463469+ergo720@users.noreply.github.com> Date: Mon, 5 Apr 2021 19:04:07 +0200 Subject: [PATCH 4/5] Add misc-batch as dependency of loader --- projects/cxbxr-ldr/CMakeLists.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/projects/cxbxr-ldr/CMakeLists.txt b/projects/cxbxr-ldr/CMakeLists.txt index 1e629bea8..9fef880cf 100644 --- a/projects/cxbxr-ldr/CMakeLists.txt +++ b/projects/cxbxr-ldr/CMakeLists.txt @@ -70,3 +70,5 @@ target_link_libraries(cxbxr-ldr install(TARGETS ${PROJECT_NAME} RUNTIME DESTINATION bin ) + +add_dependencies(cxbxr-ldr misc-batch) From affae4cc0e34c20a249500023c0be0a0bbb75659 Mon Sep 17 00:00:00 2001 From: ergo720 <45463469+ergo720@users.noreply.github.com> Date: Wed, 7 Apr 2021 16:02:27 +0200 Subject: [PATCH 5/5] Review remarks --- projects/cxbxr-ldr/CMakeLists.txt | 1 + src/CxbxVersion.cpp | 13 +++++++++++++ src/CxbxVersion.h | 9 ++------- src/common/win32/EmuShared.cpp | 2 +- src/common/win32/EmuShared.h | 6 +++--- src/core/kernel/init/CxbxKrnl.cpp | 13 ++++++------- src/emulator/cxbxr-emu.cpp | 8 +++----- src/loader/cxbxr-ldr.cpp | 6 ++++-- 8 files changed, 33 insertions(+), 25 deletions(-) diff --git a/projects/cxbxr-ldr/CMakeLists.txt b/projects/cxbxr-ldr/CMakeLists.txt index 9fef880cf..e55012806 100644 --- a/projects/cxbxr-ldr/CMakeLists.txt +++ b/projects/cxbxr-ldr/CMakeLists.txt @@ -32,6 +32,7 @@ file (GLOB HEADERS file (GLOB SOURCES "${CXBXR_ROOT_DIR}/src/common/AddressRanges.cpp" "${CXBXR_ROOT_DIR}/src/common/ReserveAddressRanges.cpp" + "${CXBXR_ROOT_DIR}/src/CxbxVersion.cpp" "${CXBXR_ROOT_DIR}/src/loader/cxbxr-ldr.cpp" ) diff --git a/src/CxbxVersion.cpp b/src/CxbxVersion.cpp index f3b0be0d3..bf65c134c 100644 --- a/src/CxbxVersion.cpp +++ b/src/CxbxVersion.cpp @@ -2,6 +2,7 @@ // PVS-Studio Static Code Analyzer for C, C++ and C#: http://www.viva64.com #include "Version.h" #include "CxbxVersion.h" +#include /*! version string dependent on trace flag */ #ifndef _DEBUG_TRACE @@ -9,3 +10,15 @@ const char* CxbxVersionStr = _GIT_VERSION " (" __DATE__ ")"; #else const char* CxbxVersionStr = _GIT_VERSION "-Trace (" __DATE__ ")"; #endif + +static constexpr const char *GitVersionStr = _GIT_VERSION; +static constexpr size_t GitVersionLength = std::char_traits::length(GitVersionStr); +static_assert(GitVersionLength < GitVersionMaxLength); + +const char *const GetGitVersionStr() { + return GitVersionStr; +} + +const size_t GetGitVersionLength() { + return GitVersionLength; +} diff --git a/src/CxbxVersion.h b/src/CxbxVersion.h index e4f1e1969..d5e98dbb3 100644 --- a/src/CxbxVersion.h +++ b/src/CxbxVersion.h @@ -1,14 +1,9 @@ #pragma once -#include "version.h" -#include - - extern const char* CxbxVersionStr; // Note: GitVersionMaxLength should be large enough to accomodate the longest git version string we can practically expect to have. This is necessary // to avoid possible mismatches in the string length which can happen if the user mixes different cxbxr versions -inline constexpr const char *CxbxGitVersion = _GIT_VERSION; -inline constexpr size_t GitVersionLength = std::char_traits::length(CxbxGitVersion); inline constexpr size_t GitVersionMaxLength = 80; -static_assert(GitVersionLength < GitVersionMaxLength); +const char *const GetGitVersionStr(); +const size_t GetGitVersionLength(); diff --git a/src/common/win32/EmuShared.cpp b/src/common/win32/EmuShared.cpp index 17b165f28..476dadd17 100644 --- a/src/common/win32/EmuShared.cpp +++ b/src/common/win32/EmuShared.cpp @@ -162,7 +162,7 @@ EmuShared::EmuShared() for (auto& i : m_DeviceType) { i = to_underlying(XBOX_INPUT_DEVICE::DEVICE_INVALID); } - std::strncpy(m_git_version, CxbxGitVersion, GitVersionLength); + std::strncpy(m_git_version, GetGitVersionStr(), GetGitVersionLength()); } // ****************************************************************** diff --git a/src/common/win32/EmuShared.h b/src/common/win32/EmuShared.h index e7e747a39..1589c2741 100644 --- a/src/common/win32/EmuShared.h +++ b/src/common/win32/EmuShared.h @@ -67,7 +67,7 @@ class EmuShared : public Mutex unsigned int m_size; // ****************************************************************** - // * Git version string of cxbx.exe + // * Git version string of the executable that first launched // ****************************************************************** char m_git_version[GitVersionMaxLength]; @@ -292,12 +292,12 @@ class EmuShared : public Mutex void SetOverlaySettings(const overlay_settings* value) { Lock(); m_imgui_overlay_settings = *value; Unlock(); } // ****************************************************************** - // * Git version Accessor (only the get method is provided because it should not be changed + // * Git version Accessor (only the get method is provided because it should not be changed) // ****************************************************************** void GetGitVersion(char *value) { Lock(); - std::strncpy(value, m_git_version, GitVersionLength + 1); + std::strncpy(value, m_git_version, GetGitVersionLength() + 1); Unlock(); } diff --git a/src/core/kernel/init/CxbxKrnl.cpp b/src/core/kernel/init/CxbxKrnl.cpp index 8c245c436..dc63a90ac 100644 --- a/src/core/kernel/init/CxbxKrnl.cpp +++ b/src/core/kernel/init/CxbxKrnl.cpp @@ -685,16 +685,15 @@ bool HandleFirstLaunch() void CxbxKrnlEmulate(unsigned int reserved_systems, blocks_reserved_t blocks_reserved) { -#ifdef CXBXR_EMU - // First of all, check if the emulation dll version matches the gui version and abort otherwise - char GitVersionGui[GitVersionMaxLength]; - g_EmuShared->GetGitVersion(GitVersionGui); - if (std::strncmp(GitVersionGui, CxbxGitVersion, GitVersionLength) != 0) { - PopupError(nullptr, "Mismatch detected between cxbx.exe and cxbxr-emu.dll, aborting."); + // First of all, check if the EmuShared version matches the emu version and abort otherwise + char GitVersionEmuShared[GitVersionMaxLength]; + g_EmuShared->GetGitVersion(GitVersionEmuShared); + if (std::strncmp(GitVersionEmuShared, GetGitVersionStr(), GetGitVersionLength()) != 0) { + PopupError(nullptr, "Mismatch detected between EmuShared and cxbx.exe/cxbxr-emu.dll, aborting." + "\n\nPlease extract all contents from zip file and do not mix with older/newer builds."); CxbxKrnlShutDown(); return; } -#endif std::string tempStr; diff --git a/src/emulator/cxbxr-emu.cpp b/src/emulator/cxbxr-emu.cpp index 1a0d5d174..e609d0ee3 100644 --- a/src/emulator/cxbxr-emu.cpp +++ b/src/emulator/cxbxr-emu.cpp @@ -166,11 +166,9 @@ DWORD WINAPI Emulate(unsigned int reserved_systems, blocks_reserved_t blocks_res return EXIT_FAILURE; } - // Check if the loader version matches the gui version and abort otherwise - char GitVersionGui[GitVersionMaxLength]; - g_EmuShared->GetGitVersion(GitVersionGui); - if (std::strncmp(GitVersionGui, reinterpret_cast(PHYSICAL_MAP1_BASE + 0x1000), GitVersionLength) != 0) { - PopupError(nullptr, "Mismatch detected between cxbx.exe and cxbxr-ldr.exe, aborting."); + // Check if the loader version matches the emu version and abort otherwise + if (std::strncmp(GetGitVersionStr(), reinterpret_cast(PHYSICAL_MAP1_BASE + 0x1000), GetGitVersionLength()) != 0) { + PopupError(nullptr, "Mismatch detected between cxbxr-ldr.exe and cxbxr-emu.dll, aborting."); EmuShared::Cleanup(); return EXIT_FAILURE; } diff --git a/src/loader/cxbxr-ldr.cpp b/src/loader/cxbxr-ldr.cpp index d62883462..06298b2af 100644 --- a/src/loader/cxbxr-ldr.cpp +++ b/src/loader/cxbxr-ldr.cpp @@ -199,8 +199,10 @@ DWORD CALLBACK rawMain() // non-existent argument. We instead pass the version string in the contiguous memory, which must have been successfully reserved by now or else the loader would // have already aborted execution. This memory is backed by the paging file, and thus its contents will always be initialized to zero. Thus, in the above scenerio // the check will fail because a version string cannot be zero. - for (unsigned i = 0; i < GitVersionLength; ++i) { - *(reinterpret_cast(PHYSICAL_MAP1_BASE + 0x1000) + i) = CxbxGitVersion[i]; + // NOTE1: the loader doesn't link against the CRT, which means we cannot just use strncpy here, and thus we use a for loop instead + // NOTE2: we choose 0x80001000 as address because the first page is used by d3d to initialize the push buffer of the nv2a, so we avoid to write a string to it + for (unsigned i = 0; i < GetGitVersionLength(); ++i) { + *(reinterpret_cast(PHYSICAL_MAP1_BASE + 0x1000) + i) = GetGitVersionStr()[i]; } // Find the main emulation function in our DLL