Path resolution fixes #3831
Open
Path resolution fixes #3831
+207
−73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asahilina
commented
Jul 6, 2024
asahilina
commented
- FEXLoader: Strip rootfs path from CONFIG_APP_FILENAME
- FileManagement: Trace symlink paths across rootfs
|
👍 I'll take a peek today and if needed over the next couple days to see what we can do to resolve these issues. Going to need a bunch of testing due to how likely this is to affect things like Proton and pressure-vessel. |
andy9a9
reviewed
Jul 6, 2024
| }; | ||
|
|
||
| // Current index for the temporary path to use. | ||
| uint32_t CurrentIndex {}; |
There was a problem hiding this comment.
What's the value here? Shouldn't be = 0 used instead?
| break; | ||
| } | ||
| } | ||
| auto FdPath = GetEmulatedFDPath(AT_FDCWD, pathname, FollowSymlink, TmpFilename); |
There was a problem hiding this comment.
Maybe FdPath and Path shouldn't be declared while their values aren't used :).
Sonicadvance1
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Sep 22, 2024
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. Which again this would be nice if it could use `std::ranges::split_view` but since that would raise the clang minspec to 16, we have to write it ourselves. 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
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Sep 22, 2024
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. Which again this would be nice if it could use `std::ranges::split_view` but since that would raise the clang minspec to 16, we have to write it ourselves. 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
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Sep 24, 2024
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. Which again this would be nice if it could use `std::ranges::split_view` but since that would raise the clang minspec to 16, we have to write it ourselves. 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
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Sep 24, 2024
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.
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 Note that d9a8309 is not enough to fix wine, you still need b483c80 here since it's needed to make the /etc/alternatives stuff work with wine's path resolution. |
Indeed, my PR is only a partial fix. I still need to work through the rest of the problems with rootfs symlink problem. |
asahilina
force-pushed
the
path-resolution-fixes
branch
from
b483c80
to
a763663
Compare
|
Rebased this on top of your change and fixed a bug that broke Steam |
If the guest tries to close the rootfs fd, ignore it. This is usually over-eager fd cleanup code before exec. Since the rootfs fd is O_CLOEXEC, it will be closed on execve anyway. Fixes the Proton wrapper closing the rootfs fd, which was harmless since the execve codepath didn't use it, but becomes a problem after the subsequent patch changes GetEmulatedPath to call GetEmulatedFDPath. Signed-off-by: Asahi Lina <[email protected]>
asahilina
force-pushed
the
path-resolution-fixes
branch
from
a763663
to
0c46244
Compare
|
Added another patch to refuse to close the RootFS FD. This was already a hidden potential problem, but more specifically regresses Proton with the change to |
Non-rootfs paths might contain a path component that links to the rootfs. This is the case with Wine's `dosdevices/z:` which links to /. To handle this, we need to trace the path component by component and resolve each potential symlink (except the last one, which is already handled by the existing logic if FollowSymlink is true). Signed-off-by: Asahi Lina <[email protected]>
asahilina
force-pushed
the
path-resolution-fixes
branch
from
0c46244
to
c66eb82
Compare
|
Fixed the formatting (I think) |
fyi, this command can reformat the project for you |
Sonicadvance1
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Sep 30, 2024
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
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Oct 4, 2024
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
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Oct 4, 2024
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
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Oct 4, 2024
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
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Oct 6, 2024
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
added a commit
to Sonicadvance1/FEX
that referenced
this pull request
Oct 11, 2024
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.
|
This seems plausible to me but @Sonicadvance1 should probably review |
|
What's the path (heh) forward for this? Does a proper fix need more looking into or are we considering integrating this PR as-is? I have reservations against the latter, but not sure they need to be elaborated. |
|
My main worry about this is the performance implications of doing a lot more syscalls for some things, but I don't know that that is solvable without a major overhaul introducing caching or something like that...
What I was going to explore next is actually completely bypassing this whole mess and introducing an alternate mode where FEX *only* does RootFS lookups, using a kernel-side overlayfs and overlaid mounts to create the complete x86 view of the filesystem. I suspect that'll work better for our (muvm) use case and solve a large number of path resolution/rootfs related issues in FEX all at once (there are just about enough tricks with path resolution and POSIX/Linux syscalls available to make path resolution correctly implementable in this scenario, without any userspace path resolution at the component level).
But that only works for that setup. Assuming there isn't interest in moving to that solution fully as a baseline, I imagine we still want to improve the internal overlay logic as in this PR. And I think for that I need some FEX folks to weigh in, since I don't really know where to go from here.
…On October 23, 2024 8:17:21 PM GMT+02:00, Tony Wasserka ***@***.***> wrote:
What's the path (heh) forward for this? Does a proper fix need more looking into or are we considering integrating this PR as-is? I have reservations against the latter, but not sure they need to be elaborated.
--
Reply to this email directly or view it on GitHub:
#3831 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.