Support for absolute paths & module handles in EAT hooks #115
Conversation
UnitTests/windows/TestEatHook.cpp
Outdated
|
|
||
| EffectTracker eatEffectTracker; | ||
|
|
||
| typedef void(* tEatTestExport)(); | ||
| uint64_t oEatTestExport; | ||
|
|
||
| extern "C" __declspec(dllexport) NOINLINE void EatTestExport() | ||
| extern "C" [[maybe_unused]] __declspec(dllexport) NOINLINE void EatTestExport() |
There was a problem hiding this comment.
Please remove this function attribute I do not prefer to have these regardless of warnings.
polyhook2/PE/EatHook.hpp
Outdated
| @@ -16,7 +16,9 @@ namespace PLH { | |||
| class EatHook : public IHook { | |||
| public: | |||
| EatHook(const std::string& apiName, const std::wstring& moduleName, const char* fnCallback, uint64_t* userOrigVar); | |||
| EatHook(const std::string& apiName, const std::wstring& moduleName, const uint64_t fnCallback, uint64_t* userOrigVar); | |||
| EatHook(const std::string& apiName, const std::wstring& moduleName, uint64_t fnCallback, uint64_t* userOrigVar); | |||
There was a problem hiding this comment.
const on the fnCallback argument please
There was a problem hiding this comment.
But it has no effect on semantics/logic, afaik. A primitive value is always copied, so the argument is always constant from the caller side. It only makes a difference in function body, which is what I have done in the source file.
There was a problem hiding this comment.
you are correct, but it's helpful to see the interface as a user that this value is const and it can prevent errors in constructors especially when you may accidentally assign to the parameter. This is more style - as most sprinkling on const is - though there are benefits.
| @@ -31,10 +33,15 @@ class EatHook : public IHook { | |||
| virtual HookType getType() const override { | |||
| return HookType::EAT; | |||
| } | |||
| private: | |||
|
|
|||
| protected: | |||
There was a problem hiding this comment.
why the change to protected? I wouldn't expect this to ever be a parent class really, do you see a case someone may want to derive from this as a base class?
There was a problem hiding this comment.
In general, I consider it a sub-optimal design choice to set any class members to private for library classes, because if there is going to be a need to modify the behavior for unforeseeable reasons, the consumer of the library won't be cursing the library author(s) for needlessly restricting access. It's not a pleasant experience to maintain a private fork of the library because it is impossible to inherit a library class to modify it. But that's just my opinion, and I wanted to hear your view on this issue as well.
There was a problem hiding this comment.
That's reasonable justification honestly. I set classes I expected to be derived from to protected already, I just never expected a case it would be useful for this but it doesn't hurt so why not.
sources/EatHook.cpp
Outdated
| dte = (LDR_DATA_TABLE_ENTRY*)dte->InLoadOrderLinks.Flink) { | ||
| // Empty module name implies current process | ||
| if(m_moduleName.empty()){ | ||
| return reinterpret_cast<uint64_t>(dte->DllBase); |
There was a problem hiding this comment.
Please revert all C++ casts to the C style.
|
I think this looks functionally correct, thanks for working on this! I am not a fan of some of the style changes and I've listed the ones I'd like reverted, otherwise looks great thanks! |
|
@stevemk14ebr. I don't know if you receive notifications for comment replies, but I left some questions just to understand on your perspective on these styling conventions, before I implement your suggestions. Thank you! |
|
I can see them! Responded (comment replies dont get a notif) |
|
thanks! |
|
@stevemk14ebr, I have applied suggestions, and left the |
|
I am open to all classes that derive from IHook to be widened to protected yea |
|
Cool, will submit a PR for that later! |
This PR implements support for absolute module paths & module handles for EAT Hooks as discussed in #114.
Some miscellaneous optimizations suggested by Clang-tidy have also been applied.
New tests have been added that cover the new use cases, and test names have been rephrased to reflect that change.