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

Dependency work and manual module import table resolver #20

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

raonygamer
Copy link
Contributor

No need to specify the dependency array anymore, the import resolver will automatically load the dependency mod if it isn't loaded and populate the (Import Address Table) with the right addresses from the GetProcAddress with the names from the (Import Lookup Table) of the mod dll.

No need to specify the dependency array anymore, the import resolver will automatically load the dependency mod if it isn't loaded and populate the (Import Address Table) with the right addresses from the GetProcAddress with the names from the (Import Lookup Table) of the mod dll.
Added some error handling, fixed the error of DllMain being called even if there is no DllMain, and changed the C-style casts to be reinterpret_casts
Copy link
Member

@ThePixelGamer ThePixelGamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a second review is going to be needed after the changes are made due to how many structural changes I asked for, it's looking good though for the most part

inc/Zenova/Profile/ModInfo.h Outdated Show resolved Hide resolved
inc/Zenova/Platform.h Outdated Show resolved Hide resolved
src/Zenova/Platform/Windows.cpp Outdated Show resolved Hide resolved
src/dllmain.cpp Outdated Show resolved Hide resolved
src/Zenova/Hooks/InputHooks.h Outdated Show resolved Hide resolved
src/Zenova/Platform/Windows.cpp Outdated Show resolved Hide resolved
inc/Zenova/Platform.h Outdated Show resolved Hide resolved
src/Zenova/Platform/Windows.cpp Outdated Show resolved Hide resolved
src/Zenova/Profile/ModInfo.h Outdated Show resolved Hide resolved
src/Zenova/Utils/Memory.cpp Outdated Show resolved Hide resolved
src/Zenova/Profile/ModInfo.cpp Outdated Show resolved Hide resolved
src/Zenova/Profile/Manager.cpp Outdated Show resolved Hide resolved
src/Zenova/Platform/Windows.cpp Show resolved Hide resolved
Copy link
Member

@ThePixelGamer ThePixelGamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for requesting so many changes, this is the first one that picks at ResolveModModuleImports

@@ -87,6 +88,98 @@ namespace Zenova::PlatformImpl {
FreeLibraryAndExitThread(reinterpret_cast<HMODULE>(CleanupVariables), 0);
}
}

void* LoadModModuleAndResolveImports(const std::string& module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this function be dropped in favor of just using a single function? I'm not sure what other places we would use "ResolveModModuleImports"


typedef BOOL(APIENTRY* DllMainFunction)(HMODULE, DWORD, LPVOID);
void ResolveModModuleImports(void* hModule, const std::string& moduleName)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code base uses { on the same line only

}

uintptr_t hModuleAddress = reinterpret_cast<uintptr_t>(hModule);
PIMAGE_DOS_HEADER dosHeader = reinterpret_cast<PIMAGE_DOS_HEADER>(hModuleAddress);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could the reinterpret cast be inlined into ntHeader's initialization?

PIMAGE_DOS_HEADER dosHeader = reinterpret_cast<PIMAGE_DOS_HEADER>(hModuleAddress);
PIMAGE_NT_HEADERS ntHeader = reinterpret_cast<PIMAGE_NT_HEADERS>(hModuleAddress + dosHeader->e_lfanew);

IMAGE_DATA_DIRECTORY importDir = ntHeader->OptionalHeader.DataDirectory[1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could importDir be inlined into imports initialization and 1 be swapped out with IMAGE_DIRECTORY_ENTRY_IMPORT

void ResolveModModuleImports(void* hModule, const std::string& moduleName)
{
if (!hModule) {
Zenova_Log(Warning, "[{}] Could not resolve the imports for the module because it was nullptr.", __FUNCTION__);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FUNCTION should already be part of the Log macro (look at FSIG)

return hModule;
}

typedef BOOL(APIENTRY* DllMainFunction)(HMODULE, DWORD, LPVOID);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to using and put it with the if statement

PIMAGE_THUNK_DATA importAddressTable = reinterpret_cast<PIMAGE_THUNK_DATA>(hModuleAddress + imports->FirstThunk);

if (imports->OriginalFirstThunk && imports->FirstThunk) {
LPCSTR moduleName = reinterpret_cast<LPCSTR>(hModuleAddress + imports->Name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this just be swapped out with this?

std::string dllName(reinterpret_cast<LPCSTR>(hModuleAddress + imports->Name));
dllName.erase(dllName.length() - 4);

}

std::map<std::pair<uintptr_t, std::string>, ULONGLONG*> symbolTableAddresses;
while (imports->Characteristics != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the while loops be swapped out with a for loop with an empty initialization?

for (; cond; post)

@@ -44,16 +35,7 @@ namespace Zenova {

void* ModInfo::loadModule()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could loadModule be inlined into Manager::loadMod

ModInfo mod(folder);
void* modHandle = mod.loadModule();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modHandle should be dropped in favor of using mod.mHandle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants