Closed DarkStarSword closed 6 years ago
Bisecting tracked it down to this:
FAIL: 1605d13 Add more platform update handling. COMPILE ERROR: 4834885 Update IDXGIDevice1 handling for Batman: telltale. PASS: 242dd9c Add some object version detail.
Most likely to be the HackerContext::QueryInterface change. IIRC, you already found that MGSV is sensitive to having C++ objects returned instead of the D3D11 COM objects. Assuming it still fails with platform_update=1. (Otherwise E_NOINTERFACe is returned.)
Please feel free to revert any of this platform_update code. It didn't work to solve the problem for Dishonored2 or Batman Telltale. Both still crash at launch.
I'm not super worried about this - I have no pressing need to update the 3DMigoto used in that fix (though of course a user might try and run into this), and was more just making a note of it to investigate later. The only reason I even tested this was to see if the Win 10 creators update broke anything in the hooking implementation (which fortunately it seems that it has not).
Also, my feeling is that getting the platform update working is probably going to be more important in the long run than fixing this regression and I wouldn't want to revert efforts towards that goal unless we had a pressing need to do so. Once the platform update is working, then we can come back to look at this.
That sounds right. I've let Paul Dusler know that he should not update these two as part of Fix Manager.
If I'm right about that QueryInterface, when the time comes, we'll probably have to make a special workaround for MGSV and Ground Zero.
Seems like it's getting sort of multi-level for this with different workarounds for calls. We'll have to consider if it makes more sense to make game-specific hacks/ifs/workarounds. Not too likely that things like SKIP_DXGI_DEVICE will be used in other games, but I don't have a good intuition here for what might be best.
Since this is related to a problem with platform_update, I wanted to add some background. And write down some research I've done.
After looking at COM interfaces and how they are implemented in much greater detail, I think I now know why the platform_update fails.
The problem stems from our use of the inheritance hierarchy. This cannot work. The confusing part is that it can work for a single level of an object, so when we subclass ID3D11Device as HackerDevice, that works because we use the composition approach to wrapping that ID3D11Device and always call out to the original. However, the only reason this works, is because we are declared as class HackerDevice : public ID3D11Device
and that ID3D11Device interface sets up the exact right vtable, which we replace every entry with our subclass methods, including boilerplate ones we don't care about.
The reason that works is because the vtable is identical in terms of the methods defined, the same order, the same parameters, so when we pass back our HackerDevice to the game and DX11, it still works, because they just jump to a vtable offset that is still correct.
Now, for platform_update, we need to also support ID3D11Device1. That is a Interface subclass as ID3D11Device1 : public ID3D11Device
My theoretical approach was to do a similar subclass for class HackerDevice1 : public HackerDevice
I think I can explain the crashes now, because this cannot work. The problem is that the vtable has been modified. It's not a C++ vs COM problem, it's that we modified the layout of the vtable that the game and DX11 is expecting, because we created other methods in our HackerDevice, which are tacked to the end of the vtable... before we do the HackerDevice1 subclass.
If this is the correct analysis, that means that when we return a HackerDevice1 object, that the vtable is broken for all calls in the Device1 object. Like GetImmediateContext1. If the game calls that, they'll get something out of HackerDevice instead, like ReplaceShader, or maybe the constructor.
It's not possible to do a second level of inheritance here, without breaking the Device1 COM interface specified in the vtable. In order to create a viable HackerDevice1, we need the vtable to be identical up to at least the end of ID3D11Device1 interface.
It's worth noting that the same problem exists in our DXGI layout, and this is no doubt why we get spurious crashes in some games that can be worked around with skip_dxgi.
Not sure what the best solution will be. Hooking is possible, but requires double the boilerplate for the Device1 and Context1 objects. Can also use composition/Has-a HackerDevice if we define HackerDevice1 : public ID3D11Device1
and implement every virtual routine. Again, massive boilerplate. Maybe there is a multiple inheritance solution that puts a secondary vtable below the primary one.
This will require further thought, and suggestions are welcome.
Here is some data, as part of my investigation, that seems worth keeping for notes. This is when TheDivision is creating their SwapChain, which works fine on Win7, crashed on Win10.
This is the crash case.
Device = class HackerDevice@000001CB805B44A0
SwapChain = 0000024BBEEA0120
Description = 000000572A4FED30
Got resolution from swap chain: 0x0
Windowed = 1
Width = 0
Height = 0
Refresh rate = 84.904999
HackerDevice::GetHackerContext returns 000001CB80B801B0
Overlay::Overlay created for 000001CB805B5770: class HackerDXGISwapChain
on HackerDevice: 000001CB805B44A0, HackerContext: 000001CB80B801B0
HackerDXGISwapChain::GetDesc(class HackerDXGISwapChain@000001CB805B5770) called
returns Windowed = 1
returns Width = 1920
returns Height = 1080
returns Refresh rate = 84.904999
returns result = 0
Hooked_LoadLibraryExW load: combase.dll.
Hooked_LoadLibraryExW load: advapi32.dll.
Hooked_LoadLibraryExW load: G:\uPlay\Tom Clancy's The Division\d3d11.dll.
Hooked_LoadLibraryExW load: G:\uPlay\Tom Clancy's The Division\d3d11.dll.
->HackerDXGISwapChain 000001CB805B5770 created to wrap 000001CB81ED0430
->return value = 0
HackerDXGIFactory::MakeWindowAssociation(class HackerDXGIFactory1@000001CB808B7FC0) called with WindowHandle = 0000000000170592, Flags = 1
Flags = DXGI_MWA_NO_WINDOW_CHANGES(no monitoring)
returns result = 0
HackerUnknown::QueryInterface(class HackerDXGISwapChain@000001CB805B5770) called with IID: {A8BE2AC4-199F-4946-B331-79599FB98DE7} // IDXGISwapChain2
return HackerUnknown(class HackerDXGISwapChain@000001CB805B5770) wrapper of 000001CB81ED0430
HackerUnknown::QueryInterface(class HackerDXGISwapChain@000001CB805B5770) called with IID: {94D99BDB-F1F8-4AB0-B236-7DA0170EDAB1} // IDXGISwapChain3
return HackerUnknown(class HackerDXGISwapChain@000001CB805B5770) wrapper of 000001CB81ED0430
HackerDXGISwapChain::GetContainingOutput(class HackerDXGISwapChain@000001CB805B5770) called
created HackerDXGIOutput wrapper = 000001CB81EEC990 of 000001CB81819F70
returns result = 0
HackerDXGIOutput::GetDesc(class HackerDXGIOutput@000001CB81EEC990) called
returned \\.\DISPLAY1, desktop=1
HackerDevice::QueryInterface(class HackerDevice@000001CB805B44A0) called with IID: IDXGIDevice
created HackerDXGIDevice(class HackerDXGIDevice1@000001CB817ED110) wrapper of 000001CB80B519C8
returns result = 0 for 000001CB817ED110
HackerDXGIDevice::GetAdapter(class HackerDXGIDevice1@000001CB817ED110) called with: 000000572A4FED10
created HackerDXGIAdapter wrapper = 000001CB81EEC750 of 000001CB80620970
returns result = 0
HackerDXGIAdapter::EnumOutputs(class HackerDXGIAdapter@000001CB81EEC750) called: output #0 requested
returns result = 0, handle = 000001CB8181D0F0
HackerUnknown::Release(class HackerDXGIOutput@000001CB81EEC990), counter=0, this=000001CB81EEC990
counter=0, this=000001CB81EEC990, deleting self.
HackerUnknown::Release(class HackerDXGIAdapter@000001CB81EEC750), counter=9, this=000001CB81EEC750
HackerUnknown::Release(class HackerDXGIDevice1@000001CB817ED110), counter=50, this=000001CB817ED110
HackerUpscalingDXGISwapChain::GetDesc(class HackerDXGISwapChain@000001CB805B5770) called
It's worth noting the very last line of the file before it dies- HackerUpscalingDXGISwapChain. Not enabled, not created earlier, should not have been called. This looks like a jump to a wrong part of the vtable.
The earlier calls to IDXGISwapChain3 returned 'this' as part of the HackerUnknown test. It is the same object as far as COM is concerned, but it's not the same object from the vtable perspective, we have a whole chain of items from HackerDXGISwapChain->HackerDXGIDeviceSubObject->HackerDXGIObject->HackerUnknown->IUnknown
With added routines on at least the HackerDXGISwapChain, so that any use of IDXGISwapChain3 will have a method mismatch.
Can also use composition/Has-a HackerDevice if we define HackerDevice1 : public ID3D11Device1 and implement every virtual routine.
I think the best approach will end up being to drop HackerDevice as a separate class altogether and only have a HackerDevice1 inheriting from ID3D11Device1. This would contain an ID3D11Device1 pointer to wrap the original DirectX object, but without the platform update that may actually point to a regular ID3D11Device (we would therefore have to be careful we don't call any Device1 specific methods from 3DMigoto outside of the wrapped Device1 methods without testing which kind of device we are wrapping).
This way the vtable should be compatible for all games whether or not they are using the platform update, because any extra things we add will end up after the Device1 part of the vtable. This should also work with games that don't use the platform update since a Device1's vtable includes the regular Device's vtable in the right spot, and it won't matter that there are extra Device1 vtable entries after that since the game won't be touching them.
This can probably be generalised in that we would always want to implement the highest version of each DirectX object that we are able to support, and ideally that we would return errors if the game performs a QueryInterface for any newer versions that we don't support (hard to do if we don't know the riids of classes that Microsoft hasn't written yet, though maybe we could try whitelisting supported types instead?).
I think the best approach will end up being to drop HackerDevice as a separate class altogether and only have a HackerDevice1 inheriting from ID3D11Device1. This would contain an ID3D11Device1 pointer to wrap the original DirectX object, but without the platform update that may actually point to a regular ID3D11Device (we would therefore have to be careful we don't call any Device1 specific methods from 3DMigoto outside of the wrapped Device1 methods without testing which kind of device we are wrapping)
Cool idea, that sounds right. No point in making a big hierarchy when they are subclasses from the Interfaces.
Does that mean it is time to switch to SDK 10.0? We could then create whatever the current highest object they use today. The only real blocker is the Decompiler. I was thinking it would break with higher than _46.dll, but maybe did not look closely enough.
I'll hack together an experimental version for platform_update level objects using this idea, and see if it helps the Dishonored2 and Batman Telltale problems.
A quick relook at Batman suggests it's going off the rails with our DXGI hierarchy, not the device or context.
Does that mean it is time to switch to SDK 10.0? We could then create whatever the current highest object they use today. The only real blocker is the Decompiler. I was thinking it would break with higher than _46.dll, but maybe did not look closely enough.
Yeah, it might be about time - we should probably bump to 1.3 when we do that, but I'd also like to make sure that we've closed out some of the recent regressions in the 1.2 series so people can keep using that for a while until 1.3 stabilises (if necessary we can always apply more bug fixes to the 1.2 branch, but if they aren't trivial backports it will divide our time). At the very least we should do a new build with the fixes in the top of tree - I can't test the FFXIV fix until the new year, but I'm reasonably confident it should work (which of course means it will probably fail in some other way I haven't thought of ;-), and if you're happy with the fix for The Division we could do a release with those.
I did fix a couple of issues in the decompiler to work with _47, but there was still some work left to do. The next main thing will be to handle a relatively minor change in the resource binding comments - I don't have my notes with me, but IIRC one of the fields changed names, and used to contain just an integer of the slot number, but now contains the HLSL register instead (so, 125 became t125, 13 became s13, etc).
Beyond that we should do some regression testing, using cmd_Decompiler built from both branches to compare the decompiler output for shaders from a variety of games. I have a vague feeling I might have hit some other difference at some point, but I'm afraid I don't recall exactly what that was.
Soliciting your opinion on possible hooking changes.
I've successfully created an experimental version in the branch: https://github.com/bo3b/3Dmigoto/tree/exp_dxgi_hook_only
This does a hook-only version of getting at the Present call, and I moved the RunFrameActions and other dependencies into the HookedDXGI.cpp file. Since we already have a stack of globals, I moved the HackerDevice, HackerContext, Overlay, and IDXGISwapChain references we need into globals.
This runs, and Overlay and hunting both work as before. It breaks Upscaling and ForceDisplayParams for the time being.
My question is whether we would be better served with a single layer wrap of the swap chain, or whether we should hook the calls we need.
We could have a single layer HackerDXGISwapChain that is closer in principle to the older code, at the expense of needing to flesh out the entire HackerDXGISwapChain->IDXGISwapChain->HackerDXGIDeviceSubObject->HackerDXGIObject->IUnknown chain. And/or doing that for IDXGISwapChain1, and making sure we don't hit any SwapChain1 calls in the non-platform-update case.
Or we could have straight hooks for each of the calls we need, with no intervening object references. So for Upscaling, we'd hook IDXGISwapChain->GetBuffer, ResizeTarget, ResizeBuffers and so on. We would also hook CreateSwapChain/CreateSwapChain1/CreatSwapChainHwnd to reimplement ForceDisplayParams.
At the moment, I'm not sure if hooking-only would be more or less reliable, I need to experiment with other games. And will report back. Particularly ones where we've setup skipdxgi*, which would no longer be necessary.
Using a strictly hooking approach for DXGI seems to be working pretty well. I've setup the code and fixed a couple of gltches and tried it on multiple games, including some where we had funny DXGI problems in the past.
Works correctly, including fixing effects, key overrides, hunting and overlay:
xcopy /C "%1*.*" "G:\Games\INSIDE\" /F /Y /S
xcopy /C "%1*.*" "G:\Games\The Witcher 3 Wild Hunt\bin\x64\" /F /Y /S
xcopy /C "%1*.*" "G:\uPlay\Tom Clancy's The Division\" /F /Y /S
xcopy /C "%1*.*" "G:\SteamLibrary\SteamApps\common\Metal Gear Solid Ground Zeroes\" /F /Y /S
xcopy /C "%1*.*" "G:\SteamLibrary\SteamApps\common\Just Cause 3\" /F /Y /S
xcopy /C "%1*.*" "G:\SteamLibrary\SteamApps\common\ShadowOfMordor\x64\" /F /Y /S
xcopy /C "%1*.*" "G:\SteamLibrary\SteamApps\common\Alien Isolation\" /F /Y /S x32
Not clear if it works:
xcopy /C "%1*.*" "G:\SteamLibrary\SteamApps\common\HeadLander\" /F /Y /S
Doesn't change:
xcopy /C "%1*.*" "G:\Games\Batman - The Telltale Series\" /F /Y /S
xcopy /C "%1*.*" "G:\SteamLibrary\steamapps\common\Dishonored2\" /F /Y /S
It's notable that it works in MGS Ground Zeroes, including removing skipdxgi* hooks. This exp branch is merged from top-of-tree and fixes this regression. The most notable drawback here is this breaks the Upscaling feature. ForceDisplayParams is working.
I'll use this branch as my primary use case to get more experience with it, and will report back.
Overall this looks pretty good to me.
My question is whether we would be better served with a single layer wrap of the swap chain, or whether we should hook the calls we need.
I don't have a strong preference on this either way, but given how few calls we need out of it hooking just those calls does seem to simplify things pretty drastically, so I'm happy to support that approach. Aside from the present call I kind of view the DXGI stuff as a means to an end, and not really a focus of 3DMigoto, so I'm not worried about not having ready access to other DXGI calls and an approach that allows us to care less about it in the future is good.
I will feel better once we've adequately explained the crashes we've seen with hooking, so that we can either be confident that we won't hit them again, or know how to resolve them if/when we do. This doesn't add many new hooks though, so just testing the new Present and CreateSwapChain hooks on Win 7 with and without the evil update and on an up to date Win 10 is probably sufficient testing to be pretty confident these will be OK.
The most notable drawback here is this breaks the Upscaling feature
I'm probably the wrong person to comment on this, because I don't use the feature myself yet and don't care much about it, but of course regressions are best avoided where possible. Maybe if @ColAngel has some free cycles he can help out getting these working together?
I will feel better once we've adequately explained the crashes we've seen with hooking, so that we can either be confident that we won't hit them again, or know how to resolve them if/when we do. This doesn't add many new hooks though, so just testing the new Present and CreateSwapChain hooks on Win 7 with and without the evil update and on an up to date Win 10 is probably sufficient testing to be pretty confident these will be OK.
Yes, exactly this. That's why I'm putting these in an exp_ branch rather than master until we can see more clearly. It's my current impression that the Present/CreateSwapChain/CreateFactory hooked calls for DXGI should be good. This gives us the skipdxgi* behavior without having to skip.
The HackerContext hooks you saw crashes with different inputs is more of a concern.
Edit: don't wait on any changes you need in master. I will merge as needed.
I'm currently working on the HackerDevice/HackerContext single layer wrap for their pieces. (non published branch just yet.) This should clarify more completely with regard to Dishonored2 and other platform_update games.
I also plan to update to latest version of In-Proc, and DirectXTK in this exp_ branch. Should we move to git submodules for those, instead of code copies?
Edit: don't wait on any changes you need in master. I will merge as needed.
No worries, there might be an auto convergence feature in the near future (should be mostly contained to CommandList.cpp) ;-)
Any preference on whether you would be happy for the next release with what is currently in master, or if you would like any of these hooking changes in? I'll probably want to do a release soon, because there are a few important bug fixes in now - probably when I'm ready to release Before the Storm.
I also plan to update to latest version of In-Proc, and DirectXTK in this exp_ branch. Should we move to git submodules for those, instead of code copies?
Yeah, I'm happy to move to using submodules for these. Have you worked with submodules before? Some of their behaviour can be a little surprising and catch out the unaware.
Any preference on whether you would be happy for the next release with what is currently in master, or if you would like any of these hooking changes in? I'll probably want to do a release soon, because there are a few important bug fixes in now - probably when I'm ready to release Before the Storm.
I'd say go ahead whenever you are ready. I think TheDivision fix is OK for the time being, it doesn't crash. Not sure if it alters the colors (based on calls), but good enough for now. The dxgi hooking changes are not quite solid enough for release I think. When I'm doing experimental work I don't focus on quality, I'm trying to go faster and proof of concept.
Yeah, I'm happy to move to using submodules for these. Have you worked with submodules before? Some of their behaviour can be a little surprising and catch out the unaware.
I've used them some. The Deviare library using a submodule to their In-Proc. I've also used them in my last contract work. I'm certainly not an expert on these, but I'm sure I can make these work without much trouble. Awhile back I'd be hesitant to use these because they were more rare, and if you don't do a clone --recursive it can be confusing. Nowadays I think this is common enough that it won't cause too many problems for people building our stuff.
The most notable drawback here is this breaks the Upscaling feature
I'm probably the wrong person to comment on this, because I don't use the feature myself yet and don't care much about it, but of course regressions are best avoided where possible. Maybe if @ColAngel has some free cycles he can help out getting these working together?
Hello! I ll take a look at it as soon as possible. Do I understand correctly, basically you try to replace whole wrapping stuff via hooks? Anyway could you please give me the link to the branch with the work in progress.
@ColAngel No rush I think. Maybe take a look when it's convenient for you. This branch is currently experimental, so it's not in production shape. But, it seems to be working well, so it is likely that we will merge this to master at some point. Maybe a month or so. We want to stabilize 1.2 before making large changes like this.
That is correct, the entire HackerDXGI section was deleted, and replaced with HookedDXGI using Nektra In-Proc style hooks. Right now it hooks only the factories, CreateSwapChain, then Present. For Upscaling it looked like you would need some more DXGI hooks for the functionality.
Branch is: https://github.com/bo3b/3Dmigoto/tree/exp_dxgi_hook_only
That branch is up to date with master as well. You can find your prior code in the git history, or as it stands now in master.
I will feel better once we've adequately explained the crashes we've seen with hooking, so that we can either be confident that we won't hit them again, or know how to resolve them if/when we do. This doesn't add many new hooks though, so just testing the new Present and CreateSwapChain hooks on Win 7 with and without the evil update and on an up to date Win 10 is probably sufficient testing to be pretty confident these will be OK.
One thought on these problems seen with hooking. It's striking that some calls work and some don't based on the input parameters. My thought exercise is, what if MGS is wrapping objects itself? That difference in inputs is suggestive of a different C++ method signature. So, maybe some sort of vtable problem. e.g. What if they do a multiple inheritance hierarchy, and we hook only the first interface? Some other calls are implemented elsewise, and we don't have any visibility to those.
Not sure. I can't quite put together a clear failing sequence when using hooking. Just feels like some of the vtable problems I've looked at recently.
I suspect the answer will turn out to be that Deviare is breaking an early exit path. To install a hook it has to binary patch the first couple of instructions and replace them with a branch, but those original instructions still need to be executed somewhere when the hook is finished, which gives it three options:
Suppose it executes them out of line and one of the instructions is a ret - what does the stack look like? Is the return address going to take it back to the game, or back to the hooked function? Once it does return is the stack pointer correct for whichever calling convention this function used?
This is a theory at the moment, but it could explain why the SetShaderResource and SetSampler calls can crash if they aren't passed any resources - the first thing they do will be:
if (!NumViews)
return;
So it is very likely that Deviare has moved those instructions out to a trampoline and broken the return.
If this were the scenario, I would expect Microsoft Detours to fail the same way, and I've never heard of any problems with it. Deviare SpyStudio also will hook pretty much every OS call (I like APIMonitor better, but they both work). Also, if this were the case, I would expect hooking to be terribly unreliable, but we know that everyone uses it without real problems. It's used in commercial software like Steam, e.g.
They don't do any jsr, it's always jmp. First branch is always short (within 4G) so as to be certain to not break an instruction (smallest possible ASM instruction). Their trampoline is allocated within 4G of the actual routine being hooked. Should even handle the scenario where code jumps to the 2nd instruction in a routine, because they use the short jmp. They then also do the jmp back to inline. Should be no impact on the stack that I've seen. Maybe I'm missing the significance of the ret happening in the trampoline.
The only tricky part of the hook is deciding how many instructions to move to the trampoline. They use a built in third party disassembler to decide where the instruction boundaries are.
@ColAngel Hold off on looking at making any changes. We are taking a different tack here, which still includes a HackerSwapChain object that should allow us to keep Upscaling without any problems.
DarkStarSword and I think keeping a single-layer wrap is a better model than straight hooking, so unless we run into problems, Upscaling should still work.
If this were the scenario, I would expect Microsoft Detours to fail the same way, and I've never heard of any problems with it.
It's a different software package, so you can't compare the two. In principle, executing things out of line can be made to work, but some instructions need special handling to work in that context (the simplest example would be a relative branch, which would need the offset patched), but there is a lot of complexity here and it wouldn't surprise me at all if Deviare has missed some edge case. Single stepping inline will always work, but risks other threads missing the hook if they come in at the wrong time and could trigger anti-debugger detection in the game. Emulating the instructions can also be made to work, but if that's the only approach it would require a full x86 instruction set emulator. It's also possible to combine 1 and 3 - executing out of line for simple instructions and emulating difficult ones.
I have some experience with things like this from uprobes (user probes) in the Linux kernel, though I stopped following that patch set before it was finally merged into mainline and I'm not positive what they ended up going with (option 1 was frowned upon, because it was considered risky for the kernel to mess with userspace virtual memory without being asked. I know at least on PPC most instructions are emulated in the kernel).
Deviare SpyStudio also will hook pretty much every OS call (I like APIMonitor better, but they both work).
Yeah, that's fair, but we don't know for sure that the version we have doesn't have a bug that was fixed in the version in SpyStudio.
I think we should stop speculating though - we need evidence. I might look into adding some extra debugging to 3DMigoto to see what I can find.
So with your finding the answer to the hook crashes, we now have both 1.2.70+ and 1.3 branches working with MSGVTPP. Pretty awesome fix(s) for the regression.
I think 1.2 is still broken with MGSVTPP since 1.2.54+, but that doesn't matter - it's still using an old version of 3DMigoto for now and once 1.3 is released I'll update it - I want to take another crack at the problem with the wallhack drug thing anyway and may need my latest command list implementation to solve it.
MGSVTPP now works flawlessly again as of 1.3.2, so closing this issue (very timely with Metal Gear Survive being released since that appears to use the same engine).
3DMigoto 1.2.53 works with MGSVTPP, but 1.2.54 does not work - no overlay and appears that the fix has not been applied.