[Bug 51687] New: GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

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: [e-mail]
          Reporter: [e-mail]
      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.

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

Matías Zúñiga  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
    Regression SHA1|                            |bd0a3c1a59db9e75500db6df0f3
                   |                            |598c981abf44e

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

Matías Zúñiga  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Matías Zúñiga  ---
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)

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

Gijs Vermeulen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|6.15                        |6.2
           Keywords|                            |patch, regression

--- Comment #2 from Gijs Vermeulen  ---
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"

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

Alexis Peypelut  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

Zebediah Figura  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #3 from Zebediah Figura  ---
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).

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

Matías Zúñiga  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #70557|0                           |1
        is obsolete|                            |

--- Comment #4 from Matías Zúñiga  ---
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.

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

--- Comment #5 from Zebediah Figura  ---
(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.

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

Matías Zúñiga  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #70571|0                           |1
        is obsolete|                            |

--- Comment #6 from Matías Zúñiga  ---
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.

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

[e-mail] changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #7 from [e-mail] ---
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.

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

--- Comment #8 from Matías Zúñiga  ---
(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)?

[Bug 51687] GetMappedFileNameA/W implementation breaks RiotClient updates.

[WineHQ Bugzilla]

https://bugs.winehq.org/show_bug.cgi?id=51687

Ker noa  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]