https://bugs.winehq.org/show_bug.cgi?id=47970
Bug ID: 47970 Summary: Legends of Runeterra crashes at launch Product: Wine Version: unspecified Hardware: x86 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: -unknown Assignee: wine-bugs@winehq.org Reporter: dt@zeroitlab.com Distribution: ---
There's not much else I can attach at the moment. Compiling a 64 bit wine version with the previous patches for League of Legends (#47198 #47915) does not seem enough to run the game. Its crashing deep inside the anti cheat during initialization. I'm trying to track this down.
https://bugs.winehq.org/show_bug.cgi?id=47970
juliansperling@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |juliansperling@gmail.com
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #1 from David Torok dt@zeroitlab.com --- Created attachment 65592 --> https://bugs.winehq.org/attachment.cgi?id=65592 Proof of concept fix for Legends of Runeterra
This gets the game up and running on my end. Feedback is appreciated. Patches can be applied on top of wine staging git. The game seems to runs great with DXVK.
The new x86_64 syscall dispatcher I created may not be the best way to solve the issue, take it as a PoC please.
https://bugs.winehq.org/show_bug.cgi?id=47970
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12@gmail.com
--- Comment #2 from Zebediah Figura z.figura12@gmail.com --- Nice work, David. I assume this on top of current Staging is enough?
With respect to 0001, I think the better thing to do is move NtContinue() into the signal_* files. Though why that check is there in the first place, I'm less sure. Can NtContinue() really return an error code? And we already have such a check in NtSetContextThread()...
0002 deserves more careful scrutiny, but the idea of returning to the syscall thunk is sensible; we already do the same thing for i386.
0003 does two things. Using NtContinue() makes sense on the model of the current Staging patch; manually setting %ds is probably correct but (a) deserves tests, and probably we should test all the other segment registers, (b) signal_i386 takes care of that in setup_raise_exception(), and so we should maybe do the same?
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #3 from David Torok dt@zeroitlab.com --- (In reply to Zebediah Figura from comment #2)
Nice work, David. I assume this on top of current Staging is enough?
Thank you Zebediah! Current staging should be good enough. (I was testing with staging-git)
Regarding #1 - I'm not yet sure about the root cause, but I was receiving an i386 context in 64 bit mode. So the check is in place to allow a full i386 context when handling an x86_64 signal. I may need to track down the root cause, because this may or may not be desirable for wine. I have not checked how windows handles this.
Regarding #2 - I have an alternative version of this patch, which clones the syscall argument's from the parent frame, like wine currently does for 32 bit. That fix originally came from Andrew Wesie - and he mentioned that approach could cause a segmentation fault if the arguments are sufficiently close to the end of the stack memory allocation. So I figured, I might try an approach that does not involve growing the stack further. I guess it is best I explain the intent with the dispatcher version I posted: The initial sub instruction changes the thunk's return address to point into the first return of the thunk, this is crucial, and needed by the software to function. Then the thunk's return and the thunk's caller's return are saved into unused space in the TEB and later restored. That is how this version solves the problem of the syscall arguments having to be a specific distance away on the stack. (aka, only having space for one return address) You may recognize, I also left the original code path, in case the TEB was not available for some reason. I'm not sure if this could happen at all.
Regarding #3 - 64 bit linux signal contexts do not have DS: https://github.com/torvalds/linux/blob/6f0d349d922ba44e4348a17a78ea51b713596... Hence Wine cannot and will not capture DS and therefore DS_sig() does not exist for x86_64. My understanding is that DS and SS should always be the same on Windows, hence the manual set in the context structure.
https://bugs.winehq.org/show_bug.cgi?id=47970
Paul Gofman gofmanp@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |gofmanp@gmail.com
https://bugs.winehq.org/show_bug.cgi?id=47970
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |4.19
--- Comment #4 from Zebediah Figura z.figura12@gmail.com --- (In reply to David Torok from comment #3)
Regarding #1 - I'm not yet sure about the root cause, but I was receiving an i386 context in 64 bit mode. So the check is in place to allow a full i386 context when handling an x86_64 signal. I may need to track down the root cause, because this may or may not be desirable for wine. I have not checked how windows handles this.
Mmh. That seems odd, and I'm not sure how we're supposed to distinguish the two. Is the game calling NtContinue() from 32-bit code? Or maybe there's a difference in flags? I'd also like to see what the %cs and %rip values of the new context are (e.g. is it trying to ljmp into a 32-bit segment?)
Regarding #2 - I have an alternative version of this patch, which clones the syscall argument's from the parent frame, like wine currently does for 32 bit. That fix originally came from Andrew Wesie - and he mentioned that approach could cause a segmentation fault if the arguments are sufficiently close to the end of the stack memory allocation. So I figured, I might try an approach that does not involve growing the stack further. I guess it is best I explain the intent with the dispatcher version I posted: The initial sub instruction changes the thunk's return address to point into the first return of the thunk, this is crucial, and needed by the software to function. Then the thunk's return and the thunk's caller's return are saved into unused space in the TEB and later restored. That is how this version solves the problem of the syscall arguments having to be a specific distance away on the stack. (aka, only having space for one return address) You may recognize, I also left the original code path, in case the TEB was not available for some reason. I'm not sure if this could happen at all.
Ah, yes, so now that I've been able to give a cursory examination, what you're doing unfortunately won't work. Nt*() calls can be reëntrant on Wine (I know, this isn't correct, but fixing it is a lot harder), so we have to use the stack. Fortunately I think this is a lot easier on x86_64, since it's always caller-cleanup.
I've taken the liberty of trying to implement this part myself:
https://github.com/wine-staging/wine-staging/commit/5fbf201ea8b61b09540905a9...
Can you please give it a try (after applying your patches 0001 and 0003) and see if it still works?
Regarding #3 - 64 bit linux signal contexts do not have DS: https://github.com/torvalds/linux/blob/ 6f0d349d922ba44e4348a17a78ea51b7135965b1/arch/x86/include/uapi/asm/ sigcontext.h#L257 Hence Wine cannot and will not capture DS and therefore DS_sig() does not exist for x86_64. My understanding is that DS and SS should always be the same on Windows, hence the manual set in the context structure.
I did a quick test. On my machine, it is true that the values for the segment selectors differ—e.g. I get nonzero values for %cs and %ss, but zero for the other four. But the values are the same outside of the exception handler and inside of it. Is the program depending on %ds having a nonzero value inside of the exception handler?
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #5 from Zebediah Figura z.figura12@gmail.com --- Ech, I missed the part where you said you already had a version that copied arguments. Alas for duplicated effort :-/
https://bugs.winehq.org/show_bug.cgi?id=47970
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Component|-unknown |ntdll Hardware|x86 |x86-64
--- Comment #6 from Zebediah Figura z.figura12@gmail.com --- I've pushed all of the changes to Staging except for the one setting SegDs to SegSs.
Reportedly, the game does in fact demand that SegDs == SegSs in an exception handler. Ideally we'd like to not have to lie about SegDs (consider e.g. debuggers), so it would be best that %ds actually is equal to %ss. I'm not immediately sure what we'd need to do to make that the case. Is setting it once in init_thread_context() enough, or will the kernel ever touch it later? Is it correct to set %ds to be equal to %ss in the first place? I can't think of any reason why not, but it would be nice to have a guarantee from the kernel.
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #7 from David Torok dt@zeroitlab.com --- Created attachment 65624 --> https://bugs.winehq.org/attachment.cgi?id=65624 Alternative patch to SegDS spoofing
Alternative patch to 0003's ds spoofing
My understanding is that the kernel has no business messing with the userland context structure arbitrarily. %ds should be preserved, since x86_64 "set_full_cpu_context" does not set %ds.
On top of that, 0003 has been applied incorrectly on the staging repo. __syscall_NtContinue has to take ( context, FALSE ) as arguments.
With that in place, Legends of Runeterra works. Thanks for the quick clean-up on the dispatcher, looks great :)
https://bugs.winehq.org/show_bug.cgi?id=47970
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Staged patchset| |https://github.com/wine-sta | |ging/wine-staging/tree/mast | |er/patches/ntdll-x86_64_Seg | |Ds Status|UNCONFIRMED |STAGED Ever confirmed|0 |1
--- Comment #8 from Zebediah Figura z.figura12@gmail.com --- Surprisingly it turns out that Windows itself lies about SegDs, i.e. it returns a fixed value for it in NtGetContextThread()/RtlCaptureContext() regardless of its actual value, so I ended up writing a patch mostly like the one you attached here. That doesn't mean that we necessarily want it like this upstream—lying about the thread context is not great even if Windows does it too—but I'm satisfied to put it in Staging, at least until it breaks something (and boy does it make me nervous...)
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #9 from David Torok dt@zeroitlab.com --- (In reply to Zebediah Figura from comment #8)
Surprisingly it turns out that Windows itself lies about SegDs, i.e. it returns a fixed value for it in NtGetContextThread()/RtlCaptureContext() regardless of its actual value, so I ended up writing a patch mostly like the one you attached here. That doesn't mean that we necessarily want it like this upstream—lying about the thread context is not great even if Windows does it too—but I'm satisfied to put it in Staging, at least until it breaks something (and boy does it make me nervous...)
Thank you for the rigorous testing and consideration! I am very happy to see everything staged :)
https://bugs.winehq.org/show_bug.cgi?id=47970
Andrew Wesie awesie@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |awesie@gmail.com
--- Comment #10 from Andrew Wesie awesie@gmail.com --- (In reply to Zebediah Figura from comment #8)
Surprisingly it turns out that Windows itself lies about SegDs, i.e. it returns a fixed value for it in NtGetContextThread()/RtlCaptureContext() regardless of its actual value, so I ended up writing a patch mostly like the one you attached here. That doesn't mean that we necessarily want it like this upstream—lying about the thread context is not great even if Windows does it too—but I'm satisfied to put it in Staging, at least until it breaks something (and boy does it make me nervous...)
What is your concern with pushing it upstream if tests show that it matches behavior with 64-bit Windows?
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #11 from Zebediah Figura z.figura12@gmail.com --- (In reply to Andrew Wesie from comment #10)
(In reply to Zebediah Figura from comment #8)
Surprisingly it turns out that Windows itself lies about SegDs, i.e. it returns a fixed value for it in NtGetContextThread()/RtlCaptureContext() regardless of its actual value, so I ended up writing a patch mostly like the one you attached here. That doesn't mean that we necessarily want it like this upstream—lying about the thread context is not great even if Windows does it too—but I'm satisfied to put it in Staging, at least until it breaks something (and boy does it make me nervous...)
What is your concern with pushing it upstream if tests show that it matches behavior with 64-bit Windows?
My primary thought is that it'd be nice if we didn't lie to debuggers. (Though, that said, I'm increasingly a proponent of running the entirety of Wine under a debugger.)
https://bugs.winehq.org/show_bug.cgi?id=47970
Jacob Hrbek werifGX@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |werifGX@gmail.com
--- Comment #12 from Jacob Hrbek werifGX@gmail.com --- The staged patch doesn't seem to be enough to fix this issue on wine-6.3 distributed for Debian Bullseye by wineHQ (suspecting regression).
Game seems to fail the same was on launch as League of Legends on mentioned bug after champ select, see screenshot.
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #13 from Jacob Hrbek werifGX@gmail.com --- Created attachment 69605 --> https://bugs.winehq.org/attachment.cgi?id=69605 LoR fatal error message
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #14 from David Torok dt@zeroitlab.com --- (In reply to Jacob Hrbek from comment #13)
Created attachment 69605 [details] LoR fatal error message
Jacob, could you send me the dump you get from that and your game exe?
https://bugs.winehq.org/show_bug.cgi?id=47970
Daniel Bomar dbdaniel42@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dbdaniel42@gmail.com
--- Comment #15 from Daniel Bomar dbdaniel42@gmail.com --- Created attachment 69676 --> https://bugs.winehq.org/attachment.cgi?id=69676 LoR.exe crash dump
Full dump was ~100MB after compressing with xz (well over the ~10MB file size limit for Bugzilla) so I've attached the small dump option it gave me. If a dev needs the big dump, let me know how else I can send it.
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #16 from Daniel Bomar dbdaniel42@gmail.com --- (In reply to David Torok from comment #14)
(In reply to Jacob Hrbek from comment #13)
Created attachment 69605 [details] LoR fatal error message
Jacob, could you send me the dump you get from that and your game exe?
I'm not the person who originally reported this but I'm getting the same thing. I've attached the dump. I'm on wine-staging 6.4 on Arch Linux.
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #17 from Daniel Bomar dbdaniel42@gmail.com --- I did some testing on this using the Wine sources from git and I can confirm a regression. With all staging patches applied:
Last known working version: v5.9 (game seems to fully work and I was able to complete a game online) First known bad version: v.5.10 (game does not launch)
I was going to do a more thorough bisect but the issue is the game seems to depend on multiple staging patches in order to launch. I tried compiling with only ntdll-x86_64_SegDs, ntdll-WRITECOPY, and user32-InternalGetWindowIcon but this configuration failed to launch the game on v5.9.
The wiki page on regression testing states that when staging patches are required, the next step is to narrow down which patches are required but I'm a little stuck here as I tried the only 3 patches mentioned in bug reports and it still does not work. I'm only able to get it to work by applying all patches.
If anyone knows which patches are required, please let me know and I can do a better bisect.
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #18 from Zebediah Figura z.figura12@gmail.com --- (In reply to Daniel Bomar from comment #17)
I did some testing on this using the Wine sources from git and I can confirm a regression. With all staging patches applied:
Last known working version: v5.9 (game seems to fully work and I was able to complete a game online) First known bad version: v.5.10 (game does not launch)
I was going to do a more thorough bisect but the issue is the game seems to depend on multiple staging patches in order to launch. I tried compiling with only ntdll-x86_64_SegDs, ntdll-WRITECOPY, and user32-InternalGetWindowIcon but this configuration failed to launch the game on v5.9.
The wiki page on regression testing states that when staging patches are required, the next step is to narrow down which patches are required but I'm a little stuck here as I tried the only 3 patches mentioned in bug reports and it still does not work. I'm only able to get it to work by applying all patches.
If anyone knows which patches are required, please let me know and I can do a better bisect.
One thing you can do is to reverse-bisect between wine-staging and upstream wine. Of course, that's not ideal in its own ways. It's a rather unfortunate case when a game depends on several wine-staging patch sets.
Another possible approach, that may be helpful enough here, is to try individual wine-staging rebase versions between 5.9 and 5.10 That doesn't constitute a full bisect, of course, but would at least narrow down to the commits for a particular day, which may be enough of a clue in itself to make a good guess.
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #19 from Daniel Bomar dbdaniel42@gmail.com --- Created attachment 69697 --> https://bugs.winehq.org/attachment.cgi?id=69697 bisect from wine-staging
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #20 from Daniel Bomar dbdaniel42@gmail.com --- Created attachment 69698 --> https://bugs.winehq.org/attachment.cgi?id=69698 bisect from upstream wine
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #21 from Daniel Bomar dbdaniel42@gmail.com --- Created attachment 69699 --> https://bugs.winehq.org/attachment.cgi?id=69699 error message shown in bad revisions
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #22 from Daniel Bomar dbdaniel42@gmail.com --- (In reply to Zebediah Figura from comment #18)
(In reply to Daniel Bomar from comment #17)
I did some testing on this using the Wine sources from git and I can confirm a regression. With all staging patches applied:
Last known working version: v5.9 (game seems to fully work and I was able to complete a game online) First known bad version: v.5.10 (game does not launch)
I was going to do a more thorough bisect but the issue is the game seems to depend on multiple staging patches in order to launch. I tried compiling with only ntdll-x86_64_SegDs, ntdll-WRITECOPY, and user32-InternalGetWindowIcon but this configuration failed to launch the game on v5.9.
The wiki page on regression testing states that when staging patches are required, the next step is to narrow down which patches are required but I'm a little stuck here as I tried the only 3 patches mentioned in bug reports and it still does not work. I'm only able to get it to work by applying all patches.
If anyone knows which patches are required, please let me know and I can do a better bisect.
One thing you can do is to reverse-bisect between wine-staging and upstream wine. Of course, that's not ideal in its own ways. It's a rather unfortunate case when a game depends on several wine-staging patch sets.
Another possible approach, that may be helpful enough here, is to try individual wine-staging rebase versions between 5.9 and 5.10 That doesn't constitute a full bisect, of course, but would at least narrow down to the commits for a particular day, which may be enough of a clue in itself to make a good guess.
Thanks for the advice. I just uploaded the results of the bisect plus the error message I was getting from the bad revisions. I wasn't sure how to reverse-bisect between two different git repositories so I started with the 2nd method you mentioned. That showed the first bad commit was a rebase so I used that information to do a bisect on upstream Wine. I had to do a bit of manual fiddling with the patches to get the intermediate builds to compile but I think my results are correct.
# first bad commit: [be0eb9c92eb7a4fcd9d0d48568c8ed5e8326ef0b] ntdll: Move the thread startup code to the Unix library
https://bugs.winehq.org/show_bug.cgi?id=47970
Vijay Kamuju infyquest@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |infyquest@gmail.com
--- Comment #23 from Vijay Kamuju infyquest@gmail.com --- patch upstreamed to wine https://source.winehq.org/git/wine.git/commitdiff/195c57eeb7e62263af229baf5f...
https://bugs.winehq.org/show_bug.cgi?id=47970
Julian Rüger jr98@gmx.net changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jr98@gmx.net
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #24 from Daniel Bomar dbdaniel42@gmail.com --- (In reply to Vijay Kamuju from comment #23)
patch upstreamed to wine https://source.winehq.org/git/wine.git/commitdiff/ 195c57eeb7e62263af229baf5f486f353bf6459e
That patch doesn't seem to have fixed it. Backporting that patch did work on be0eb9c92eb7a4fcd9d0d48568c8ed5e8326ef0b but it broke again on the very next commit (35b063a404457fdf956d1913738a3c8a66266cb4) and it doesn't work on the current wine version either.
I've been trying to debug the problem but haven't really gotten anywhere. I have a hunch that the problem started with the wine-staging rebase of winebuild-Fake_Dlls. Some of the code for the syscall thunks got moved from thread.c to loader.c. But those patches aren't even in the current version of wine so I'm not sure.
https://bugs.winehq.org/show_bug.cgi?id=47970
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |195c57eeb7e62263af229baf5f4 | |86f353bf6459e Resolution|--- |FIXED Status|STAGED |RESOLVED
--- Comment #25 from Zebediah Figura z.figura12@gmail.com --- Yes, ideally that regression should be reported as a separate bug. I'm marking this one fixed.
https://bugs.winehq.org/show_bug.cgi?id=47970
Alexandre Julliard julliard@winehq.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #26 from Alexandre Julliard julliard@winehq.org --- Closing bugs fixed in 6.6.
https://bugs.winehq.org/show_bug.cgi?id=47970
--- Comment #27 from David Torok dt@zeroitlab.com --- (In reply to Zebediah Figura from comment #25)
Yes, ideally that regression should be reported as a separate bug. I'm marking this one fixed.
Thank you for upstreaming this Zebediah! I opened #50952 for the regression.
-
WineHQ Bugzilla