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

Fix skybox_swapper by exposing R_LoadNamedSkys to sv_skyname #382

Closed

Conversation

Nbc66
Copy link

@Nbc66 Nbc66 commented Feb 6, 2025

This exposes the R_LoadNamedSkys by signature matching the address of R_LoadNamedSkys from engine.dll
and installs this as a callback func to sv_skyname

This is how it looks:

8mb.video-PPO-F3RFK0Tl.mp4

Note: This is signature matching for SP only right now

So if in the future you decide to switch to MP I'll update the signature for MP so it can work on the MP branch as well


Does this PR close any issues?

PR Checklist

  • My PR follows all guidelines in the CONTRIBUTING.md file
  • My PR targets a develop branch OR targets another branch with a specific goal in mind

@Nbc66
Copy link
Author

Nbc66 commented Feb 6, 2025

Note about linux:

Linux ofc has a different signature I don't really work with linux but I extracted the signature from ghidra from engine.so

so if anybody knows how to do this on linux pls write it here

the linux signature: 55 89 E5 57 56 8D BD A4 FE FF FF 31 F6 53 81 EC 8C 01 00 00 C7 45 C8 ? ? ? ? A1 ? ? ? ? C7 45 CC ? ? ? ? C7 45 D0 ? ? ? ? C7 45 D4 ? ? ? ? C7 45 D8 ? ? ? ? 8B 10 C7 45 DC ? ? ? ?

@Blixibon
Copy link
Member

Blixibon commented Feb 7, 2025

Allowing sv_skyname to be changed on the fly (and therefore fixing skybox_swapper) would be a huge deal, but patching the engine through signature matching is controversial and a number of users have voiced concern against it in the past.

Since this directly reinterprets memory from another binary, it would also be difficult for me (or any other reviewer) to be able to predict what this change can and will do, as it is based on an address in the engine rather than any visible code. I must assume that this is referring to the correct function in the correct version of the binary, and I must also assume this method will not cause any issues in the future. The fact it would require further changes to support different branches and platforms is also not ideal. Overall, I do not feel like I can approve this in good conscience.

For the record, you are not in trouble for proposing this change. There was no official rule or policy put in place against signature matching, although I will probably be adding one now so that we have a clear stance on this going forward.

@Blixibon Blixibon closed this Feb 7, 2025
@Nbc66
Copy link
Author

Nbc66 commented Feb 7, 2025

Allowing sv_skyname to be changed on the fly (and therefore fixing skybox_swapper) would be a huge deal, but patching the engine through signature matching is controversial and a number of users have voiced concern against it in the past.

so this doesn't patch the engine.dll this just gets the memory address of the function to be able to use it. this doesn't rewrite anything memory wise all this is, is a direct call to the function this is the same thing as calling a function the only issue with it is that this function changes from sp and mp because these are compiled binary every time they compile the signature changes

as in every time the engine.dll or engine.so is compiled the signature would change

@Nbc66
Copy link
Author

Nbc66 commented Feb 7, 2025

Since this directly reinterprets memory from another binary, it would also be difficult for me (or any other reviewer) to be able to predict what this change can and will do, as it is based on an address in the engine rather than any visible code. I must assume that this is referring to the correct function in the correct version of the binary, and I must also assume this method will not cause any issues in the future. The fact it would require further changes to support different branches and platforms is also not ideal. Overall, I do not feel like I can approve this in good conscience.

there wouldn't be really any other way besides just changing the engine using leaked code

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.

2 participants