[Bug 51687] New: GetMappedFileNameA/W implementation breaks RiotClient updates.
https://bugs.winehq.org/show_bug.cgi?id=51687 Bug ID: 51687 Summary: GetMappedFileNameA/W implementation breaks RiotClient updates. Product: Wine Version: 6.15 Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: -unknown Assignee: wine-bugs(a)winehq.org Reporter: matias.nicolas.zc(a)gmail.com Distribution: --- Created attachment 70554 --> https://bugs.winehq.org/attachment.cgi?id=70554 trace log Since wine (vanilla or staging) 6.2, the Riot Client Launcher fails when updating itself. This worked fine with wine 6.1 (vanilla and staging) This wasn't much of a problem since LoL players used a lutris versions based on wine 5.x. Starting with the last League Of Legends Client update, the CEF upgrade requires a patched wine 6.15, so users will begin getting this error on the next Launcher update. The launcher throws the following error when starting the update process: ``` Update failed: Failed getting path to current executable: File path could not be found in a mapped drive: \??\c:\Riot Games\Riot Client\RiotClientServices.exe ``` I bisected the problem to commit bd0a3c1a59db9e75500db6df0f3598c981abf44e. For some reason the launcher worked with the stub implementation of GetMappedFileNameA/W, but not with a "real" one. I can confirm that reverting the commit on 6.15 makes the update work. Attached is a trace log of the execution (from wine staging 6.15, with the most frequent calls removed). The interesting part should start in line 546, when the debug message says that it will start the update, and end in line 1705, when the update process ends. In line 1361 there is a call to GetMappedFileNameW, which should be where the error begins. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 Matías Zúñiga <matias.nicolas.zc(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Regression SHA1| |bd0a3c1a59db9e75500db6df0f3 | |598c981abf44e -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 Matías Zúñiga <matias.nicolas.zc(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |matias.nicolas.zc(a)gmail.com --- Comment #1 from Matías Zúñiga <matias.nicolas.zc(a)gmail.com> --- Created attachment 70557 --> https://bugs.winehq.org/attachment.cgi?id=70557 GetMappedFileName: Return DOS filename The attached patch fixes the problem for me. I just learned that the "\??\" is the unix prefix, and i think GetMappedFileNameW shouldn't return that, so i copied part of the logic from get_process_image_name to get_mapping_filename (GetMappedFileNameW is the only user of this function) Looking at the logs and some example code from [microsoft](https://docs.microsoft.com/en-us/windows/win32/memory/obtaining-a-file-name-...), it turns out that the output path from GetMappedFileName shouldn't start with a device like "c:\", but with a dos path like "\Device\Harddisk1\" (the output from QueryDosDevice). With these two changes, the update process completes successfully, an better than before wine 6.1 (there was an error dialogue before) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 Gijs Vermeulen <gijsvrm(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|6.15 |6.2 Keywords| |patch, regression --- Comment #2 from Gijs Vermeulen <gijsvrm(a)gmail.com> --- Your patches fixes some todos in the psapi tests, but also instroduces a failure. Would you mind having a look at it and submitting your patch to wine-devel? psapi_main.c:475: Test succeeded inside todo block: map name does not start with a device name: \Device\Ha rddiskVolume1\users\gverm\Temp\mapf545.tmp psapi_main.c:483: Test failed: map name does not start with a device name: \Device\HarddiskVolume1\users\g verm\Temp\mapf545.tmp psapi_main.c:486: Test succeeded inside todo block: map name does not start with a device name: \Device\Ha rddiskVolume1\users\gverm\Temp\mapf545.tmp psapi_main.c:494: Test succeeded inside todo block: map name does not start with a device name: \Device\Ha rddiskVolume1\users\gverm\Temp\mapf545.tmp psapi_main.c:569: Test succeeded inside todo block: szImgPath="\Device\HarddiskVolume2\home\gverm\Desktop\ 64Bit\dlls\psapi\tests\psapi_test.exe" szMapPath="\Device\HarddiskVolume2\home\gverm\Desktop\64Bit\dlls\ps api\tests\psapi_test.exe" -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 Alexis Peypelut <iroalexis(a)outlook.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |iroalexis(a)outlook.fr -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 Zebediah Figura <z.figura12(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12(a)gmail.com --- Comment #3 from Zebediah Figura <z.figura12(a)gmail.com> --- We should probably be resolving DOS drive symlinks on the server side (or in ntdll?) rather than in kernelbase; the test failure sort of hints at that. \??\ isn't the unix prefix; it's the global NT namespace prefix. Unfortunately our handling of file paths is a little weird; the summary is that we basically treat drive letters as the canonical form even though they aren't on Windows. I've had some ideas on how to fix it, but unfortunately none of them are particularly pretty, and I haven't yet found an application that isn't satisfied with easier workarounds (such as resolving NT symlinks manually before returning them). -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 Matías Zúñiga <matias.nicolas.zc(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70557|0 |1 is obsolete| | --- Comment #4 from Matías Zúñiga <matias.nicolas.zc(a)gmail.com> --- Created attachment 70571 --> https://bugs.winehq.org/attachment.cgi?id=70571 GetMappedFileName: Return nt filename and resolve DOS drive path (In reply to Zebediah Figura from comment #3)
We should probably be resolving DOS drive symlinks on the server side (or in ntdll?) rather than in kernelbase; the test failure sort of hints at that.
\??\ isn't the unix prefix; it's the global NT namespace prefix.
Unfortunately our handling of file paths is a little weird; the summary is that we basically treat drive letters as the canonical form even though they aren't on Windows. I've had some ideas on how to fix it, but unfortunately none of them are particularly pretty, and I haven't yet found an application that isn't satisfied with easier workarounds (such as resolving NT symlinks manually before returning them).
I understand. I made the changes in kernelbase, because that's where QueryDosDeviceW was available. In the new (attached) version, i move the read_nt_symlink helper to ntdll.RtlReadNtSymlink, and use it directly instead of calling QueryDosDeviceW. Since read_nt_symlink is a small piece of code, maybe it should be copied to virtual.c, instead of exporting it in ntdll. The regressions tests pass, and 4 todos were removed. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 --- Comment #5 from Zebediah Figura <z.figura12(a)gmail.com> --- (In reply to Matías Zúñiga from comment #4)
I understand. I made the changes in kernelbase, because that's where QueryDosDeviceW was available.
In the new (attached) version, i move the read_nt_symlink helper to ntdll.RtlReadNtSymlink, and use it directly instead of calling QueryDosDeviceW. Since read_nt_symlink is a small piece of code, maybe it should be copied to virtual.c, instead of exporting it in ntdll.
The regressions tests pass, and 4 todos were removed.
Better, but I still have some comments: * You can't just add new helpers from ntdll; our exports have to match native. That said, if you've fixed NtQueryVirtualMemory, you shouldn't need to make any changes to kernelbase. * add_device_path() works, though on the other hand "\??\" is also a symlink and can be resolved as such. I.e. it should be a matter of just taking the path that's already returned, and (repeatedly) resolving all of the symlinks. * I don't understand why you should need to make any changes to the server at all. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 Matías Zúñiga <matias.nicolas.zc(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70571|0 |1 is obsolete| | --- Comment #6 from Matías Zúñiga <matias.nicolas.zc(a)gmail.com> --- Created attachment 70579 --> https://bugs.winehq.org/attachment.cgi?id=70579 GetMappedFileName: Return nt filename and resolve DOS drive path (In reply to Zebediah Figura from comment #5)
(In reply to Matías Zúñiga from comment #4)
I understand. I made the changes in kernelbase, because that's where QueryDosDeviceW was available.
In the new (attached) version, i move the read_nt_symlink helper to ntdll.RtlReadNtSymlink, and use it directly instead of calling QueryDosDeviceW. Since read_nt_symlink is a small piece of code, maybe it should be copied to virtual.c, instead of exporting it in ntdll.
The regressions tests pass, and 4 todos were removed.
Better, but I still have some comments:
* You can't just add new helpers from ntdll; our exports have to match native. That said, if you've fixed NtQueryVirtualMemory, you shouldn't need to make any changes to kernelbase.
* add_device_path() works, though on the other hand "\??\" is also a symlink and can be resolved as such. I.e. it should be a matter of just taking the path that's already returned, and (repeatedly) resolving all of the symlinks.
* I don't understand why you should need to make any changes to the server at all.
Right, that's what I thought. Since its a relavevely small helper function, I can just copy the code to virtual.c I was stripping the \??\ prefix because that's what QueryDosDeviceW required, but since NtOpenSymbolicLinkObject works with the prefix I can use that. Will the paths returned by get_mapping_filename always be in the form "\??\X:" if it does contain a drive path to resolve? An updated patch is attached. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 GloriousEggroll(a)gmail.com changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |GloriousEggroll(a)gmail.com --- Comment #7 from GloriousEggroll(a)gmail.com --- Posting here incase it gets missed on the e-mail response: I've discovered that this patch, or at least the v2 submitted version in the mailing list, causes Elder Scrolls Online to crash a few seconds after the game launches. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 --- Comment #8 from Matías Zúñiga <matias.nicolas.zc(a)gmail.com> --- (In reply to GloriousEggroll from comment #7)
Posting here incase it gets missed on the e-mail response:
I've discovered that this patch, or at least the v2 submitted version in the mailing list, causes Elder Scrolls Online to crash a few seconds after the game launches.
Hi GloriousEggroll! Yeah sorry, i totally forgot about this patch (and soon will send an updated version addressing some of Zebediah's comments) I dont own that game, so i cant try to debug the issue. Do you have some debug info for when the crash happens? Did it broke recently (it worked with a previous wine versions + this patch)? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=51687 Ker noa <blue-t(a)web.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |blue-t(a)web.de -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
WineHQ Bugzilla