stevemk14ebr / PolyHook_2_0

C++20, x86/x64 Hooking Libary v2.0
MIT License
1.56k stars 227 forks source link

VCPKG bad include folder structure #50

Closed dauthleikr closed 4 years ago

dauthleikr commented 4 years ago

Hi, I tried installing this using vcpkg and getting the following error when including x64Detour.hpp: I:\vcpkg\installed\x64-windows\include\x64Detour.hpp(13,10): fatal error C1083: Cannot open include file: 'headers/Detour/ADetour.hpp': No such file or directory

Looking at the include folder of vcpkg: image

and the content of x64Detour.hpp:

...
#include "headers/Detour/ADetour.hpp"
...

the Problem seems to be that files from headers/Detour/... are here, but the folder structure was flattened (into vcpkg's include directory)?

If I somehow messed up any installation part, I'd appreciate a hint (I'm new to using vcpkg)

xeropresence commented 4 years ago

I was actually in the process of writing up my own issue for this very same problem.

There is a further issue, even if you correct the headers by removing the "header/" and "detour/" chunks, something is wrong with how capstone is being compiled.

It seems like capstone has no active architectures enabled

I used

capstone:x86-windows-static
polyhook2:x86-windows-static

setup a small program c++ program

added

 <VcpkgTriplet Condition="'$(Platform)'=='Win32'">x86-windows-static</VcpkgTriplet>

to the  <PropertyGroup Label="Globals"> in the .vcxproj


#include "CapstoneDisassembler.hpp"
#include "x86Detour.hpp"

NOINLINE int __cdecl hookMe1() {
    volatile int var = 1;
    volatile int var2 = 0;
    var2 += 3;
    var2 = var + var2;
    var2 *= 30 / 3;
    var = 2;
    printf("%d %d\n", var, var2); // 2, 40
    return var;
}

uint64_t hookMe1Tramp = NULL;
NOINLINE int __cdecl h_hookMe1() {
    std::cout << "Hook 1 Called!" << std::endl;

    //effects.PeakEffect().trigger();
    return PLH::FnCast(hookMe1Tramp, &hookMe1)();
}

int main()
{
    PLH::CapstoneDisassembler dis(PLH::Mode::x86);
    PLH::x86Detour detour((char*)&hookMe1, (char*)&h_hookMe1, &hookMe1Tramp, dis);
    detour.hook();
}

and upon execution you get

error opening cap

I stepped into cs_open and it seems like cs_arch_init is empty. So even if the header issue gets resolved it seems like there is a secondary issue with capstone. I've not used vcpkg for very long either so perhaps I too am missing something

xeropresence commented 4 years ago

Looks like the capstone issue was resolved with

vcpkg install capstone[x86]:x86-windows-static --recurse

to enable the x86 capstone feature.

Looks like the vcpkg for polyhook2 needs to have

Build-Depends: capstone replace with Build-Depends: capstone[x86] as that configures capstone correctly.

xeropresence commented 4 years ago

Doing some further digging into how capstone is configured as its headers are inside the capstone directory.

Inside capstones cmake

install(FILES ${HEADERS_COMMON} DESTINATION include/capstone)

from the cmake_install.cmake for capstone

if("x${CMAKE_INSTALL_COMPONENT}x" STREQUAL "xUnspecifiedx" OR NOT CMAKE_INSTALL_COMPONENT)
  file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/include/capstone" TYPE FILE FILES
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/arm64.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/arm.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/capstone.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/evm.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/mips.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/ppc.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/x86.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/sparc.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/systemz.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/xcore.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/m68k.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/tms320c64x.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/m680x.h"
    "C:/Users/x/Downloads/vcpkg-master/buildtrees/capstone/src/6598a4351e-91aba689cf/include/capstone/platform.h"
    )
endif()

seems like this is what vcpkg is using to decide where its output headers go.

Since this repo is lacking any such configuration everything gets dropped into the base includes directory which is not optimal.

I guess there are a few different ways this could be resolved, but I imagine the cleanest way would be to specify a overarching include directory for all the files so everything gets put inside a single polyhook2 directory, and then specify the relative folders for where every header is supposed to be so that cmake puts them in the correct place.

Otherwise a similar patch file that's inside the polyhook2 ports directory for vcpkg could be setup to modify all the headers, but I think having all of polyhooks headers just dropped into the base includes folder isn't very good style.

xeropresence commented 4 years ago

Looks like polyhook2 does have a headers install location, its just the very last line in the cmake

install(FILES ${HEADER_FILES} DESTINATION include)

stevemk14ebr commented 4 years ago

Hey guys sorry I do not use vcpkg myself and another contributer submitted it as a build. If you would like to submit a PR I would happily review, would definitely be interested in fixing.

xeropresence commented 4 years ago

I don't use cmake very often so I'm not sure if theres a better way to do this.

I modified the existing cmake and added

install(FILES ${HEADER_FILES} DESTINATION include/headers)
install(FILES ${DETOUR_HEADER_FILES} DESTINATION include/headers/Detour)
install(FILES ${PE_HEADER_FILES} DESTINATION include/headers/PE)
install(FILES ${VIRTUAL_HEADER_FILES} DESTINATION include/headers/Virtuals)
install(FILES ${EXCEPTION_HEADER_FILES} DESTINATION include/headers/Exceptions)

to the bottom, this sets up all the headers into the correct directories, but again I don't think its great having everything just shoved into a random folder called headers.

We could rename the headers directory to polyhook2 and then modify the cmake to be

install(FILES ${HEADER_FILES} DESTINATION include/polyhook2 )
install(FILES ${DETOUR_HEADER_FILES} DESTINATION include/polyhook2/Detour)
install(FILES ${PE_HEADER_FILES} DESTINATION include/polyhook2/PE)
install(FILES ${VIRTUAL_HEADER_FILES} DESTINATION include/polyhook2/Virtuals)
install(FILES ${EXCEPTION_HEADER_FILES} DESTINATION include/polyhook2/Exceptions)

that would create a very nice directory structure, but we would have to modify all the existing files in the repo that include

#include "headers/..."

@stevemk14ebr what do you think about this solution?

Regardless we also will have to submit a pr to the vcpkg repo to enable capstone[x86] but I don't think that will be too big of a deal, but should we add some of the other capstone features too, or is x86 the only ones used by polyhook2?

stevemk14ebr commented 4 years ago

I have no problem with relative imports. If you submit a PR with all the required changes I can review and test the building and then we can tackle vcpkg. I got lots of tests, so should be easily to tell if it gets messed up

xeropresence commented 4 years ago

Alright, I have to go out for a few hours when I get back I'll try issing a pr.

stevemk14ebr commented 4 years ago

This looks like it may help instead, maybe try this? https://stackoverflow.com/questions/11096471/how-can-i-install-a-hierarchy-of-files-using-cmake

and yes we only use the x86/x64 feature of capstone. Polyhook cannot hook any other architectures.

xeropresence commented 4 years ago

That works as well. I was just looking at the patchfile included in the vcpkg repo

https://github.com/microsoft/vcpkg/blob/master/ports/polyhook2/fix-build-error.patch

and noticed that it was what added the install command, so the question is should the replacement command be merged into the main repo as well, or should it merely be updated inside that patch.

I've issued https://github.com/stevemk14ebr/PolyHook_2_0/pull/51 which has ommited the change, if you think it should be included then I'll push another commit that adds

INSTALL(DIRECTORY ${PROJECT_SOURCE_DIR}/polyhook2 DESTINATION include/) which worked when i tested it

stevemk14ebr commented 4 years ago

I am fine with the changes in that patch file, if you wouldn't mind adding them. Please verify that it builds locally without vcpkg as well, and then if this all works ill merge and we can move onto the vcpkg PR

xeropresence commented 4 years ago

Including those changes breaks compiling outside vcpkg

xeropresence commented 4 years ago
1> [CMake] CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
1> [CMake] Please set them or make sure they are set and tested correctly in the CMake files:
1> [CMake] C:/Users/x/Documents/PolyHook_2_0/CAPSTONE_INCLUDE_DIR
1> [CMake]    used as include directory in directory C:/Users/x/Documents/PolyHook_2_0
1> [CMake] CAPSTONE_LIBRARY
1> [CMake]     linked by target "PolyHook_2" in directory C:/Users/x/Documents/PolyHook_2_0
xeropresence commented 4 years ago

Could leave this portion in, as there was no issue compiling inside visual studio with it

#Install targets
install(TARGETS ${PROJECT_NAME}
  RUNTIME DESTINATION bin
  LIBRARY DESTINATION lib
  ARCHIVE DESTINATION lib
)

#Install headers
INSTALL(DIRECTORY ${PROJECT_SOURCE_DIR}/polyhook2 DESTINATION include/)
xeropresence commented 4 years ago

I've gone and pushed another commit to that pr that adds that second chunk

stevemk14ebr commented 4 years ago

tested and merged, thanks. Please author the appropriate PR for vcpkg now

stevemk14ebr commented 4 years ago

I've incorporated these changes into the zydis branch as well. If it is possible please allow vcpkg to build with that branch too. Your work is appreciated very much.

xeropresence commented 4 years ago

I've only been using vcpkg for a few days, when I wanted to add gumbo to my new project (https://github.com/xeropresence/AOPP) I wanted to make it easy for others to compile so that's when I started looking into it and noticed polyhook2 (which is also used by my new project) was on there as well. With that being said, im not sure how you would separate capstone vs zydis, perhaps as a 'feature'? I'll look into getting a PR into the vcpkg repo, looks like I'll have to sign and agree to their CLA.

stevemk14ebr commented 4 years ago

If it's too hard no worries 😃

stevemk14ebr commented 4 years ago

ok i made it easy. Zydis was a super set of capstone, so now master has both capstone and zydis in it and i deleted the branch.

xeropresence commented 4 years ago

theres nothing preventing someone from compiling with both zydis and capstone correct? you would decide which one you wanted when you passed in the dissam to the hook right?

stevemk14ebr commented 4 years ago

That's correct. They're fully abstracted see the templated tests: https://github.com/stevemk14ebr/PolyHook_2_0/blob/f5051a47888cc0960e6592f0c65dec2b214f9139/UnitTests/TestDetourx86.cpp#L131

xeropresence commented 4 years ago

Looks like that actually makes it more complicated.

vcpkg now fails even with setting DEP_ZYDIS_NEED to off.

The project is including CapstoneDisassembler.hpp and ZydisDisassembler.hpp regardless of the flags, and as such has a hard-dependency on them.

stevemk14ebr commented 4 years ago

When I get a chance I'll make the includes guarded. Make it without those set to off for now and I'll fix it up.

xeropresence commented 4 years ago

I've got it so locally vcpkg will compile it.

I was looking over the other patch they include

https://github.com/microsoft/vcpkg/blob/master/ports/polyhook2/fix-build-tests-error.patch

and this made me wonder if the default build config for the project is optimal.

Would it make more sense for the default build configuration be to construct the library/dll instead of the unit tests?

xeropresence commented 4 years ago

Looks like zydis compat is out the window for the moment anyway,

https://github.com/microsoft/vcpkg/pull/8426

they are still using version 2.0 which lacks zycore

stevemk14ebr commented 4 years ago

is it possible to build zydis without using the version in vcpkg? For example i don't believe we use the capstone version that is in vcpkg (if there even is one)

xeropresence commented 4 years ago

vcpkg uses the capstone vcpkg. I've issued a vcpkg pr here https://github.com/microsoft/vcpkg/pull/9932

xeropresence commented 4 years ago

https://github.com/microsoft/vcpkg/pull/9932

The pr has been merged, using vcpkg install polyhook2 should work as expected now!