samuelgr/Xidi

Possible Errors with Xidi

Closed this issue · 9 comments

Polymega commented

Hi Samuel,

Hope things are well! The other week we released an update to the Silent Hill 2: Enhanced Edition project which includes the debut of Xidi with it. We've had some reports of the project being unable to launch due to VC Redist packages needing to be installed, along with others having an "error 0xc0000142" code while Xidi was in the game's directory.

Elisha made a build of Xidi with all dependencies removed but users are still reporting the error mentioned above. May we ask for your thoughts on this matter? Please see the tickets below for some of the user reports:

Thank you!

samuelgr commented

Based on Elisha's latest custom build seeming to help and some testing I did on my own, I think there are a couple of separate issues taking place here at the same time.

It seems like the fix that would work for your users is to disable enhanced processor instructions and change the dependency to use static linking. I already refactored all the compiler settings so that these settings can be changed by modifying Properties/BuildSettings.props, which is much less likely to be modified than the individual Visual Studio project files themselves. The idea is to make it far easier and more maintainable to create a custom build and keep it updated with newer versions of Xidi.

More details below.

  1. External dependency on Visual Studio 2022 runtime

I know that in the past this was fixable by building Xidi with the runtime linked statically. I do not own a Steam Deck myself but I have seen other users saying Xidi works on Steam Deck by installing the runtime and setting it up correctly. A custom build that statically links in Visual Studio runtimes (as Elisha has already been doing) would very likely work too.

  1. Startup error 0xc0000142 and enhanced processor instructions

This particular error code translates to STATUS_DLL_INIT_FAILED and indicates the DLL is crashing during initialization.

I was able to reproduce the 0xc0000142 error by adding this code into DllMain and making a custom build:

*((int*)0) = 55;

This code attempts to write to a null pointer, which causes a crash. I also tried this:

__asm {
   ud2
}

This code causes the compiler to emit an instruction that is invalid on every x86 processor. It also leads to a crash.

Since it seems that removing the enhanced processor instructions compiler setting fixes the problem, probably what is happening is some instruction that was introduced in AVX-generation processors (~2011) happens to be present in my official builds and is getting executed when the application loads Xidi, hence the crash. I gather that this is happening on Windows 11 for ARM, which suggests that whatever instruction happens to be causing this problem is one that the x86-to-ARM translator doesn't currently support.

elishacloud commented

Thanks for looking into this. I think you are right on your comments. Given that this is just a wrapper and it is passing most of the work off to XInput, I am not sure how much is benefited by using enhanced processor instructions? Maybe disabling them would help make Xidi more compatible.

I already refactored all the compiler settings so that these settings can be changed by modifying Properties/BuildSettings.props, which is much less likely to be modified than the individual Visual Studio project files themselves.

Ok, great. That will help in the future. I wonder if you also want to change the projects to use "Multi-Threaded (/MT)"? This way none of the users of Xidi need to worry about installing runtime environments.

samuelgr commented

I will certainly consider turning off the enhanced instructions in a future release. The original rationale for enabling the enhanced processor instructions was to cause the compiler to generate the most efficient code it could for things like bit manipulation, floating-point math (force feedback uses this extensively), and so on without having to worry about the constraints of very old processors. However, given that ARM is as modern as it gets for Windows 11, and the x86 emulator doesn't support those enhanced instructions, I suppose I might need to re-think that line of reasoning. Furthermore, to be fair, I also haven't measured the performance impact of using the enhanced instructions.

As for the use of /MT vs /MD, there are a few reasons why I prefer /MD for the official builds. More on this below.

