FEXLoader: Fixes newer wine versions and Fedora #4082

Sonicadvance1 · 2024-09-22T22:03:32Z

This was brought up by #3831 but I finally got the courage to look at the hard problem.

Although I'm only tackling half of the problem with this PR, which is that FEXLoader needs to strip the rootfs path from the executed path if it begins with the rootfs, plus some changes to the surrounding code.

The primary concern here is that when an application has been executed under FEX, specifically through binfmt_misc, then FEX needs to prepend the full rootfs path otherwise Linux can't find the program. Additionally execveat with an FD will resolve a full path to the rootfs.

So past FEX's initial setup, we need to strip off the rootfs path to provide an "absolute" path that is visible to the guest application later. Which is kind of funny since we have a RootFSRedirect function which did the exact opposite. This was due to legacy problems in the original ELFLoader that couldn't handle symlinks correctly, which has since been resolved, so that no longer needs to exist.

There was also some weirdness in GetApplicationNames where the passed in argument list was modifying Args[0] and then saving the Program as well. Which I just got rid of. Also stopped passing in the arguments by value because....why did I write it like that?

In InterpreterHandler we now need to check if we can open the path inside the rootfs or fallback without it. Plus I had to change the shebang handling so it stopped prefixing the rootfs AGAIN. Took the time to change the shebang handling there so it stops creating string copies and instead just generates views.

Overall this fixes a fairly major flaw with how we were representing /proc/self to the application, which was breaking wine since it would prefix the rootfs multiple times, which was weird.

It doesn't address the remaining problem in #3831, which is that applications can still see some of the leaky abstractions with symlinks through the rootfs, but I want to get at least this step in.

neobrain · 2024-09-24T09:12:53Z

Could you add some concrete examples of what this PR is aiming to fix? From the description it's a bit difficult to reconstruct what exactly the problematic cases here are. (As a general suggestion, please avoid mixing in rants about split_view into high-level overviews since that gets in the way of communicating the line of thought.)

Source/Common/Config.h

Sonicadvance1 · 2024-09-24T09:59:09Z

Could you add some concrete examples of what this PR is aiming to fix? From the description it's a bit difficult to reconstruct what exactly the problematic cases here are.

The concrete example is that FEXBash winecfg doesn't work; It exits out when wine-preloader tries to execute wine32. This is due to us accidentally providing a fully resolved path of the application all the way to the guest. This causes wine to try and execute a path that is <Rootfs_Prefix>/<RootFS_Prefix>/usr/bin/wine32 and then execve returns ENOENT.

FEX initially needs the path fully resolved to actually load the application, but then we need to strip off the rootfs prefix (And NOT reapply it!). This fixes the logic inside of FileManagement with proc/self and syscalls so Wine can properly execute its subprocess applications.

(As a general suggestion, please avoid mixing in rants about split_view into high-level overviews since that gets in the way of communicating the line of thought.)

Removed the sentence.

neobrain · 2024-09-28T14:10:45Z

As discussed externally, this is what's going on:

Overview

When launching programs contained in the RootFS, FEX could previously report invalid program names to the guest (via /proc/self/exe and argv[0]) by double-prepending the RootFS path. This could produce paths like /home/ryanh/.fex-emu/RootFS/Ubuntu_24_04/home/ryanh/.fex-emu/RootFS/Ubuntu_24_04/usr/bin/winecfg.

This could break programs like Wine that read /proc/self/exe to determine their installation path.

To resolve the issue, FEX now consistently strips away the RootFS prefix from executable paths reported to the guest through /proc/self/exe and argv[0].

Implementation details

Fixes come from multiple places.

Config.cpp: RecoverGuestProgramFilename: Was modifying Args[0] in-place. This would later get reused in...
FEXLoader.cpp: InterpreterHandler & RootFSRedirect: Was reading Args[0] & ProgramName respectively with rootfs prepended
Returned values from Config.cpp was stored in CONFIG_APP_FILENAME which was then read by...
FileManagement.cpp: FileManager::GetSelf: Which results in symlinkat emulation to return the fully qualified / which would feed in to the first two values via...
Syscalls.cpp: ExecveHandler - Which would check to see if the path exists and pass it directly to the newly executed interpreter.

