FEXLoader: Adds support for execveat with AT_EMPTY_PATH #2334

Sonicadvance1 · 2023-01-13T06:21:42Z

This is a fairly tricky edge case to support with FEX.
If execveat is used with AT_EMPTY_PATH then the application can pass an
FD to execve instead of a filename. This includes FDs that have been
deleted from the disk so the child process can't open it by filename
anymore.

To work around this limitation, we need to pass the FD to the new FEX
process and open it directly, similar to how binfmt_misc works with FDs.
The FD will get passed through environment variables, which the new
process will check for and then remove the variable from the
environment.

Lots of prickly edge cases to support here.

Without binfmt_misc:

Passes the FD to FEXLoader directly.
- Requires duplicating the FD if it has O_CLOEXEC on the FD.

With binfmt_misc:

Shebang file, pass directly to FEXLoader, just like without binfmt.
x86 ELF Files, rely on the kernel's binfmt_misc support here.
Unsupported ELF files, let kernel handle it through binfmt_misc

Argument handling:

The application can pass in no arguments.
- Means our application configurations were failing to find a config
- Also various checks in the frontend were failing.
- If opened through an FD, find the symlink for that FD for the
  application configuration instead.

Side note:
Fixed a performance issue in execve where when we were checking for file
format support. Either ELF or Shebang files, we were reading the /whole/
file upfront. We only need to read a header worth of ELF files, and only
257 bytes if it is potentially a shebang file. Should dramatically
reduce some application's execve times.

Source/Tests/LinuxSyscalls/Syscalls.cpp

neobrain

Logic here seems very intricate. I've done a first pass trying to wrap my head around it, adding a few comments with minor performance improvements and suggestions to clarify some documentation.

Source/Tests/LinuxSyscalls/Syscalls.cpp

FEXHeaderUtils/FEXHeaderUtils/SymlinkChecks.h

Source/Common/Config.cpp

neobrain · 2023-01-20T22:08:54Z

Source/Tests/LinuxSyscalls/Syscalls.cpp

+  File.seekg(0, File.end);
+  FileSize = File.tellg();
+  File.seekg(0, File.beg);
+
+  // Is the file large enough for shebang
+  if (FileSize <= 2)
+    return false;
+
+  // The maximum length of the shebang line is `#!` + 255 chars
+  std::vector<char> Header(std::min(257ul, FileSize));
+  File.read(&Header.at(0), Header.size());


Seeking twice to perform the pathological size check is probably (?) more expensive than always reading and checking later, so:

std::array<char, 257> Header; auto ChunkSize = File.read(Header.data(), Header.size());

(where the size check now happens in IsShebangFile)

Some of the logic with this comment and others conflicted. So you'll need to check that again with the changes.

Source/Tests/LinuxSyscalls/Syscalls.cpp

neobrain · 2023-01-20T22:19:10Z

Source/Tests/LinuxSyscalls/Syscalls.cpp

+    if (strcmp(pathname, "/proc/self/exe") == 0 ||
+        strcmp(pathname, "/proc/thread-self/exe") == 0 ||
+        strcmp(pathname, PidSelfPath) == 0) {
+      // If pointing to self then redirect to the application


What exactly is the scenario described here? Is it "FEX runs java, and java then calls execve on a symlink to its own executable to start the guest Java application"?

At that point, what is /proc/self/exe actually pointing to - FEX or java? Comment needs to be a bit clearer, I think.

Updated comment.

Source/Tests/LinuxSyscalls/Syscalls.h

Source/Tests/LinuxSyscalls/Syscalls.cpp

neobrain

Second pass done: I mostly understand what's happening now. The added documentation really helped!
We're getting there :)

Source/Tests/LinuxSyscalls/Syscalls.cpp

FEXHeaderUtils/FEXHeaderUtils/SymlinkChecks.h

Source/Common/Config.h

Source/Common/Config.cpp

neobrain · 2023-01-22T20:06:20Z

Source/Common/Config.cpp

+      //   - Full application path with execve (See Side Note)
+      //   - FD path with execveat and FD doesn't have an existing filesystem on the disk
+      //
+      // ProgramFDFromEnv: Arg[0] is set to Application provided data or `/dev/fd/<FD>` from above fix-up.


This is useful information, but it mixes "things that applications do" with "how they do it" and (maybe?) "how FEX handles" it. Currently it's difficult to tell which of these you're describing at which point.

If you're describing what values certain values take, I'd use "the value of X will be" over "X is set", since the latter is ambiguous about who sets it (FEX before this line? FEX after this line? The application? ...).

If you're describing values that we actively set after the comment, active phrasing ("We set"/"FEX sets") helps clarify this over passive phrasing ("is set").

Source/Tests/LinuxSyscalls/Syscalls.cpp

neobrain · 2023-01-22T20:24:35Z

Source/Tests/LinuxSyscalls/Syscalls.cpp

+    FDExecEnv = "FEX_EXECVEFD=" + std::to_string(Args.dirfd);
+
+    // Insert the FD for FEX to track.
+    EnvpArgs.emplace_back(FDExecEnv.data());


Where is this extra argument actually used?

It's used in the next FEXInterpreter instance that is getting executed.

Where can I see this in the code? Do we actively have to extract it again from envp on the other end, or does this happen indirectly?

https://github.com/FEX-Emu/FEX/pull/2334/files#diff-cd1944a762a8c051124ab5532528cddff1933f5c0d2e3a45785c36751868b244R224
We extract it from envp here.

Ah, yes - I was somehow looking for a manual envp readout as opposed to getenv. Makes sense!

Source/Tests/ELFCodeLoader2.h

Source/Common/Config.cpp

neobrain

Looks good!

Source/Common/Config.h

Fixes FEX-Emu#2136 This is a fairly tricky edge case to support with FEX. If execveat is used with AT_EMPTY_PATH then the application can pass an FD to execve instead of a filename. This includes FDs that have been deleted from the disk so the child process can't open it by filename anymore. To work around this limitation, we need to pass the FD to the new FEX process and open it directly, similar to how binfmt_misc works with FDs. The FD will get passed through environment variables, which the new process will check for and then remove the variable from the environment. Lots of prickly edge cases to support here. Without binfmt_misc: - Passes the FD to FEXLoader directly. - Requires duplicating the FD if it has O_CLOEXEC on the FD. With binfmt_misc: - Shebang file, pass directly to FEXLoader, just like without binfmt. - x86 ELF Files, rely on the kernel's binfmt_misc support here. - Unsupported ELF files, let kernel handle it through binfmt_misc Argument handling: - The application can pass in no arguments. - Means our application configurations were failing to find a config - Also various checks in the frontend were failing. - If opened through an FD, find the symlink for that FD for the application configuration instead. Side note: Fixed a performance issue in execve where when we were checking for file format support. Either ELF or Shebang files, we were reading the /whole/ file upfront. We only need to read a header worth of ELF files, and only 257 bytes if it is potentially a shebang file. Should dramatically reduce some application's execve times.

Sonicadvance1 force-pushed the fix_execveat branch 3 times, most recently from 82a2b36 to c74bf13 Compare January 13, 2023 23:58

neobrain reviewed Jan 14, 2023

View reviewed changes

Sonicadvance1 force-pushed the fix_execveat branch from c74bf13 to a958f36 Compare January 14, 2023 20:12

neobrain reviewed Jan 20, 2023

View reviewed changes

Sonicadvance1 force-pushed the fix_execveat branch 2 times, most recently from 7c68474 to 9bd0ecb Compare January 22, 2023 03:37

neobrain reviewed Jan 22, 2023

View reviewed changes

FHU: Add helpers for symlink checking

79b8442

Sonicadvance1 force-pushed the fix_execveat branch 4 times, most recently from 7e59e6c to 42c435c Compare January 22, 2023 23:12

Sonicadvance1 commented Jan 23, 2023

View reviewed changes

Source/Tests/ELFCodeLoader2.h Show resolved Hide resolved

neobrain reviewed Jan 23, 2023

View reviewed changes

Source/Common/Config.cpp Outdated Show resolved Hide resolved

neobrain approved these changes Jan 23, 2023

View reviewed changes

Source/Common/Config.h Outdated Show resolved Hide resolved

Sonicadvance1 force-pushed the fix_execveat branch from 42c435c to 472675d Compare January 23, 2023 10:07

Sonicadvance1 merged commit dcce9ad into FEX-Emu:main Jan 23, 2023

Sonicadvance1 deleted the fix_execveat branch January 23, 2023 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEXLoader: Adds support for execveat with AT_EMPTY_PATH #2334

FEXLoader: Adds support for execveat with AT_EMPTY_PATH #2334

Sonicadvance1 commented Jan 13, 2023

neobrain left a comment

neobrain Jan 20, 2023

Sonicadvance1 Jan 22, 2023

neobrain Jan 20, 2023

Sonicadvance1 Jan 22, 2023

neobrain left a comment

neobrain Jan 22, 2023

neobrain Jan 22, 2023

Sonicadvance1 Jan 22, 2023

neobrain Jan 23, 2023

Sonicadvance1 Jan 23, 2023

neobrain Jan 23, 2023

neobrain left a comment

FEXLoader: Adds support for execveat with AT_EMPTY_PATH #2334

FEXLoader: Adds support for execveat with AT_EMPTY_PATH #2334

Conversation

Sonicadvance1 commented Jan 13, 2023

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

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

neobrain left a comment

Choose a reason for hiding this comment