Replacing CDetour #2153
Comments
Because it uses |
|
Can you tell me what steps are needed to replace it? |
I haven't tried this because I use C++23 for pretty much everything but it should work Edit: Please use |
|
Ok I just tested it, seems to be easier to use once you go through the hassle of renaming everything and only 2 cpp files. Only nitpick is that it doesn't have an option to start with the hook disabled but I guess that could be worked around somehow. But safetyhook does seem like the lighter and better solution. I attached the edited files if anyone wants to try. Hopefully this can be decided sooner rather than later because it means extensions will have to be recompiled if they are using the broken cdetour. |
|
Has the complexity of "fix CDetour" been considered? We have libudis86 in there already, which should be able to easily give us the instruction info we need to rewrite the addresses (or to non-relative addressing) as part of copying the instructions to the trampoline location - is that the only major problem? My motivation for asking this:
|
|
Removing CDetour isn't wise. Sourcemod afaik does not detour any function whose first assembly bytes are relative addressing operation ? but even if there are any in use, we could patch that on a per function basis. Then there's the issue of community extensions, many of them pull CDetour directly from the official sourcemod repository. Removing CDetour means also potentially breaking those extensions (unless we keep CDetour for an undefinite amount of time). It'd make more sense for extensions to implement their own detour library, if CDetour isn't enough for them. |
|
My take on replacing CDetour: instead of replacing it entirely, why not make it as a wrapper for other hook libraries so it won't bring any breaking changes to community extensions, if it is really needed. |
I glanced at it and for me it would take longer than I'm willing. It isn't rocket science, but it isn't exactly trivial either. And add to the uncertainty of your work being accepted, probably the only people that want to do it are the core sourcemod devs like yourself or TheDS. Adding support for fixing relative addressing seems like a major task especially because udis86 doesn't seem to have metadata indicating whether an instruction has a relative offset, while zydis does. These libraries also have other goodies like pausing all threads, using code caves, or mid-function hooks. I don't know if they are necessary but I feel like alliedmodders doesn't have the manpower anymore (given the speed at which x64 fixes are coming in) nor should it be wasted it on a half baked detour library. udis also hasn't been updated in quite a while so maybe it's even missing some instructions, even though valve probably won't use newer instructions, but you never know. I think a compatibility header might be possible without too much work, you could still use the CDetour class with all the same interface, just calling the new library behind the scenes. But I don't know how to translate the class function macros and some of the other macros I never use. I'm sure one of you could easily figure it out.
Do you know this for sure? Even if no new detours are ever added, I don't think it is safe to bet future compiles will never rearrange code. People are also adding new hooks to sourcemod from time to time. |
There's no reason to turn down contributions. Changing detouring libs has more uncertainties than fixing the present lib.
I don't know on which scale you rate the speed at which x64 fixes come in. But Sourcemod and Metamod have been ready for tf2 x64 for a while. And I've begun the work as early as last year alliedmodders/hl2sdk#127 - What remains are a few bug fixes with sdktools, which are in no hurry to be merged. TF2 32 bits support drop is still quite a while away, and it isn't like sourcemod and metamod are fundamentally broken. Both SM & MM do very much work on TF2 x64. What we lack is playtest, because not many server operators are willing to make the leap like you are and report issues.
If you want to change the detouring lib, then yes much like you say, the experience should be seamless.
Neither it is safe to bet at all that detours are eternal. Without speaking of relative addressing, if a function gets inlined, or optimised, that detour is still gonna crash. We can still operate on a case by case basis, and fix any detour that happens to break in a future update. I'd still argue that CDetour should just be fixed, but if you want to pursue an alternative library and make a proof of concept then why not. But I think this entails much more work. |
There are plenty of reasons, you just mentioned one of them. I've also seen contributions get turned down here as well, so I'm always thinking if I want to bother doing the work with the probability that the work gets thrown away.
While your pull request was started a year ago, it took a year before it was finally merged. That doesn't exactly instill confidence. I'd expect most things that cause the server to crash instantly to have been fixed many months ago, and it seems like no one had released any 64 bit compiled extensions until I did.
Do you have any source for this? I asked Valve, and they refused to give a time frame. So 32 bit servers can be dropped at any time with no warning. If you somehow have a guarantee from someone in power like Eric Smith, then I'd understand the lack of urgency.
I believe that it should be possible to just edit the CDetour class and the macros so that you wouldn't have to change anything else but I don't know for sure. I am completely unfamiliar with the class function macros. My guess is that there shouldn't be any issues but I don't know for sure. I just don't much benefit in duplicating existing libraries. You also have to maintain it, and as we have seen, the current implementation is struggling to stay relevant. |
You make it sound like contributions being turned down is random. Usually the PR has been just inactive, or the author has refused to do the requested changes. Anyways let's not argue this point further, we will end up derailing this conversation.
Because I wanted it to be merged a year later. TF2 x64 existence was not confirmed back then only rumoured, albeit from reliable sources. But as soon as tf2 x64 went live, this PR got merged. So the real delay is 2 days rather than a year.
Which is the case, Metamod and Sourcemod do not insta crash on tf2 x64. They've been long stable in that regard.
asherkin/connect#15 - asherkin/accelerator#13 - SouthernCrossGaming/async - SouthernCrossGaming/SteamWorks - caxanga334/NavBot etc...
I'd rather not name drop anyone for my source of information. However it has been guaranteed ever since the release of x64 beta, that it would go in '3 phases', the first being the 'x64 beta test' which begun on January and concluded last month, the hybrid phase of 32 bits/64 bits which we are currently in, and finally the full drop of 32 bits. While no precise date, that let me know we had quite the breathing room. And again, for the most part SM & Metamod are ready for tf2 x64. Bugs have been found in sdktools and quickly fixed. Bugs we would have found earlier if more server operators made the leap to 64 bits. Support for 32 bits could drop tomorrow, we would still be operational. The two real issues are Dhooks and Address type for plugins, but they're their own world of problems, completly disconnected from this conversation.
If dealing with the macros part is too complicated, feel free to just make a PR proof concepting the bridge that connects SM API (i.e gamedata reading) to this new detouring lib. And I can contribute the macros in later. I can't guarantee the PR will be merged, but if you want any considerations to be taken towards changing the core detouring lib, somebody has to make the first step for it. And it has to be well justified, with why fixing CDetour would take more time/be impractical than an using another library, quoting the manpower as justification isn't enough imo. |
sdktools is used in default sourcemod plugins, and not counting that as part of sourcemod misleading.
Ok the situation is better than I thought, but no binary for connect, no binary for steamworks, your accelerator pull has not been merged. Someone made a copy of my extension without telling me or even forking it properly? This still makes me think that sourcemod does not have the manpower to be reimplementing parts of libraries like zydis.
I don't follow how this means that we have a lot of breathing room. The beta test lasted from Feb 3 to April 18. That's 2.5 months. 1 month is gone already and you're assuming the hybrid phase won't be shorter than the beta test.
I don't want to ruffle too many feathers here. But all I'm going to say is that PRs do get stuck in limbo here for trivial reasons, there is a very recent example of this. I also don't know how you can believe this when later you say this:
So are you not admitting that you already imagine a situation where you spend a lot of time on a CDetour replacement just to have the work abandoned? If fixing CDetour is as easy as you claim, why has it not been fixed already or why hasn't anyone come up and said they are committing to fixing this? This is a bottleneck for extension porting and a ticking time bomb for sourcemod core. |
I never claimed that, and the issues that have been reported for sdktools have been all trivial to fix. You make it sound like it's a way bigger deal than it actually is. And repeating myself, the only reason those 'bugs' have only been discovered this late, is because not many server owners have made the leap to x64 bits. It's the same issue as when toolchains update released for tf2, or vscript beta. It says nothing about the dev cycle of sourcemod, and is merely a consequence of how the community operates.
They all have binaries, either attached in the CI or on their official build repositories. You use my PR not being merged on accelerator as supporting evidence of your point, but you should read the discussion over there in full and see the reason it hasn't been merged yet is simply because we need a few extra changes. There's nothing stuck in limbo.
That's the nature of open-source, the maintainers of this repository can't be here all the time, nor remember everything. And they do very much read new PRs and issues, if you want something merged feel free to ping any of them for their thoughts.
This is merely speaking of caution, I can't tell you 'yes do this PR it will be merged' this would be getting ahead of myself. This is just open-source in general, a maintainer can decide for X reason a change is invalid. I'm in favor of any changes that would ultimately make detouring more robust, but it has to be sound changes. And the maintainers could be thinking the same.
You're being incredibly insensitive here. There are many, many things that require fixing in sourcemod core. And blaming generally anyone for why it hasn't been fixed on the grounds that it's easy, is being seriously insulting. From an objective standpoint, CDetour is by far a low issue, nothing in sourcemod core currently crashes because of it, and if something does it has yet to be reported. Extensions, contrary to plugins, have at least the liberty of changing their detour library, independently of what sourcemod has to offer. Edit : If you want to provide an alternative to CDetour for extension authors, go for it. I've already offered my help for the macro part, even if your PR ends up not being merged, that possibility shouldn't deter you ? Extensions are free to pull whatever detouring framework they want, if they want to use your changes then they will. |
I completely disagree. dhooks only made it to the core recently, we all got by without it for 10+ years. Even now it's not needed and is only a convenience. I'm only using dhooks for 4 hooks out of convenience and they could easily be ported back to C++. Dhooks is used in 0 core plugins or extensions while CDetour is. People are either going to release broken 64 bit ports or not bother porting if CDetour remains broken. I've never seen anyone use PrepSDKCall_SetAddress. Calling a function that doesn't have a symbol or pattern seems like madness to me. Source games don't JIT functions. Why is pseudo address a problem now when it's been around since 2017?
As I just mentioned, CDetour is used in sourcemod and other people's extensions. If all I had to worry about was my own code, then yes it wouldn't be an issue. |
The issue isn't with Not all 64 bits addresses can be mapped to PseudoAddress, and plugins most notably tf2attributes or gamemode plugins. They happen to be making SDKCalls that involve pointers as parameters. And while it was possible before to pass them as 32 bits ints, that's not possible anymore, and because we can't map all pointers to PseudoAddress we can't update sdkcall to support a special parameter type. So those plugins are effectively dead on arrival until that's fixed.
If you worry about other's code, then you should know that a large chunk of tf2 plugins heavily relies on dhooks. Even prior its officialisation into the core. We're talking a good +10 years for some of them.
etc.... You're right, dhooks is convenience at the end of the day, but converting all the detours they use into extensions would require much more work than just fixing dhooks. On the flip side I don't think there's this many extensions out there remaining that make use of CDetour and are affected by the relative address issue. Again, if you want to work on alternative backend for CDetour go ahead. You're doing this for the community anyways, if you cant get the macro part to work, people will pitch in, but help people get there by proof concepting. |
I don't think it's fair for a few niche game modes to be holding up the fixing of code that affects the core. If dhooks is broken, it doesn't crash everything else. If CDetours is broken then sourcemod itself will crash or has the potential to crash on a random update. Most of these game modes only run on 1-2 servers. We don't use any of those plugins. It seems like we are only having this argument because you think dhooks is more important because you are more involved with custom game modes than we are. But anyway I think I've managed to do it without breaking any compatibility at all, but I don't use the macros myself and I don't understand the class macros at all so hopefully someone can test them. |
And it hasn't, I've fixed the rest of sourcemod & metamod for tf2 x64 prior tackling the dhooks problem, and sourcemod/metamod run perfectly fine as of writing this post. Minus the few sdktools bugs that have been reported recently, and quickly fixed. No choices have been made here, why should anyone work on something that's seemingly not broken ? It's great if you want to make sure things are stable for the future though 👍 People contribute what they think is important, and nobody is being dictated on what they should do. Regardless of what you or I think. And there's no need to make unsavory comments on sourcemod development cycle and state of PRs in the process. If you want to contribute something for the benefit of everyone, then just do it. Be ready to have some scrutiny on the changes, that's all.
I can give this a try tomorrow, thanks for the code, though from a quick glance as long as you've reimplemented the whole |
|
Small update, I'm afraid I'm gonna have to wait the weekend before I can get the chance to try this out. |
There's a lot of conversation here, the simple technical background is nothing has changed around relative address handling with CDetour from x86 to x64. This has always been a problem, and is worked around by ordering calls presently which is not as clean as it could have been then, and now. Insinuating this is a new problem is fine because compilers have changed dramatically so, but it's a legacy problem that we should try to resolve. Apologies for the issue toggle. |
I don't think it was ever a problem in x86 because srcds doesn't omit frame pointers so it always had a function prologue that was guaranteed to be free of relative addressing and fit a 32 bit jump so no actual function logic needed to be relocated. Whatever compiler valve is using on x64 is putting lea rax,[rip+x] at the very top of the function. On x64, if your trampoline is further than 2gb, the jump takes 15 bytes. I have maybe 100 functions detoured and none of them ever crashed in 32 bit. The more I look into 64 bit hooking, the more I doubt anyone here with the chops to reimplement this stuff in cdetour wants to spend the time to do so. Even if you do change relative offsets, there's the fact that some instructions aren't even able to accept offsets larger than 2gb like cmp. You have to load the address into a register. And now you have to make sure the register isn't already being used. So you might be asking yourself by now, why not just allocate memory within 2gb? That's what safetyhook does right now and I haven't seen it fail yet, but it does this by spamming mmap 4096 bytes at a time until it succeeds. This just makes things a bit slower on startup but better than crashing. Only polyhook2 handles this situation properly, but it crashes due to using iostream which breaks when you statically link libstdc++, and the author doesn't want to accept my compile flag to replace iostream with stdio stuff. |
|
why spam the address space when you can read /proc/maps or VirtualQueryEx? why do you even need to allocate memory close by? for functions that are < N bytes? (I'm guessing "N" is ~12 or whatever it takes to encode an absolute mov + jmp). how many functions that people want to hook right now are in that category? why would you need cmp in a detour thunk? why would you need to check if the register is used when you can save and restore it? and, if the register is used, aren't you're hosed anyway because the calling convention is different? I just took a look at CDetour for the first time (yes) and... it's like 300 lines of code? I think if something mashed a keyboard it might accidentally trigger the right keys to fix it on the first try. Anyone have a cat? |
I'm not a linux kernel expert, but I would guess its just easier than parsing text. Chances are there is nothing allocated -2gb from where the code is stored.
because that allows you to fix rip relative offsets simply by changing the offset number instead of converting into multiple instructions.
because due to compiler behavior on 64 bits, stuff like this is being put at the very beginning of a function, even before frame pointer setup. Straight from tf2. No matter what you do, you are going to end up with a relative instruction in your trampoline. This probably explains the situation better than I can. |
|
and because of the [rip+X] it's non-trivial(ish) to relocate the instruction into the thunk? if that kind of thing will be common in the target binary - that's a pretty good argument for not reinventing the wheel. big if, but assuming (1) that's true, and (2) polyhook solves every problem, and (3) is a drop-in replacement for CDetour... stevemk14ebr/PolyHook_2_0#200 this PR? seems like the maintainer might be amenable to the changes and wanted a cleaner patch set, which is pretty reasonable. worst case though, fork it into AM's repository. we typically do this anyway even if we're not making changes. |
|
also, curious, why does a detouring library need iostream at all? |
if your thunk/trampoline is farther than 32 bits, you can't fix [rip+x] simply by changing x because x can only be 32 bits on many instructions.
Yes. but I think it would be better not to fork any libraries so we can keep benefitting from updates without having to worry about merge conflicts. Safetyhook is looking into proper relative instruction rewriting and also has a unique feature to pause threads and ensure they aren't running in detours while they are being modified, maybe that would be added to polyhook in the future.
The library has a logging feature. |
I recommend not rushing the work and working at our own pace rather than doing sloppy work and regretting it later. It's just video-game and not end of world if 64-bit update comes out a bit earlier and SourceMod taking a while to get back up and running. Just like how it is right now with CSGO to CS2 transistion. SourceMod is volunteer work meant for fun and enjoyment rather than some ticking clock down to the deadline and needing to use terms like "breathing room" at all lol. Majority of people who are getting uppity about slow progress on x64 SourceMod are Communities who make 3000$+ or more per month from in-game adverts. For those people I recommend perhaps spending some of that revenue on hiring software developers to port SourceMod to x64 and perhaps contributing that work to repo itself at same time, instead of pressuring volunteer workers who make much much less if anything for helping you keep those money farm servers up and running. Bad business decisions are not SourceMod devs responsiblity |
I don't know why people think I make a lot of money on ads. Nobody is paying top dollar for ads that 99% of people close in 1 second. The amount I get from ads don't even cover the cost of the servers by itself. Even as the largest TF2 community, we're not getting anywhere close to 3000, and even if you did you're not going to find anyone that can do this kind of work when they are being paid $10,000/month right out of college. This work is not being rushed, nor is it rocket science. Most of this can be fixed in 1 weekend provided you use libraries that already do the work for you and you accept pull requests in a timely manner and not wait for the perfect one. There's nothing being done that can't be reversed in the future. If you think that this is a volunteer project, who cares if sourcemod is down for months, you obviously don't care about tf2 as much and you're entitled to that opinion. But your fantasy about tf2 communities being rich and greedy for not hiring programmers to work on this is not only wrong but highly insulting. |
|
It would be very much appreciated if the pissing match stopped long enough for us to actually discuss the issue at hand if all parties are interested. With x64 we use a 8-byte absolute jump, right? That’s going to absolutely demolish any prologue in the game and start eating up the first basic block in like 90% of scenarios. Could we get the code allocator to allocate regions within 32 bits of each image and allocate all the hooks stuff in those near regions so we could do relative 32-bit jumps? That should be slightly safer in terms of avoiding the first basic block… methinks |
cdetour right now only does this if the trampoline is >2gb away. For a jump > 2gb, it takes a whopping 15 bytes.
As I just explained, the compiler puts the relative instructions at the very beginning of the function so it doesn't matter how small your jump is. Safetyhook already does this and it looks like they are attempting to parse /proc/pid/maps which probably why I noticed it took maybe half a second longer to load my hooks since it's parsing hundreds of lines of text every time a hook is made, but this is preferable to crashing. This is a drop in replacement for CDetour as far as I can tell. Maybe in the future it can be changed to another hooking library or upgraded when safetyhook does full instruction relocation. |
@bottiger1 What compiler and can you provide disassembly? |
I don't know what compiler valve uses. And I already pasted the disasembly |
Like bottiger has pointed out, the crux of the problem isn't what kind of jump is used. As long as
Can't think of a situation on my side where this would be problematic, but I feel like this could be cause for concerns. Should we consider leaving the original detour library as an option, for faster detour setup ? |
It would be way cleaner and easier to just edit safetyhook to skip the attempt to allocate within 2gb on 32 bits than to maintain 2 hooking backends. And I think the slowdown isn't as bad as I've made it seem. I'm probably hooking way more functions than anyone else and it also seems to be implementing a memory allocator itself so it isn't calling mmap every hook.
I don't know what you mean. It has 1 flag on creation to start disabled instead of enabled. |
I sincerely doubt it, unless if you're running full CEntity. There was a request in from asherkin beyond a decade ago for valve to create dead memory (in general, but specific to TF) so we could handle situations like this. Depending on who is left having the simple conversation may still be just that. Again, this is far from a new problem but may be more prevalent now because compilers have gotten far more sophisticated - cstrike had a ton of these on interesting functions. CDetour was a culmination of headers from multiple people into a common header. If there's a manager that's an improvement, it's long been known detours run over eachother. If that can be improved, it's a net win. |
|
I'd agree having a central manager for all extensions would be the best, but I think we should fix what's broken first and then worry about implementing new interfaces later because that is almost certainly going to take a lot longer. |
If there's a manager I think that should be sourcehook given that it already manages vtable hooks. (so off scope this repo anyways)
Ignore me, I was just saying that hoping there were different methods of creating detours (trading away safety) with this library based on your lag observations, so that it would not hang the thread for so long. |
I'm starting a discussion here about replacing CDetour which is currently broken on 64 bits. I'm estimating around 1 out of every 15 functions I've hooked happen to have a relative address at the beginning of the function and CDetour simply copies these instructions without updating the address, resulting in a crash.
I've looked around at different libraries to fix this issue and they all seem to have various shortcomings.
I decided to try polyhook 2 even though Linux support is listed as "Future". After a bit of fixing I have managed to get it to compile and stop crashing.
https://github.com/bottiger1/PolyHook_2_0/tree/linuxfix
If you want to try it yourself, you'll need to set -DPOLYHOOK_DISABLE_IOSTREAM=ON when running cmake.
Possible complaints
I've tested it on one of the functions that CDetour can't hook and it works. I'm proposing we use this as CDetour replacement, unless someone has a better idea.
The text was updated successfully, but these errors were encountered: