Fix Crash on Bootup if Save Directory does not exist.#1250
Fix Crash on Bootup if Save Directory does not exist.#1250TopazTK wants to merge 2 commits intoOpenKH:masterfrom
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughA new directory-ensuring mechanism is introduced in Panacea.cpp via a global flag and FetchPattern helper function. The function scans memory for byte patterns and returns matching addresses. Initialization and asset-loading paths are extended to conditionally create directories based on runtime pattern resolution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
OpenKh.Research.Panacea/Panacea.cpp (2)
1165-1181: Consider moving directory creation toPanacea::Initializeor at least adding error handling forcreate_directories.
std::filesystem::create_directoriescan throwstd::filesystem::filesystem_error. In a function that's on the hot path for every file load (guarded by a bool, but still), an unhandled exception could be worse than the original crash. Wrap in a try-catch or use thestd::error_codeoverload.Additionally, placing this logic inside a file-loading function is a code smell — it's unrelated to loading files. If there's a reason it can't go in
Initialize(), a comment explaining why would help future maintainers.♻️ Use the non-throwing overload
- std::filesystem::create_directories(_stringBuffer); + std::error_code ec; + std::filesystem::create_directories(_stringBuffer, ec); + if (ec && OpenKH::m_DebugLog) + fprintf(stdout, "Failed to create save directory: %s\n", ec.message().c_str());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Research.Panacea/Panacea.cpp` around lines 1165 - 1181, This code currently calls std::filesystem::create_directories inside the file-load path guarded by ENSURE_DIRECTORY; move the directory-creation logic into Panacea::Initialize (or at least out of the hot file-loading function) and/or protect it with non-throwing error handling: replace the throwing overload call in the block that uses FetchPattern/_fetchFunc with the std::filesystem::create_directories overload that takes a std::error_code (or wrap the call in a try-catch and handle/log std::filesystem::filesystem_error), and keep ENSURE_DIRECTORY semantics unchanged; if you must keep it here, add a short comment explaining why it cannot be done in Panacea::Initialize.
300-302:strlen(patvalid)is recomputed every outer-loop iteration.The pattern and mask don't change — hoist
patlenout of the loop. This is a minor inefficiency given the function scans potentially megabytes of memory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@OpenKh.Research.Panacea/Panacea.cpp` around lines 300 - 302, The code recomputes pattern length inside the memory-scan loop; hoist the strlen(patvalid) call out of the outer for loop in Panacea.cpp so patlen is computed once before iterating over addresses (the loop that uses g_hInstance and endAddress and compares against patvalid); replace the inner size_t patlen = strlen(patvalid); with a single declaration/initialization of patlen before the for(...) so the loop body reuses the precomputed length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@OpenKh.Research.Panacea/Panacea.cpp`:
- Around line 293-312: FetchPattern currently can fall off the end without
returning (undefined behavior) and may read out-of-bounds or skip matches by
stepping 0x10; fix by computing size_t patlen = strlen(patvalid) before the
scan, set the scan step to 1 instead of 0x10, compute a safe last-start address
using endAddress - patlen (or endAddress - patlen + 1 as appropriate) to avoid
reading past the module image, and ensure the function returns nullptr when no
match is found; keep references to the existing symbols FetchPattern, pattern,
patvalid, endAddress and moduleInfo.SizeOfImage when making these changes.
- Around line 1165-1181: The directory-ensuring block can crash because
_fetchFunc may be null, _stringBuffer is uninitialized, and create_directories
is unguarded; fix by (1) trying both FetchPattern calls but checking the
returned pointer before invoking it (use FetchPattern(...)->assign to _fetchFunc
and if still null skip directory logic), (2) initialize or replace the stack
buffer (char _stringBuffer[256]) with a zeroed buffer or better a
std::string/std::array to ensure a valid C-string, (3) only call
std::filesystem::create_directories when the resolved path string is
non-empty/valid, and (4) restructure the if/else so the fetch->call mapping is
explicit (e.g., if first pattern found use Steam string, else if second pattern
found use EGS string), and only set ENSURE_DIRECTORY = true after successful
directory creation; references: ENSURE_DIRECTORY, _fetchFunc, FetchPattern(...),
_stringBuffer, std::filesystem::create_directories.
---
Nitpick comments:
In `@OpenKh.Research.Panacea/Panacea.cpp`:
- Around line 1165-1181: This code currently calls
std::filesystem::create_directories inside the file-load path guarded by
ENSURE_DIRECTORY; move the directory-creation logic into Panacea::Initialize (or
at least out of the hot file-loading function) and/or protect it with
non-throwing error handling: replace the throwing overload call in the block
that uses FetchPattern/_fetchFunc with the std::filesystem::create_directories
overload that takes a std::error_code (or wrap the call in a try-catch and
handle/log std::filesystem::filesystem_error), and keep ENSURE_DIRECTORY
semantics unchanged; if you must keep it here, add a short comment explaining
why it cannot be done in Panacea::Initialize.
- Around line 300-302: The code recomputes pattern length inside the memory-scan
loop; hoist the strlen(patvalid) call out of the outer for loop in Panacea.cpp
so patlen is computed once before iterating over addresses (the loop that uses
g_hInstance and endAddress and compares against patvalid); replace the inner
size_t patlen = strlen(patvalid); with a single declaration/initialization of
patlen before the for(...) so the loop body reuses the precomputed length.
| char* FetchPattern(const char* pattern, const char* patvalid) | ||
| { | ||
| HINSTANCE g_hInstance = GetModuleHandle(NULL); | ||
| MODULEINFO moduleInfo; | ||
| GetModuleInformation(GetCurrentProcess(), g_hInstance, &moduleInfo, sizeof(MODULEINFO)); | ||
| const void* endAddress = (const char*)g_hInstance + moduleInfo.SizeOfImage; | ||
|
|
||
| for (char* addr = (char*)g_hInstance; addr < (char*)endAddress - 0x10; addr += 0x10) | ||
| { | ||
| size_t patlen = strlen(patvalid); | ||
|
|
||
| int i = 0; | ||
| for (; i < patlen; i++) | ||
| if (patvalid[i] != '?' && pattern[i] != addr[i]) | ||
| break; | ||
|
|
||
| if (i == patlen) | ||
| return addr; | ||
| } | ||
| } |
There was a problem hiding this comment.
FetchPattern has undefined behavior: missing return when pattern is not found.
If no match is found, control falls off the end of a non-void function, which is UB in C++. The caller (line 1168/1172) will receive a garbage pointer and likely crash or silently corrupt state.
Additional concerns:
- Step size
0x10: Scanning in 16-byte increments will miss patterns at non-aligned offsets. While MSVC x64 tends to align functions to 16 bytes by default, this is not guaranteed for all build configurations — stepping by 1 is the safe choice for a general pattern scanner. - Out-of-bounds read: The boundary
addr < (char*)endAddress - 0x10doesn't account for the actual pattern length (15 bytes). On the last valid iteration,addr[14]could read past the module image.
🐛 Proposed fix
char* FetchPattern(const char* pattern, const char* patvalid)
{
HINSTANCE g_hInstance = GetModuleHandle(NULL);
MODULEINFO moduleInfo;
GetModuleInformation(GetCurrentProcess(), g_hInstance, &moduleInfo, sizeof(MODULEINFO));
const void* endAddress = (const char*)g_hInstance + moduleInfo.SizeOfImage;
- for (char* addr = (char*)g_hInstance; addr < (char*)endAddress - 0x10; addr += 0x10)
+ size_t patlen = strlen(patvalid);
+ for (char* addr = (char*)g_hInstance; addr + patlen <= (char*)endAddress; addr++)
{
- size_t patlen = strlen(patvalid);
-
int i = 0;
for (; i < patlen; i++)
if (patvalid[i] != '?' && pattern[i] != addr[i])
break;
if (i == patlen)
return addr;
}
+ return nullptr;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@OpenKh.Research.Panacea/Panacea.cpp` around lines 293 - 312, FetchPattern
currently can fall off the end without returning (undefined behavior) and may
read out-of-bounds or skip matches by stepping 0x10; fix by computing size_t
patlen = strlen(patvalid) before the scan, set the scan step to 1 instead of
0x10, compute a safe last-start address using endAddress - patlen (or endAddress
- patlen + 1 as appropriate) to avoid reading past the module image, and ensure
the function returns nullptr when no match is found; keep references to the
existing symbols FetchPattern, pattern, patvalid, endAddress and
moduleInfo.SizeOfImage when making these changes.
| if (!ENSURE_DIRECTORY) | ||
| { | ||
| char _stringBuffer[256]; | ||
| bool(*_fetchFunc)(const char* str, char* buff, char* a3) = reinterpret_cast<bool(*)(const char*, char*, char*)>(FetchPattern("\x48\x89\x5C\x24\x20\x55\x56\x57\x48\x81\xEC\x60\x01\x00\x00", "xxxxxxxxxxxxxxx")); | ||
|
|
||
| if (!_fetchFunc) | ||
| { | ||
| _fetchFunc = reinterpret_cast<bool(*)(const char*, char*, char*)>(FetchPattern("\x48\x89\x5C\x24\x20\x55\x56\x57\x48\x81\xEC\x40\x01\x00\x00", "xxxxxxxxxxxxxxx")); | ||
| _fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Epic Games Store", _stringBuffer, nullptr); | ||
| } | ||
|
|
||
| else | ||
| _fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Steam", _stringBuffer, nullptr); | ||
| std::filesystem::create_directories(_stringBuffer); | ||
|
|
||
| ENSURE_DIRECTORY = true; | ||
| } |
There was a problem hiding this comment.
Multiple crash-paths in the directory-ensuring block.
-
Null function pointer dereference (line 1173): If both
FetchPatterncalls returnnullptr(once the missing-return UB inFetchPatternis fixed),_fetchFuncis still null when line 1173 calls it → crash. There is no null guard before the second call. -
Uninitialized buffer passed to
create_directories(line 1178):_stringBufferis stack-allocated and never zero-initialized. If neither pattern resolves (or the resolved function doesn't write to the buffer),create_directoriesoperates on garbage data. -
create_directoriesis called unconditionally (line 1178): Even when no valid path was resolved, the code proceeds to create directories. This should be guarded. -
Blank line between
}andelse(lines 1175-1176): While syntactically valid, the danglingelsestyle is unusual and can be misleading during maintenance.
🐛 Proposed fix
if (!ENSURE_DIRECTORY)
{
- char _stringBuffer[256];
+ char _stringBuffer[256] = {};
bool(*_fetchFunc)(const char* str, char* buff, char* a3) = reinterpret_cast<bool(*)(const char*, char*, char*)>(FetchPattern("\x48\x89\x5C\x24\x20\x55\x56\x57\x48\x81\xEC\x60\x01\x00\x00", "xxxxxxxxxxxxxxx"));
if (!_fetchFunc)
{
_fetchFunc = reinterpret_cast<bool(*)(const char*, char*, char*)>(FetchPattern("\x48\x89\x5C\x24\x20\x55\x56\x57\x48\x81\xEC\x40\x01\x00\x00", "xxxxxxxxxxxxxxx"));
- _fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Epic Games Store", _stringBuffer, nullptr);
}
-
- else
+
+ if (_fetchFunc)
+ {
+ // Try EGS first; fall back to Steam.
+ if (!_fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Epic Games Store", _stringBuffer, nullptr))
+ _fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Steam", _stringBuffer, nullptr);
+
+ if (_stringBuffer[0])
+ std::filesystem::create_directories(_stringBuffer);
+ }
- _fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Steam", _stringBuffer, nullptr);
- std::filesystem::create_directories(_stringBuffer);
ENSURE_DIRECTORY = true;
}Note: The proposed fix above changes the store-detection logic. The original code uses the first pattern to identify Steam vs. EGS, but the two
FetchPatterncalls appear to target different function signatures (differing only in stack frame size0x60vs0x40). Please verify the intended semantics: which pattern corresponds to which store, and whether trying the EGS string first in both cases is correct for your use-case.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!ENSURE_DIRECTORY) | |
| { | |
| char _stringBuffer[256]; | |
| bool(*_fetchFunc)(const char* str, char* buff, char* a3) = reinterpret_cast<bool(*)(const char*, char*, char*)>(FetchPattern("\x48\x89\x5C\x24\x20\x55\x56\x57\x48\x81\xEC\x60\x01\x00\x00", "xxxxxxxxxxxxxxx")); | |
| if (!_fetchFunc) | |
| { | |
| _fetchFunc = reinterpret_cast<bool(*)(const char*, char*, char*)>(FetchPattern("\x48\x89\x5C\x24\x20\x55\x56\x57\x48\x81\xEC\x40\x01\x00\x00", "xxxxxxxxxxxxxxx")); | |
| _fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Epic Games Store", _stringBuffer, nullptr); | |
| } | |
| else | |
| _fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Steam", _stringBuffer, nullptr); | |
| std::filesystem::create_directories(_stringBuffer); | |
| ENSURE_DIRECTORY = true; | |
| } | |
| if (!ENSURE_DIRECTORY) | |
| { | |
| char _stringBuffer[256] = {}; | |
| bool(*_fetchFunc)(const char* str, char* buff, char* a3) = reinterpret_cast<bool(*)(const char*, char*, char*)>(FetchPattern("\x48\x89\x5C\x24\x20\x55\x56\x57\x48\x81\xEC\x60\x01\x00\x00", "xxxxxxxxxxxxxxx")); | |
| if (!_fetchFunc) | |
| { | |
| _fetchFunc = reinterpret_cast<bool(*)(const char*, char*, char*)>(FetchPattern("\x48\x89\x5C\x24\x20\x55\x56\x57\x48\x81\xEC\x40\x01\x00\x00", "xxxxxxxxxxxxxxx")); | |
| } | |
| if (_fetchFunc) | |
| { | |
| // Try EGS first; fall back to Steam. | |
| if (!_fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Epic Games Store", _stringBuffer, nullptr)) | |
| _fetchFunc("KINGDOM HEARTS HD 1.5+2.5 ReMIX\\Steam", _stringBuffer, nullptr); | |
| if (_stringBuffer[0]) | |
| std::filesystem::create_directories(_stringBuffer); | |
| } | |
| ENSURE_DIRECTORY = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@OpenKh.Research.Panacea/Panacea.cpp` around lines 1165 - 1181, The
directory-ensuring block can crash because _fetchFunc may be null, _stringBuffer
is uninitialized, and create_directories is unguarded; fix by (1) trying both
FetchPattern calls but checking the returned pointer before invoking it (use
FetchPattern(...)->assign to _fetchFunc and if still null skip directory logic),
(2) initialize or replace the stack buffer (char _stringBuffer[256]) with a
zeroed buffer or better a std::string/std::array to ensure a valid C-string, (3)
only call std::filesystem::create_directories when the resolved path string is
non-empty/valid, and (4) restructure the if/else so the fetch->call mapping is
explicit (e.g., if first pattern found use Steam string, else if second pattern
found use EGS string), and only set ENSURE_DIRECTORY = true after successful
directory creation; references: ENSURE_DIRECTORY, _fetchFunc, FetchPattern(...),
_stringBuffer, std::filesystem::create_directories.
|
GitHub Actions / .NET Build fails due to something unrelated. Please disregard. |
1 participant
Adhoc fix, needs testing on EGS (if it even happens due to not having a bypass), but this will hold in the meantime.
// Deleted CodeRabbit summerization, we don't do that here.