UObject::GetName() is a ticking time bomb

[EusthEnoptEron]

I noticed some random crashes in my mod after ~5+ minutes. After some debugging, I found that it seems to be related to UObject::GetName(). (Calling it in a loop greatly helps speeding up the crash.)

My C++ is a bit rusty, but I think returning a stack-allocated pointer might result in memory corruption down the line: https://github.com/RussellJerome/UnrealModLoader/blob/73e6bc6e3e54cb1c338b3301c2b6fe42390792da/UnrealEngineModLoader/UE4/Basic.hpp#L287-L299

Perhaps you could do something to the effect of:

return (char*)this + (GameProfile::SelectedGameProfile.IsUsing4_22 ? 0xC : 0x10);

Anyway, thanks for the framework -- it has been a great help.

[RussellJerome]

Sorry for the late response I rarely check Github lol. I didn't realize that GetName would cause such a problem. I have added your fix into the dev branch and it will be in the next patch/update release (thanks for the heads up) :P

This issue will remain open until I merge the main and dev branch and release the next patch. If you have any more questions, need help, have recommendations, or have profiles you want to share, feel free to join the discord. https://discord.gg/xmXUSNvypY

[EusthEnoptEron]

No problem! Your loader has made it super easy for me to get started with UE4 modding (except that I had to figure out a few addresses... :P I'll provide the profile once I've replaced the offsets with patterns.)

Though I think I might have misinterpreted the issue here:

1. I guess you'd used Read(...) to read memory locations that would otherwise be off limits / different process? In that case, it might be better to go with a different solution that still uses Read().
2. While working on my mod, I came across the same symptoms again (sudden crash) and looking into the issue closer, I think I got the real culprit: https://github.com/RussellJerome/UnrealModLoader/blob/73e6bc6e3e54cb1c338b3301c2b6fe42390792da/UnrealEngineModLoader/UnrealEngineModLoader/Memory/mem.cpp#L5-L15 The process handle is never closed. Meaning that if you keep calling GetName() (old implementation) or any other method that uses Read() (like in my case), after a while the process will crash.

I guess we could just mark the handle static and call it a day (would also improve performance):

bool Read(void* address, void* buffer, unsigned long long size)
{
    static auto hProcess = OpenProcess(PROCESS_ALL_ACCESS, FALSE, _getpid());
    return ReadProcessMemory(hProcess, address, buffer, size, nullptr);
}

bool Write(void* address, void* buffer, unsigned long long size)
{
    static auto hProcess = OpenProcess(PROCESS_ALL_ACCESS, FALSE, _getpid());
    return WriteProcessMemory(hProcess, address, buffer, size, nullptr);
}

Or, of course, close the handle... but that creates some overhead when calling a function that uses it. (In my case, I was calling UObject::IsA(...) in Tick().)

[RussellJerome]

Thanks for the Input! I think leaving the method you told me about for reading the names is fine as is, but I will def be making those variables static. I apologize for that issue, must have gone over my head 😅 . Keeping this issue open still until the next release/dev branch push to main. (Once again sorry for the delay as I never get GitHub issue notifications haha)

[RussellJerome]

Update Released with Fix