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

Default to standard x64 calling convention if none is supplied #3

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

NotAdam
Copy link
Contributor

@NotAdam NotAdam commented Aug 18, 2020

This changes the behaviour around hooks where a default calling convention isn't supplied, though I'm not 100% sure if this is a best-fit fix, but didn't want to also write a bunch of extra code to pass a default through a Hook constructor if it's not necessary.

I figure that this is a reasonable default for x64 given it's not the hellscape that x86 is so a default option won't cause any unexpected breakages (or unlikely to), but still allows for the previous behaviour. x86 has been left as is given the aformentioned hellscape that is x86 calling conventions.

If you'd prefer this to be done via a constructor overload (or something else) I can move things around.

@NotAdam NotAdam changed the title Default to standard calling conventions if none is supplied Default to standard x64 calling convention if none is supplied Aug 18, 2020
@Sewer56
Copy link
Member

Sewer56 commented Aug 19, 2020

I figure that this is a reasonable default for x64 given it's not the hellscape that x86 is so a default option won't cause any unexpected breakages (or unlikely to), but still allows for the previous behaviour.

Sure works for me no problem.
I don't work with x64 applications very often but from experience the default convention is used very often anyway in compiled programs; as it already does register parameter passing.

@Sewer56 Sewer56 merged commit 26cf3c3 into Reloaded-Project:master Aug 19, 2020
@NotAdam
Copy link
Contributor Author

NotAdam commented Aug 19, 2020

I don't work with x64 applications very often but from experience the default convention is used very often anyway in compiled programs; as it already does register parameter passing.

Yeah, that's why I figured this would be a good way of fixing this particular thing so I don't break a bunch of third party code in the process of migrating to this lib.

Thanks for the merge. When you get a chance would you be able to publish a new nuget package?

@Sewer56
Copy link
Member

Sewer56 commented Aug 19, 2020

My current plan is to update immediately after the first .NET 5 preview that implements UnmanagedCallersOnly attribute, since I added function pointer support in the net5 branch: https://github.com/Reloaded-Project/Reloaded.Hooks/tree/net5

It seems that at the current moment, this attribute is ignored and all functions are treated as stdcall under x86 as opposed to cdecl, which is what this library creates wrappers for.

x64 is working fine though obviously due to default convention.

@Sewer56
Copy link
Member

Sewer56 commented Aug 19, 2020

Forgot to mention.

Sadly I wouldn't get much use out of function pointer support myself though, because of dotnet/runtime#39176 ; which makes me unable to release Reloaded-II under .NET 5, even though everything is working fine on my end. ¯\(ツ)

@NotAdam
Copy link
Contributor Author

NotAdam commented Aug 19, 2020

I did see the .NET 5 stuff, there's a few things coming soon that look pretty good for this kind of thing.

That's an interesting issue though, because we're looking at moving to .net 5 (and ditching EasyHook) and that bug repro is how I was looking to implement it which is slightly problematic. Likely we'll be stuck on framework until that's fixed then, thanks for the heads up.

Can probs just submodule for now until then, main thing was ditching EasyHook because it's completely broken when it comes to hooking

@Sewer56
Copy link
Member

Sewer56 commented Aug 19, 2020

For the record, you're still a-ok if you are getting .NET code execution from the target program's primary thread (e.g. hook some function the program will execute down the road). But obviously, that's no good, as depending on your use case, you often will want to hook something executed at startup.

In the meantime if interested you're still a-ok moving to Core 3.1 though as this is a .NET 5 regression, you shouldn't need to change any code to migrate from 3.1 to 5 if they ever decide to fix it. Well, that is outside of updating the target framework in your .csproj of course. I did upgrade Reloaded-II to .NET 5 after all, but had to downgrade again due to the issue.

@NotAdam
Copy link
Contributor Author

NotAdam commented Aug 19, 2020

That wouldn't really work in our case, ideally we can start the process suspended, then load hostfx and then you're able to hook things pre init. Only real fix I guess would be to hijack the main thread and run our own code first, then set rip back to the game entrypoint but that's pretty gross.

Worst case scenario, that'd work but the less I have to touch the easier it becomes to manage. Will sit on it for now anyway though, appreciate the insight. See what happens when .NET 5 comes out lol

@Sewer56
Copy link
Member

Sewer56 commented Aug 19, 2020

ideally we can start the process suspended, then load hostfx and then you're able to hook things pre init

Reloaded-II does exactly this, so feel free to use it as reference.

Only real fix I guess would be to hijack the main thread and run our own code first,

Yeah hijacking the entry point is possible. Personally I didn't like that idea though, I agree it sounds way too hacky for our own good; especially since my own goal is to support any program too.

See what happens when .NET 5 comes out lol

I bet we'll be stuck waiting in limbo given it was milestoned for 6.0.0; unless somehow you can convince enough people to updoot the issue to encourage the developers working for a certain multi billion dollar corporation.

@Dormanil
Copy link

Looks like they are backporting the fix for the issue you found into .NET 5. dotnet/runtime#44267

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.

3 participants