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
2 changes: 2 additions & 0 deletions src/Zenova/Platform/PlatformImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,7 @@ namespace Zenova {
namespace PlatformImpl {
bool Init(void*);
void Destroy();
void* LoadModModuleAndResolveImports(const std::string& module);
void ResolveModModuleImports(void* hModule, const std::string& moduleName);
}
}
93 changes: 93 additions & 0 deletions src/Zenova/Platform/Windows.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <iostream>
#include <stdexcept>
#include <tuple>
#include <map>

#include "Zenova.h"
#include "Zenova/Globals.h"
Expand Down Expand Up @@ -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"

{
void* hModule = LoadLibraryExA(module.c_str(), NULL, DONT_RESOLVE_DLL_REFERENCES);
ThePixelGamer marked this conversation as resolved.
Show resolved Hide resolved
ResolveModModuleImports(hModule, module);
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

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

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)

Platform::DebugPause();
return;
}

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_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

PIMAGE_IMPORT_DESCRIPTOR imports = reinterpret_cast<PIMAGE_IMPORT_DESCRIPTOR>(hModuleAddress + importDir.VirtualAddress);
if (!imports) {
Zenova_Log(Warning, "[{}] Could not resolve the imports for the module because the imports data directory could not be found in the PE.", __FUNCTION__);
Platform::DebugPause();
return;
}

std::map<std::pair<uintptr_t, std::string>, ULONGLONG*> symbolTableAddresses;
Copy link
Member

Choose a reason for hiding this comment

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

Using a std::map here doesn't really make sense, I think we should make a new struct type here and use that with std::vector
an example using the names from the structured bindings

struct SymbolTableAddress {
  uintptr_t moduleBase;
  std::string entrySymbol;
  ULONGLONG* function;
}

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)

PIMAGE_THUNK_DATA importLookupTable = reinterpret_cast<PIMAGE_THUNK_DATA>(hModuleAddress + imports->OriginalFirstThunk);
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::string importModNameId(moduleName);
{
size_t dot = importModNameId.find_last_of('.');
if (dot != std::string::npos) {
importModNameId = importModNameId.substr(0, dot);
}
}

uintptr_t moduleBase = Platform::GetModuleBaseAddress(moduleName);
if (!moduleBase) {
moduleBase = (uintptr_t)manager.loadMod(importModNameId);
if (!moduleBase)
moduleBase = (uintptr_t)LoadLibraryA(moduleName);
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 be swapped for Platform::LoadModule

}

while (importLookupTable->u1.AddressOfData != 0) {
if (!(importLookupTable->u1.Ordinal & IMAGE_ORDINAL_FLAG)) {
PIMAGE_IMPORT_BY_NAME nameData =
reinterpret_cast<PIMAGE_IMPORT_BY_NAME>(hModuleAddress + importLookupTable->u1.AddressOfData);

symbolTableAddresses.insert({ { moduleBase, nameData->Name }, &(importAddressTable->u1.Function) });
}
importLookupTable++;
importAddressTable++;
}
}

imports++;
}

for (auto& [infos, function] : symbolTableAddresses) {
auto& [moduleBase, entrySymbol] = infos;
if (moduleBase) {
ULONGLONG procAddress = reinterpret_cast<ULONGLONG>(
Platform::GetModuleFunction(reinterpret_cast<void*>(moduleBase), entrySymbol.c_str()));
Copy link
Member

Choose a reason for hiding this comment

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

the c_str should be dropped as it's forcing the creation of another string


if (procAddress && function) {
u32 oldPageProtection = Platform::SetPageProtect(function, sizeof(ULONGLONG), ProtectionFlags::Execute | ProtectionFlags::Read | ProtectionFlags::Write);
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd be okay with switching from the ProtectionFlags to PAGE_EXECUTE_READWRITE here, it would reduce the clutter a bit (also oldProt is a good enough name)

(*function) = procAddress;
Platform::SetPageProtect(function, sizeof(ULONGLONG), oldPageProtection);
}
}
else {
Copy link
Member

Choose a reason for hiding this comment

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

I think this error handling should be moved up to where moduleBase is set, nothing else is effectively executed anyways if it's null

Zenova_Log(Warning, "[{}] Could not resolve the address for '{}' because the module could not be loaded.", __FUNCTION__, entrySymbol);
return;
}
}

if (ntHeader->OptionalHeader.AddressOfEntryPoint) {
reinterpret_cast<DllMainFunction>(hModuleAddress + ntHeader->OptionalHeader.AddressOfEntryPoint)(
reinterpret_cast<HMODULE>(hModule),
DLL_PROCESS_ATTACH,
nullptr
);
}
}
}

namespace Zenova {
Expand Down
16 changes: 11 additions & 5 deletions src/Zenova/Profile/Manager.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#include "Manager.h"

#include <filesystem>
#include "Zenova/Log.h"
#include "Zenova/Mod.h"
#include "Zenova/Globals.h"
Expand Down Expand Up @@ -102,29 +102,35 @@ namespace Zenova {
load(profile);
}

void Manager::loadMod(const std::string& modName)
void* Manager::loadMod(const std::string& modName)
{
if (std::find_if(mods.begin(), mods.end(),
[&modName](const ModInfo& mod) { return mod.mNameId == modName; }) != mods.end())
return;
return nullptr;

std::string folder = manager.dataFolder + "\\mods\\" + modName + "\\";
if (!std::filesystem::exists(folder))
return nullptr;

logger.info("Loading {}", modName);

// todo: verify path
std::string folder = manager.dataFolder + "\\mods\\" + modName + "\\";
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


if (mod.loadModule()) {
if (modHandle) {
ModContext ctx = { folder };
CALL_MOD_FUNC(mod, ModLoad, ctx)

PackManager::addMod(folder);

mods.push_back(std::move(mod));
return modHandle;
}
else {
logger.warn("Failed to load {}", mod.mName);
Platform::ErrorPrinter();
return nullptr;
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Zenova/Profile/Manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include <vector>

#include "ProfileInfo.h"
#include "ModInfo.h"
#include "Zenova/Profile/ModInfo.h"

namespace Zenova {
class Manager {
Expand All @@ -28,7 +28,7 @@ namespace Zenova {
void update();
void load(const ProfileInfo& profile);
void swap(const ProfileInfo& profile);
void loadMod(const std::string& modName);
void* loadMod(const std::string& modName);
std::string getVersion();
size_t getModCount();

Expand Down
24 changes: 3 additions & 21 deletions src/Zenova/Profile/ModInfo.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#include "ModInfo.h"
#include "Zenova/Profile/ModInfo.h"

#include <fstream>
#include <utility>
Expand All @@ -17,14 +17,6 @@ namespace Zenova {
mName = JsonHelper::FindString(modDocument, "name");
mDescription = JsonHelper::FindString(modDocument, "description");
mVersion = JsonHelper::FindString(modDocument, "version");
auto& dependenciesObject = JsonHelper::FindMember(modDocument, "dependencies", false);
if (!dependenciesObject.IsNull() && dependenciesObject.IsArray())
{
for (auto& object : dependenciesObject.GetArray())
{
mDependencies.push_back(object.GetString());
}
}
}
}

Expand All @@ -34,8 +26,7 @@ namespace Zenova {
mNameId(std::move(mod.mNameId)),
mName(std::move(mod.mName)),
mDescription(std::move(mod.mDescription)),
mVersion(std::move(mod.mVersion)),
mDependencies(std::move(mod.mDependencies))
mVersion(std::move(mod.mVersion))
{}

ModInfo::~ModInfo() {
Expand All @@ -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

loadDependencies();
mHandle = Platform::LoadModule(mModFolder + mNameId);
mHandle = PlatformImpl::LoadModModuleAndResolveImports(mModFolder + mNameId);
return mHandle;
}

void ModInfo::loadDependencies() const
{
for (auto& dependencyName : mDependencies)
{
manager.loadMod(dependencyName);
}
}
}
3 changes: 0 additions & 3 deletions src/Zenova/Profile/ModInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,10 @@ namespace Zenova {
std::string mName = "";
std::string mDescription = "";
std::string mVersion = "";
std::vector<std::string> mDependencies = {};

ModInfo(const std::string& modFolder);
ModInfo(ModInfo&&) noexcept;
~ModInfo();

void* loadModule();
void loadDependencies() const;
};
}
Loading