asahilina · 2024-09-29T14:26:04Z

FWIW I rebased #3831 on top of this (which replaces one of the two commits there) and it works as intended, plus Proton is now working fine again too (that was due to a regression in #3831, not this PR).

wine does need the symlink resolution fixes at least on Fedora though, so this PR is not sufficient to fix it (the title is a bit confusing).

neobrain

Reasonable change overall, just some comments to polish the implementation a little.

Source/Tools/FEXLoader/FEXLoader.cpp

neobrain · 2024-09-30T17:49:37Z

Source/Tools/FEXLoader/FEXLoader.cpp

+  if (Program.ProgramPath.starts_with(LDPath())) {
+    Program.ProgramPath = Program.ProgramPath.substr(LDPath().size());
+  }


Is there any chance this will result in a relative path? If not, how is it prevented? It would probably be undesirable behavior.

LDPath will never be relative, if it is then it's a configuration error and should be handled elsewhere.
By that definition then ProgramPath will never be relative when it starts with LDPath.

Is there a chance we accidentally strip one '/' off to many, though? For example, does it make a difference if the user configures the RootFS as "/home/user/rootfsfolder/" vs "/home/user/rootfsfolder"?

Another possible failure path: Can the program paths end up containing "..", escaping the RootFS?

To clarify, I'm not concerned about LDPath being relative, but about the result of calling substr().

Another possible failure path: Can the program paths end up containing "..", escaping the RootFS?

Probably, but this doesn't change any behaviour around that anyway. It'll be broken before and it'll be broken afterwards.

Slightly changed the code doing some more thorough testing, to ensure the beginning / isn't stripped off.

Source/Tools/FEXLoader/FEXLoader.cpp

neobrain · 2024-10-01T16:38:11Z

Source/Tools/FEXLoader/FEXLoader.cpp

-  RootFSRedirect(&Program.ProgramPath, LDPath());
-  InterpreterHandler(&Program.ProgramPath, LDPath(), &Args);
+  if (Program.ProgramPath.starts_with(LDPath())) {
+    // From this point on, ProgramPath needs to not have the LDPath prefixed on to it.


It's probably better to move towards RootFS long-term since that's what every other place referring to that setting calls it.

BTW, is ProgramPath used for anything else (other than /proc/self/exe and argv[0]) after this point? Is removing the prefix the right choice for all other uses as well?

It's probably better to move towards RootFS long-term since that's what every other place referring to that setting calls it.

A follow-up PR can be done to rename the variable names from LDPath to RootFS, there's a couple of places that can be hit at once.
Additional followup PR should be to rename APP_FILENAME to APP_FILEPATH because it's not the filename but the fully qualified guest path. But that should be independent.

BTW, is ProgramPath used for anything else (other than /proc/self/exe and argv[0]) after this point? Is removing the prefix the right choice for all other uses as well?

It also gets passed to the ELFCodeLoader, but it correctly handles the rootfs prefixing as necessary.

It gets passed to seccomp, but only for a log.

It gets passed to gdbserver, but only for removing the executable from the segment map for some reason?

Probably something to check on later, won't really affect this.

Sonicadvance1 · 2024-10-04T20:37:40Z

Reworked a bit of this and now it's less frenetic. Adding unittests for the argument parsing and ensuring the ProgramPath is always absolute if prefixed with the rootfs path. Will be good to get this in before release.

Split this out so we can unittest it. Adds a unittest to handle specific edge cases.

This was brought up by FEX-Emu#3831 but I finally got the courage to look at the hard problem. Although I'm only tackling half of the problem with this PR, which is that FEXLoader needs to strip the rootfs path from the executed path if it begins with the rootfs, plus some changes to the surrounding code. The primary concern here is that when an application has been executed under FEX, specifically through binfmt_misc, then FEX needs to prepend the full rootfs path otherwise Linux can't find the program. Additionally execveat with an FD will resolve a full path to the rootfs. So past FEX's initial setup, we need to strip off the rootfs path to provide an "absolute" path that is visible to the guest application later. Which is kind of funny since we have a `RootFSRedirect` function which did the exact opposite. This was due to legacy problems in the original ELFLoader that couldn't handle symlinks correctly, which has since been resolved, so that no longer needs to exist. There was also some weirdness in `GetApplicationNames` where the passed in argument list was modifying Args[0] and then saving the Program as well. Which I just got rid of. Also stopped passing in the arguments by value because....why did I write it like that? In InterpreterHandler we now need to check if we can open the path inside the rootfs or fallback without it. Plus I had to change the shebang handling so it stopped prefixing the rootfs AGAIN. Took the time to change the shebang handling there so it stops creating string copies and instead just generates views. Overall this fixes a fairly major flaw with how we were representing `/proc/self` to the application, which was breaking wine since it would prefix the rootfs multiple times, which was weird. It doesn't address the remaining problem in FEX-Emu#3831, which is that applications can still see some of the leaky abstractions with symlinks through the rootfs, but I want to get at least this step in.

Sonicadvance1 · 2024-10-16T17:05:06Z

Making noise.

Sonicadvance1 force-pushed the fix_wine branch from 231b3a0 to 996f4d9 Compare September 22, 2024 22:07

neobrain reviewed Sep 24, 2024

View reviewed changes

Source/Common/Config.h Show resolved Hide resolved

Sonicadvance1 force-pushed the fix_wine branch 2 times, most recently from b5d1cbc to d9a8309 Compare September 24, 2024 09:57

neobrain reviewed Sep 30, 2024

View reviewed changes

Sonicadvance1 force-pushed the fix_wine branch from d9a8309 to 776df1a Compare September 30, 2024 19:12

neobrain reviewed Oct 1, 2024

View reviewed changes

Source/Tools/FEXLoader/FEXLoader.cpp Outdated Show resolved Hide resolved

neobrain reviewed Oct 1, 2024

View reviewed changes

Sonicadvance1 force-pushed the fix_wine branch 3 times, most recently from 17aa5b0 to 7217614 Compare October 4, 2024 20:24

Sonicadvance1 force-pushed the fix_wine branch from 7217614 to a4e5a4c Compare October 6, 2024 01:07

Sonicadvance1 added 2 commits October 11, 2024 01:40

FHU: Add StringArgumentParser function

6f096e7

Split this out so we can unittest it. Adds a unittest to handle specific edge cases.

Sonicadvance1 force-pushed the fix_wine branch from a4e5a4c to fe5bc02 Compare October 11, 2024 08:40

alyssarosenzweig approved these changes Oct 16, 2024

View reviewed changes

Sonicadvance1 merged commit d66ed71 into FEX-Emu:main Oct 16, 2024
12 checks passed

Sonicadvance1 deleted the fix_wine branch October 16, 2024 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEXLoader: Fixes newer wine versions and Fedora #4082

FEXLoader: Fixes newer wine versions and Fedora #4082

Sonicadvance1 commented Sep 22, 2024 •

edited

Loading

neobrain commented Sep 24, 2024

Sonicadvance1 commented Sep 24, 2024

neobrain commented Sep 28, 2024

asahilina commented Sep 29, 2024 •

edited

Loading

neobrain left a comment

neobrain Sep 30, 2024

Sonicadvance1 Sep 30, 2024

neobrain Sep 30, 2024

neobrain Sep 30, 2024

Sonicadvance1 Sep 30, 2024

Sonicadvance1 Oct 4, 2024

neobrain Oct 1, 2024

Sonicadvance1 Oct 4, 2024

Sonicadvance1 commented Oct 4, 2024

Sonicadvance1 commented Oct 16, 2024

FEXLoader: Fixes newer wine versions and Fedora #4082

FEXLoader: Fixes newer wine versions and Fedora #4082

Conversation

Sonicadvance1 commented Sep 22, 2024 • edited Loading

neobrain commented Sep 24, 2024

Sonicadvance1 commented Sep 24, 2024

neobrain commented Sep 28, 2024

Overview

Implementation details

asahilina commented Sep 29, 2024 • edited Loading

neobrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sonicadvance1 commented Oct 4, 2024

Sonicadvance1 commented Oct 16, 2024

Sonicadvance1 commented Sep 22, 2024 •

edited

Loading

asahilina commented Sep 29, 2024 •

edited

Loading