One option you could consider if you want to avoid having to rebuild Xidi with /MT is to add the individual runtime DLL files it needs to your Xidi installation package and distribute them alongside Xidi. This is presumably as simple as adding a few extra binaries into the package like msvcp140.dll and msvcp140_atomic_wait.dll, all of which are installed along with Visual Studio. (I haven't tested this myself, but I think it might work.) It appears that Microsoft explicitly permits this, and you would be able to distribute the official builds of Xidi without worrying about the external dependency.

Reason 1: Licensing

I have actually tried to figure out if Microsoft allows binaries to be distributed with its copyrighted runtime code embedded within, and I haven't found a clear answer.

Reason 2: Redundancy

This likely doesn't apply to your project, but I fully intend Xidi to be loaded into programs and games alongside other DLLs of mine that also use the Visual C++ runtime. This already happens whenever the HookModule method is used to force a game to load Xidi because Hookshot also uses the Visual C++ runtime, and I have a few future projects in mind that would add even more such loaded modules. There is no good reason to have multiple complete copies of the same runtime loaded into memory - it just seems wasteful of resources.

Reason 3: Update Management

Users of Xidi, myself included, probably have multiple games installed on their machines all of which use Xidi in one way or another. Let's suppose there is a security vulnerability or other important update Microsoft releases to fix issues with their Visual C++ runtime. If I were to use /MT, I would need to rebuild Xidi, and users would subsequently need to replace all of their installed instances with the updated version. On the other hand, if I continue using /MD, I can just rely on Microsoft to push out an update without the users needing to do anything.

  • 👍1
MarioTainaka commented

I will certainly consider turning off the enhanced instructions in a future release. The original rationale for enabling the enhanced processor instructions was to cause the compiler to generate the most efficient code it could for things like bit manipulation, floating-point math (force feedback uses this extensively), and so on without having to worry about the constraints of very old processors. However, given that ARM is as modern as it gets for Windows 11, and the x86 emulator doesn't support those enhanced instructions, I suppose I might need to re-think that line of reasoning. Furthermore, to be fair, I also haven't measured the performance impact of using the enhanced instructions.

As for the use of /MT vs /MD, there are a few reasons why I prefer /MD for the official builds. More on this below.

One option you could consider if you want to avoid having to rebuild Xidi with /MT is to add the individual runtime DLL files it needs to your Xidi installation package and distribute them alongside Xidi. This is presumably as simple as adding a few extra binaries into the package like msvcp140.dll and msvcp140_atomic_wait.dll, all of which are installed along with Visual Studio. (I haven't tested this myself, but I think it might work.) It appears that Microsoft explicitly permits this, and you would be able to distribute the official builds of Xidi without worrying about the external dependency.

Reason 1: Licensing

I have actually tried to figure out if Microsoft allows binaries to be distributed with its copyrighted runtime code embedded within, and I haven't found a clear answer.

Reason 2: Redundancy

This likely doesn't apply to your project, but I fully intend Xidi to be loaded into programs and games alongside other DLLs of mine that also use the Visual C++ runtime. This already happens whenever the HookModule method is used to force a game to load Xidi because Hookshot also uses the Visual C++ runtime, and I have a few future projects in mind that would add even more such loaded modules. There is no good reason to have multiple complete copies of the same runtime loaded into memory - it just seems wasteful of resources.

Reason 3: Update Management

Users of Xidi, myself included, probably have multiple games installed on their machines all of which use Xidi in one way or another. Let's suppose there is a security vulnerability or other important update Microsoft releases to fix issues with their Visual C++ runtime. If I were to use /MT, I would need to rebuild Xidi, and users would subsequently need to replace all of their installed instances with the updated version. On the other hand, if I continue using /MD, I can just rely on Microsoft to push out an update without the users needing to do anything.

Imo I think the installer should just do an OS check and install both X64 and X86 redists and call it a day.

elishacloud commented

I will certainly consider turning off the enhanced instructions in a future release.

Ok, great. Thanks!

This is presumably as simple as adding a few extra binaries into the package like msvcp140.dll and msvcp140_atomic_wait.dll, all of which are installed along with Visual Studio.

The issue with just including these dlls is that when you open the file you can see other dll files besides just these ones that are statically linked that may cause other issues. In other words, we may not always be able to just add those few dlls and be done with it. See graphic below when compiling with /MD.

image

In my experience, relying on all these other dlls has a chance of causing issues if one of those dlls has an issue, or is missing, which I have seen. All of these dlls tend to be flaky, in my experience, in Windows. Having less things you need to rely on is better, in my opinion.

However, all of these are gone and only a few core Windows dll files are linked when you compile with /MT so there is a much better chance of correct functionality.

I have actually tried to figure out if Microsoft allows binaries to be distributed with its copyrighted runtime code embedded within, and I haven't found a clear answer.

Based on the documentation (here, here and here) it is pretty clear, to me at least, that we can distribute these files as we chose, including embedding them in the code.

There is no good reason to have multiple complete copies of the same runtime loaded into memory - it just seems wasteful of resources.

Given how small these embedded files are and how much memory modern computers have, I doubt this is an issue. However, besides all that, only a part of the dlls that you embed are included, not the whole dll.

When you compile with /MT it only increases the size of Xidi by about 120 KBs. However, msvcp140.dll is 437 KBs by itself. And then msvcp140_atomic_wait.dll is another 46 KBs. It would take four other dlls with embedded runtime files to just equal these two, and that does not include all the other dlls that need to be loaded that I showed you above.

In that vast majority of cases embedding the files into the dll will decrease the memory usage, not increase it.

Users of Xidi, myself included, probably have multiple games installed on their machines all of which use Xidi in one way or another. Let's suppose there is a security vulnerability or other important update Microsoft releases to fix issues with their Visual C++ runtime.

Keep in mind that there could be a vulnerability in Xidi that would cause a user to update all their instances, so that may require users to update all their copies of Xidi regardless of the runtimes. Also, loading the full runtime increases the risk since all the functions are there for an attacker to use.

However, when you embed the runtime into Xidi then it reduces the risk in two ways:

  1. The embedded functions are in a different location for different programs. For instance, they are in a different location in Xidi then they are in the sh2-enhce project I'm working. This makes it much harder for an attacker to use since they would likely need to make a specific attack for Xidi and then a different specific attack for sh2-enhce, rather than just a generic attack on the runtime.
  2. Not all of the runtime functions are embedded so there is a good chance that the vulnerable ones won't even be included so there is no vulnerability at all in many cases.

Anyways, this whole discussion is a philosophical discussion. For me, I would rather increase the chance of my code working and decrease the amount of work that my users need to do to get the software working by embedding the runtime into the project. And since I don't really see much negatives to it, only positives, that is the solution I think is best.

samuelgr commented

I definitely agree that much of the discussion here is philosophical. Our projects have different needs and we both have differing views on the subject, so I'll start with the practical before moving onto the more philosophical.

Practical

While I do acknowledge and respect that your use case might call for /MT, I don't intend to change Xidi's official builds to /MT from /MD. Our differing views on this subject is actually one of the main motivations for refactoring the build settings into a separate file that is unlikely to be changed. That way, if you decide to maintain a fork that uses /MT, your maintenance burden will be almost nothing.

Philosophical

The suggestion I made last time regarding distributing the runtime DLLs alongside Xidi falls under the local deployment method, which Microsoft lists in one of the links you shared but doesn't recommend over using the redist installer. It would probably work, though I haven't tested it myself.

The issue with just including these dlls is that when you open the file you can see other dll files besides just these ones that are statically linked that may cause other issues.

DLLs of the form "api-ms-win-*.dll" are actually just "umbrella libraries" that are not expected to exist as literal files. This falls under the Windows feature known as "api sets." As an example, based on this feature, Xidi's hook module for WinMM checks for the joystick API set rather than winmm.dll explicitly. If it gets a handle back and you were to call GetModuleFileName() on that handle, chances are you would get back the path to the system's winmm.dll file.

I would rather increase the chance of my code working and decrease the amount of work that my users need to do to get the software working by embedding the runtime into the project.

I haven't heard of any issues from the users of Xidi that installing the runtime library redist has been an issue or even an annoyance. For as long as Xidi has been released it has been a documented requirement, and users both on and off of Github are very familiar with the fact that they need to do the installation first. Users have even published guides on how to get the redist working on Steam Deck running Linux.

Other applications that are distributed via installers often will install the redist silently by running the redist installer executable with the right command-line parameters, which requires no work on the part of users. It happens in the background (maybe with a UAC prompt if that's needed) and then it's done.

All of the above together tells me that installing the redist is an easy thing for users to do, and it's a once-and-done operation. Furthermore, Microsoft itself actually recommends linking dynamically with their runtime instead of statically. This is explicitly stated in one of the links you shared.

I'll revisit the remaining discussion points in the same order as with the last post: licensing, redundancy, and update management.

Licensing

I read through the three links you shared, and they all talk about distributing the DLLs or the redist installer EXEs unmodified. I haven't seen anything explicit about embedding bits and pieces of their copyrighted binary code into a DLL and distributing that. Static linking isn't just a matter of embedding an entire DLL file, it means the linker picks and chooses which bits of their binary to include, which one could argue counts as a "modification."

In any case I tend to be very risk-averse when it comes to licensing. If I'm not explicitly told I can do something with somebody else's code then I assume I can't do it. That's just my own personal view on the subject, though. Maybe Microsoft is totally okay with their runtime code being statically linked and distributed as such.

Redundancy

Given how small these embedded files are and how much memory modern computers have, I doubt this is an issue.

...

In that vast majority of cases embedding the files into the dll will decrease the memory usage, not increase it.

I recognize that the total binary size growth is quite small when switching to /MT, especially compared to the size of the DLLs themselves. But when I say "redundancy" I am not concerned so much with binary size as I am with runtime behavior.

Let's think back to the use case I mentioned earlier of having 5 DLLs loaded into the same process, all linked with the same Visual C++ runtime library. This is a realistic use case given some of the ideas I have for future projects that I intend to work together.

If we link with /MT then each DLL has to do a separate runtime initialization of all of the common runtime features. Furthermore, since memory allocation and even the entire heap and its associated management is implemented in the runtime, we would get at least 5 separately-managed heaps being initialized and managed, one for each DLL. (Operators new and delete, smart pointers like std::unique_ptr, and so on all make use of the runtime's heap memory management by default.) While it is true that each DLL will only contain the exact parts of the runtime that it needs, there is still the runtime core that every binary needs.

If we link with /MD then we do all of the above steps once, and we also guarantee that all of the loaded DLLs get the exact same version of the runtime. We only have one heap, we only need one copy of whatever common data structures the runtime needs to support its functionality, and we do all of the initialization only once.

Update Management

Keep in mind that there could be a vulnerability in Xidi that would cause a user to update all their instances, so that may require users to update all their copies of Xidi regardless of the runtimes.

True, and an update will be required no matter what if there is a security vulnerability in a project or even just a new feature that users want to use. But we need to consider which code is more likely to have a bug, and what the possible severity of that bug might be.

Xidi is completely implemented in user-mode using documented standard interfaces. It is a relatively small project that serves one specific, relatively niche, purpose.

The Visual C++ runtime, on the other hand, implements an entire general-purpose standard that supports applications running in a wide variety of environments, including production. It has a much larger surface area and orders of magnitude more users. Furthermore, Microsoft has complete freedom to implement it as they see fit, using interfaces and techniques that I would never dream of using because they aren't necessarily publicly available. (I'm not saying they do this, but they could.)

Also, loading the full runtime increases the risk since all the functions are there for an attacker to use.

Also true, but there is still the runtime core that gets included with statically-linked modules. Furthermore, I actually have no idea which parts of the runtime the linker chooses to include in my binary. If Xidi were programmed to use a particular part of the runtime, Microsoft's implementation could very well include a dependency on another part of the runtime, and short of doing a lot of digging into binary dumps, I would have no easy way of being able to tell. In other words, it is not immediately obvious which parts of the runtime get included in a statically-linked binary and which are left out. The problem is I would have to know that in order to determine whether or not an update to the runtime necessitates an update to Xidi. This is not true if I keep using /MD - Microsoft takes care of all the updates, and neither I nor the users of Xidi have to worry about it at all.

elishacloud commented

Thanks for the comments @samuelgr. I respect your thoughts and what you're doing for your project is probably the right thing! 👍

Another way to solve this is for us to include Xidi project as a submodule of sh2-enhce. This would eliminate all the issues here, as I would compile Xidi inside sh2-enhce like I do with d3d8to9 and other modules.

Anyways, just a couple of random thoughts below:

Our differing views on this subject is actually one of the main motivations for refactoring the build settings into a separate file that is unlikely to be changed. That way, if you decide to maintain a fork that uses /MT, your maintenance burden will be almost nothing.

Yes, I like this idea and it works for us if we decided to recompile the build each new release.

I haven't heard of any issues from the users of Xidi that installing the runtime library redist has been an issue or even an annoyance.

I believe the users for sh2-enhce are different than the users for Xidi. Xidi is already a technical product for a technical user. sh2-enhce is for any user, even non-technical ones. We have a lot of issues with users installing (or not installing the correct runtimes). We want to reduce the number of trouble tickets we have here.

Other applications that are distributed via installers often will install the redist silently by running the redist installer executable with the right command-line parameters

Frankly, I don't like doing this because it seems like one more moving part to the install that can go wrong (and I have seen the runtime fail to install before). I like the KISS principle. Besides all that, it causes issues with uninstall. How do you know when to uninstall it? If you remove it and other programs are relying on it then you break those. If you don't remove it then you break the main thesis of uninstall, which is to remove everything that you added. I really don't like leaving things around on my user's computers.

The suggestion I made last time regarding distributing the runtime DLLs alongside Xidi falls under the local deployment method .... It would probably work, though I haven't tested it myself.

Yes, maybe this is the way to go. If we can only include a couple of extra files for the runtimes then it solves some of the install and uninstall issues. I was just concerned if this would work or if there are other dependencies besides just the runtimes. I need to test this more, but it may save me from having to recompile Xidi for each release.

Also true, but there is still the runtime core that gets included with statically-linked modules. Furthermore, I actually have no idea which parts of the runtime the linker chooses to include in my binary.

That is kind of the point. Unless there is a specific attack on Xidi then there is no need for an update, generic attacks on the runtime won't work against Xidi if the runtime is embedded.

I read through the three links you shared, and they all talk about distributing the DLLs or the redist installer EXEs unmodified.

  1. You're not modifying the exe files or any of the dll files.
  2. Nowhere in the licensing does it say you cannot include these embedded. The licensing is what restricts you and if it doesn't restrict you then it doesn't restrict you.
  3. This is a built-in option that Microsoft gives for released builds from the very tool that they created. This article here even tells you how to build "a release version of your project" by "static linking" the runtimes using the "/MT" command. If they didn't want you to release code using this option then why did they give you the ability to do this with release builds?

There is no way it would stand up in court that you did anything wrong if you embedded the functionality with the /MT command.

Edit: for the redundancy, we would need to run actual tests and monitor the memory use with and without embedding the code to see which is smaller or larger. However, since the code is coming from a common source (i.e. the runtimes), the only duplication issue we would need to think about is disk space and memory. The disk space is so small it is not worth talking about.

As far as memory, I very much doubt that all the heap allocations and all the duplicate stuff from embedding these would take more space than the ~500KBs used by the full runtime dlls, since the full runtime dlls would need to be loaded into memory if you don't embed them. I would guess that you would need to load 3 or 4 modules with the files embedded before you would equal the size of the full runtime dlls being loaded. So unless the user has lots of different modules with the files embedded loaded into the same process I still believe that it would be less memory usage by embedding them.

Anyways, the amount of memory we are talking about here is really not worth this whole discussion since it is not even in the MB range and computers today have GBs of RAM. As far as I can see, there is no practical concern with duplicating the runtimes by embedding them individually.

samuelgr commented

With commit 0ad5223 I've removed the enhanced processor instructions from the build settings.