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

Improve replacing xinput1_3 with xinput9_1 #583

Merged
merged 3 commits into from
Nov 22, 2023
Merged

Improve replacing xinput1_3 with xinput9_1 #583

merged 3 commits into from
Nov 22, 2023

Conversation

Jan200101
Copy link
Contributor

The previous logic compared the whole argument, which may be a path, to a string literal.
This checks if the argument ends with the string literal instead.

The previous logic incorrectly loaded compared the whole argument, which may be a path, to the string literal.
This fix checks if the argument ends with the string literal instead
Copy link
Contributor

@RoyalBlue1 RoyalBlue1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. This seems to be a bit unnecessary since the dll is loaded from the game and the string will never change, but the readability changes are quite nice

@GeckoEidechse GeckoEidechse changed the title improve replacing xinput1_3 with xinput9_1 improve replacing xinput1_3 with xinput9_1 Oct 19, 2023
@GeckoEidechse GeckoEidechse added needs testing Changes from the PR still need to be tested almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Oct 19, 2023
@p0358
Copy link
Contributor

p0358 commented Oct 22, 2023

But I still believe that overall it'd be better to replace the whole LoadLibrary fragment with patching the source string, but welp. I guess an advantage here is that a hook already exists anyway

@Jan200101
Copy link
Contributor Author

Jan200101 commented Oct 23, 2023

@p0358 would this work for you?

{
	HMODULE moduleAddress;

	LPCSTR lpLibFileNameEnd = lpLibFileName + strlen(lpLibFileName);
	LPCSTR lpLibName = lpLibFileNameEnd - strlen(XINPUT1_3_DLL);

	// replace xinput dll with one that has ASLR
	if (lpLibFileName <= lpLibName && !strncmp(lpLibName, XINPUT1_3_DLL, strlen(XINPUT1_3_DLL) + 1))
	{
		lpLibFileName = "XInput9_1_0.dll";
	}

	moduleAddress = _LoadLibraryExA(lpLibFileName, hFile, dwFlags);

	if (moduleAddress)
		CallLoadLibraryACallbacks(lpLibFileName, moduleAddress);

	return moduleAddress;
}

It would also fix the issue of xinput 1_3 callbacks being called, but 9_1 being passed.

@p0358
Copy link
Contributor

p0358 commented Oct 23, 2023

It would also fix the issue of xinput 1_3 callbacks being called, but 9_1 being passed.

Yeah it's good for that thing, though it didn't address either of my earlier comments xd But it turns out strlen on static string does get optimized with /O2, so I guess only the first one would stand (and it doesn't with a const variable so eh). So I guess it either has to stay a macro or be replaced to the literal in all cases

Idk then, someone else should also share their opinion on whether to keep the macro, move it inside of the func or replace with three string literals

Anyway I guess the rest is fine. You do get rid of the error on missing dll now, but it wasn't a fatal failure originally to begin with, and the DLL is always there on Vista+ under normal circumstances, so that also should be just fine

@GeckoEidechse
Copy link
Member

Is there a good way to test this. Like how can I see what DLLs are loaded to verify that this works correctly? ^^"

@Jan200101
Copy link
Contributor Author

Is there a good way to test this. Like how can I see what DLLs are loaded to verify that this works correctly? ^^"

Use a controller, I guess?
Process Monitor should be able to show you what DLLs are loaded.
Alternatively, x64dbg

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So based on @Jan200101's comment I found ListDLLs and used that

Before PR:

NorthstarLauncher.exe pid: 22440
Command line: "C:\Program Files (x86)\Steam\steamapps\common\Titanfall2/NorthstarLauncher.exe"  -profile=R2Northstar

Base                Size      Path
0x00000000e70d0000  0x185000  C:\Program Files (x86)\Steam\steamapps\common\Titanfall2\NorthstarLauncher.exe
...
0x00000000a3f50000  0x7000    C:\Windows\SYSTEM32\XInput9_1_0.dll
0x00000000a7770000  0x11000   C:\Windows\SYSTEM32\XInput1_4.dll
...

With PR:

NorthstarLauncher.exe pid: 18568
Command line: "C:\Program Files (x86)\Steam\steamapps\common\Titanfall2/NorthstarLauncher.exe"  -profile=R2Northstar

Base                Size      Path
0x0000000067670000  0x184000  C:\Program Files (x86)\Steam\steamapps\common\Titanfall2\NorthstarLauncher.exe
...
0x00000000a3f50000  0x7000    C:\Windows\SYSTEM32\XInput9_1_0.dll
0x00000000a7770000  0x11000   C:\Windows\SYSTEM32\XInput1_4.dll
...

No clue why xinput1_4 is in there but the behaviour with the PR didn't change so I consider this tested successfully ^^

@GeckoEidechse GeckoEidechse changed the title improve replacing xinput1_3 with xinput9_1 Improve replacing xinput1_3 with xinput9_1 Nov 22, 2023
@GeckoEidechse GeckoEidechse merged commit 90e0376 into R2Northstar:main Nov 22, 2023
2 checks passed
@Jan200101 Jan200101 deleted the PR/xinput_ov branch June 24, 2024 12:42
@Jan200101 Jan200101 removed needs testing Changes from the PR still need to be tested almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Jun 24, 2024
ASpoonPlaysGames added a commit to ASpoonPlaysGames/NorthstarLauncher that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants