[sanitizers_common] when spawning a subprocess for symbolizers, use posix_spawn instead of fork()

[alex]

Summary

Fixes: https://github.com/google/sanitizers/issues/979

Note that I can't seem to get the tests running locally, so I haven't been able to properly verify this!

[eugenis]

It appears that Android plans to support posix_spawn in P release:

https://android.googlesource.com/platform/bionic/+/master/libc/libc.map.txt#1385

This code needs to work in M, at least.

Sorry I did not notice this before. Try vfork?

[alex]

In D48451#1140158, @eugenis wrote:

Sorry I did not notice this before. Try vfork?

Simply using vfork instead of fork is probably a no-go: the vfork's man page (on macOS, I haven't verified the panoply of other BSDs) says that EINVAL is returned when "A system call other than _exit() or execve() (or libc functions that make no system calls other than those) is called following calling a vfork() call." That is, on platforms with this constraint using vfork where we currently use fork will fail, because we use other syscalls to do things like setup the file descriptor table.

• One option would be to simply use vfork on Linux only (where it does not have this restriction), and fork on all other platforms; this probably has the smallest diff.
• Another option would be to use posix_spawn on every platform except Android older than P and retain the existing fork-based implementation for them; this has a larger diff, but maybe in some glorious future we can delete the fall-back implementation for Android and end up with less code. This also let's us avoid calling vfork directly, which I have confidence is safe here, but which nonetheless has relatively fiddly semantics.

Do you have a preference between these two?

[eugenis]

In D48451#1140698, @alex wrote:

Simply using vfork instead of fork is probably a no-go: the vfork's man page (on macOS, I haven't verified the panoply of other BSDs) says that EINVAL is returned when "A system call other than _exit() or execve() (or libc functions that make no system calls other than those) is called following calling a vfork() call." That is, on platforms with this constraint using vfork where we currently use fork will fail, because we use other syscalls to do things like setup the file descriptor table.

Really? Is that actually the case in practice, or is the man page too cautious? Cloning file descriptor tables should not make vfork any more expensive, and without that it is kinda useless. That's a pity.

I'd prefer to go with the smallest diff for now.

[alex]

Good call actually checking the manpage... both dup2 and close seem to work perfectly fine in the child after vfork. I'll update the patch accordingly.

[alex]

• Simply use vfork everywhere, the macOS manpage does not describe it's semantics correctly.

[alex]

• Query for sysconf open max before vforking

[eugenis]

Looks great.

[alex]

Fantastic, thanks for the review. (As an FYI I'm not an llvm committer, so I can't land this myself).

[eugenis]

No problem, I can land it for you. Thanks for the patch!

[eugenis]

Tests are crashing with corrupted stack.

lib/sanitizer_common/sanitizer_linux.cc

800

You can't return from a function that calls vfork() in the child process. It will destroy the stack frame in the parent process.

[kubamracek]

I've talked to our runtime and kernel folks and we should not be using vfork on Darwin. Even when the behavior you tested seems to work today, it might break tomorrow. Please opt out Darwin from vfork.

Also, it seems best to use posix_spawn wherever it's available. If Android doesn't have it, we should only fallback to fork/vfork on Android.

[eugenis]

OK. It looks like internal_vfork has to be implemented in platform-specific assembly, which is was to much hassle.
Let's do posix_spawn, and fall back to fork on android < P.

[kubamracek]

Also, FYI, this is a related attempt of using posix_spawn on Darwin: https://reviews.llvm.org/D40032.

[alex]

• Switch back to using posix_spawn everywhere but Android

[kubamracek]

Thanks!

As a suggestion (perhaps for a future patch, not required to fix now): Could we detect the Android version at runtime and call posix_spawn on newer Android versions (where we know it's available)?

[alex]

Yes, I imagine such a thing is possible. It may also be detect at compile time which version of the SDK it's targetting, but I don't know the Android SDK (NDK?) well enough to say how.

[alex]

• Use posix_spawn based implementation on r28+ for Android

[eugenis]

lib/sanitizer_common/sanitizer_posix_libcdep.cc

440

This check can be done at runtime by using weak declarations.

456

Do you need to destroy the file_actions object?

[alex]

lib/sanitizer_common/sanitizer_posix_libcdep.cc

440

Hmm, it's kind of a pain because there's several functions involved, and also struct declarations. How important is it to do this at runtime?

[alex]

• Simply use vfork everywhere, the macOS manpage does not describe it's semantics correctly.
• Query for sysconf open max before vforking
• Switch back to using posix_spawn everywhere but Android
• Use posix_spawn based implementation on r28+ for Android
• Destroy file_actions

[eugenis]

These are pointers to opaque structs, aka void *.
NDK packs a single asan library built for the lowest supported API level. It will basically never reach 28.

On the other hand, no one else has complained about fork() so far.

[alex]

Would you be ok landing this with the current approach (+/- any other feedback of course), and filing a follow-up bug to switch it to a runtime check?

[eugenis]

OK, sounds fine.

lib/sanitizer_common/sanitizer_posix_libcdep.cc

493

this could be moved to file_closer

[alex]

• Use at_scope_exit to simplify destroying the posix_spawn_file_actions

[alex]

Do you need any additional changes from me?

[eugenis]

I'm fine with this.
Kuba?

[kubamracek]

LGTM. Thanks for addressing the comments!

[eugenis]

Tests hang with this change when output is redirected to a pipe. The test binary exits OK, but it looks like the process on the receiving end never gets an EOF.
Sorry, I don't have time to think about this now.
Please learn how to run tests and debug this.

lib/sanitizer_common/sanitizer_linux.cc

 int internal_fork() {
 #if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
   return internal_syscall(SYSCALL(clone), SIGCHLD, 0);
 #else
   return internal_syscall(SYSCALL(fork));
 #endif
 }
 
+int internal_vfork() {
+#if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
+  return internal_syscall(SYSCALL(clone), CLONE_VM | CLONE_VFORK | SIGCHLD, 0);
+#else
+  return internal_syscall(SYSCALL(vfork));
+#endif
+}
+
 #if SANITIZER_LINUX
 #define SA_RESTORER 0x04000000
 // Doesn't set sa_restorer if the caller did not set it, so use with caution
 //(see below).
 int internal_sigaction_norestorer(int signum, const void *act, void *oldact) {
   __sanitizer_kernel_sigaction_t k_act, k_oldact;
   internal_memset(&k_act, 0, sizeof(__sanitizer_kernel_sigaction_t));
   internal_memset(&k_oldact, 0, sizeof(__sanitizer_kernel_sigaction_t));

lib/sanitizer_common/sanitizer_mac.cc

 extern "C" pid_t __fork(void) SANITIZER_WEAK_ATTRIBUTE;
 
 int internal_fork() {
   if (&__fork)
     return __fork();
   return fork();
 }
 
+int internal_vfork() {
+  return vfork();
+}
+
 int internal_forkpty(int *amaster) {
   int master, slave;
   if (openpty(&master, &slave, nullptr, nullptr, nullptr) == -1) return -1;
   int pid = internal_fork();
   if (pid == -1) {
     close(master);
     close(slave);
     return -1;

lib/sanitizer_common/sanitizer_posix.h

 uptr internal_unlink(const char *path);
 uptr internal_rename(const char *oldpath, const char *newpath);
 uptr internal_lseek(fd_t fd, OFF_T offset, int whence);
 
 uptr internal_ptrace(int request, int pid, void *addr, void *data);
 uptr internal_waitpid(int pid, int *status, int options);
 
 int internal_fork();
+int internal_vfork();
 int internal_forkpty(int *amaster);
 
 // These functions call appropriate pthread_ functions directly, bypassing
 // the interceptor. They are weak and may not be present in some tools.
 SANITIZER_WEAK_ATTRIBUTE
 int real_pthread_create(void *th, void *attr, void *(*callback)(void *),
                         void *param);
 SANITIZER_WEAK_ATTRIBUTE

lib/sanitizer_common/sanitizer_posix_libcdep.cc

if (stacksize < minstacksize) {
     }
   }
 }
 #endif // !SANITIZER_GO
 
 pid_t StartSubprocess(const char *program, const char *const argv[],
                       fd_t stdin_fd, fd_t stdout_fd, fd_t stderr_fd) {
   auto file_closer = at_scope_exit([&] {
     if (stdin_fd != kInvalidFd) {
       internal_close(stdin_fd);
     }
     if (stdout_fd != kInvalidFd) {
       internal_close(stdout_fd);
     }
     if (stderr_fd != kInvalidFd) {
       internal_close(stderr_fd);
     }
   });
 
-  int pid = internal_fork();
+  int max_fds = sysconf(_SC_OPEN_MAX);
+
+  int pid = internal_vfork();
 
   if (pid < 0) {
     int rverrno;
     if (internal_iserror(pid, &rverrno)) {
-      Report("WARNING: failed to fork (errno %d)\n", rverrno);
+      Report("WARNING: failed to vfork (errno %d)\n", rverrno);
     }
     return pid;
   }
 
   if (pid == 0) {
     // Child subprocess
     if (stdin_fd != kInvalidFd) {
       internal_close(STDIN_FILENO);
       internal_dup2(stdin_fd, STDIN_FILENO);
       internal_close(stdin_fd);
     }
     if (stdout_fd != kInvalidFd) {
       internal_close(STDOUT_FILENO);
       internal_dup2(stdout_fd, STDOUT_FILENO);
       internal_close(stdout_fd);
     }
     if (stderr_fd != kInvalidFd) {
       internal_close(STDERR_FILENO);
       internal_dup2(stderr_fd, STDERR_FILENO);
       internal_close(stderr_fd);
     }
 
-    for (int fd = sysconf(_SC_OPEN_MAX); fd > 2; fd--) internal_close(fd);
+    for (int fd = max_fds; fd > 2; fd--) internal_close(fd);
 
     execv(program, const_cast<char **>(&argv[0]));
     internal__exit(1);
   }
 
   return pid;
 }
 
 bool IsProcessRunning(pid_t pid) {
   int process_status;
   uptr waitpid_status = internal_waitpid(pid, &process_status, WNOHANG);
   int local_errno;
   if (internal_iserror(waitpid_status, &local_errno)) {
     VReport(1, "Waiting on the process failed (errno %d).\n", local_errno);
     return false;
   }
   return waitpid_status == 0;
 }
 
 int WaitForProcess(pid_t pid) {