[sanitizers] extracted process management functions

[aizatsky]

nits

[aizatsky]

Updated test.

[samsonov]

lib/sanitizer_common/sanitizer_common.h

283

Why do you check if they are negative, instead of checking if they are specified (i.e. not kInvalidFd)?

286

It's unclear that program should be absolute path. Either add a comment, or rename variable (e.g. "filename").

764

Why is fn_ public?

767

Why do you need this?

773

You can add a AutoFileCloser as well (takes fd in a constructor, and close file in dtor if fd is valid).
FTR there is ScopedHandle in lib/sanitizer_common/sanitizer_symbolizer_win.cc you might want to generalize on.

lib/sanitizer_common/sanitizer_linux_libcdep.cc

550

This function must work on all POSIX platforms, as SymbolizerProcess::StartSymbolizerSubprocess from sanitizer_symbolizer_posix_libcdep.cc is used there. You might need to move this function (and IsProcessRunning below)
to sanitizer_posix_libcdep.cc instead.

lib/sanitizer_common/sanitizer_mac.cc

673

See above: delete this.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

142

Looks like it makes more sense to do const_cast in the StartSubprocess function.

146

Shouldn't this be outfd[1]? (outfd[0] will be closed by StartSubprocess).

lib/sanitizer_common/sanitizer_win.cc

780

Please note that this should be implemented based on SymbolizerProcess::StartSymbolizerSubprocess from lib/sanitizer_common/sanitizer_symbolizer_win.cc.

[aizatsky]

review

[aizatsky]

Thanks for reviewing. PTAL.

lib/sanitizer_common/sanitizer_common.h

283

Why not? Valid descriptors are >=0 and I really don't like special values.

286

It shouldn't. It could be relative to current dir which is perfectly fine. Renamed to "filename" though I can't say I like it.

767

Because otherwise it is impossible to use AutoRunner with lambdas.

773

Code size overhead was Kostya's primary concern. I don't think AutoFileCloser is generic enough. Plus I'd hate checking for kInvalidFd in it.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

142

I'd actually like a little advice here.

Here's how execve is defined in standard:

int execv(const char *path, char *const argv[]);

So argv points to mutable strings. I don't actually understand why. Is it a standard oversight? WDYT?

I'd be happy to define argv in StartSubprocess as "const char *const argv[];" if you thinks it is ok to cast const off later.

[samsonov]

lib/sanitizer_common/sanitizer_common.h

283

But you *are* using "special values", harcoding the assumption that valid file descriptors are positive, and InvalidFd is negative.

This is not always so, e.g. on Windows fd_t is void*, so ">=0" check would always pass. It's better to use a named constant kInvalidFd than literal 0.

286

... but you call FileExists in the implementation? So, e.g. this wouldn't work: StartSubprocess("llvm-symbolizer", argv, ....);

767

Ack.

773

... but you check for kInvalidFd three times in the implementation, though. Anyway, I don't have a strong opinion here.

lib/sanitizer_common/sanitizer_posix_libcdep.cc

324 ↗

(On Diff #45922)

Minor nit: we tend to catenate these to stdin, stdout etc.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

142

Yeah, let's do the latter for now.

[aizatsky]

review

[aizatsky]

All done. PTAL.

lib/sanitizer_common/sanitizer_common.h

283

I didn't know that. Using kInvalidFd.

286

Thought about this. Removed FileExists. (I used it to debug things).

773

Let's see how this one would be used. If we end up using it only for files - I'll create another utility.

[filcab]

Thanks for working on this.

lib/sanitizer_common/sanitizer_common.h

284

Nit: s/would/will/g

285

We should probably add that all fd after STDERR_FILENO get closed.

290

Should this argument (and StartSubprocess' return value) be something else, since, as Alexey said, "on Windows fd_t is void*"?
Probably either a typedef or just a uptr?

761

I'd prefer a more descriptive name (RunOnDestruction or something), even if it's longer. It seems it's mostly used in utility functions (at_scope_exit), which also makes a longer name a smaller problem.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

142

Nit: I think we usually have the comment before the parameter.

[samsonov]

LGTM after addressing existing comments. Thanks!

lib/sanitizer_common/sanitizer_common.h

284

"will be redirected"

290

process id is not fd_t. But I agree that uptr might be more appropriate. We use it for ReportFile::fd_pid, for example.

lib/sanitizer_common/sanitizer_posix_libcdep.cc

324 ↗

(On Diff #45936)

Please match arg names to function declaration. Also (sorry for not being clear) I meant stdin_fd, stdout_fd, stderr_fd to avoid confusing with predefined values of the same name in headers.

[aizatsky]

review

[aizatsky]

All done. Thanks a lot for review. Running check-all and submitting.

lib/sanitizer_common/sanitizer_common.h

290

Introduced pid_t typedef.

lib/sanitizer_common/sanitizer_common.h

 void DumpProcessMap();
 bool FileExists(const char *filename);
 const char *GetEnv(const char *name);
 bool SetEnv(const char *name, const char *value);
 const char *GetPwd();
 char *FindPathToBinary(const char *name);
 bool IsPathSeparator(const char c);
 bool IsAbsolutePath(const char *path);
+// Starts a subprocess and returs its pid.
+// If *_fd parameters are >=0 their corresponding input/output
+// streams would be redirect to the file. The files would always be closed
+// in parent process even in case of an error.
+int StartSubprocess(const char *program, char *const argv[],
+                    fd_t std_in_fd = kInvalidFd, fd_t std_out_fd = kInvalidFd,
+                    fd_t std_err_fd = kInvalidFd);
+// Checks if specified process is still running
+bool IsProcessRunning(int pid);
 
 u32 GetUid();
 void ReExec();
 char **GetArgv();
 void PrintCmdline();
 bool StackSizeIsUnlimited();
 void SetStackSizeLimitInBytes(uptr limit);
 bool AddressSpaceIsUnlimited();
struct SignalContext {
   static SignalContext Create(void *siginfo, void *context);
 };
 
 void GetPcSpBp(void *context, uptr *pc, uptr *sp, uptr *bp);
 
 void DisableReexec();
 void MaybeReexec();
 
+template <typename Fn>
+struct AutoRunner {
+  explicit AutoRunner(Fn fn) : fn_(fn) {}
+  ~AutoRunner() { fn_(); }
+  Fn fn_;
+};
+
+// A simple scope guard. Usage:
+// auto cleanup = at_scope_exit([]{ do_cleanup; });
+template <typename Fn>
+AutoRunner<Fn> at_scope_exit(Fn fn) {
+  return AutoRunner<Fn>(fn);
+}
+
 }  // namespace __sanitizer
 
 inline void *operator new(__sanitizer::operator_new_size_type size,
                           __sanitizer::LowLevelAllocator &alloc) {
   return alloc.Allocate(size);
 }
 
 struct StackDepotStats {
   uptr n_uniq_ids;
   uptr allocated;
 };
 
 #endif  // SANITIZER_COMMON_H

lib/sanitizer_common/sanitizer_common.cc

while (templ && templ[0]) {
     asterisk = false;
   }
   return true;
 }
 
 static const char kPathSeparator = SANITIZER_WINDOWS ? ';' : ':';
 
 char *FindPathToBinary(const char *name) {
+  if (FileExists(name)) {
+    return internal_strdup(name);
+  }
+
   const char *path = GetEnv("PATH");
   if (!path)
     return nullptr;
   uptr name_len = internal_strlen(name);
   InternalScopedBuffer<char> buffer(kMaxPathLength);
   const char *beg = path;
   while (true) {
     const char *end = internal_strchrnul(beg, kPathSeparator);

lib/sanitizer_common/sanitizer_linux.h

 // Linux-only syscalls.
 #if SANITIZER_LINUX
 uptr internal_prctl(int option, uptr arg2, uptr arg3, uptr arg4, uptr arg5);
 // Used only by sanitizer_stoptheworld. Signal handlers that are actually used
 // (like the process-wide error reporting SEGV handler) must use
 // internal_sigaction instead.
 int internal_sigaction_norestorer(int signum, const void *act, void *oldact);
 void internal_sigdelset(__sanitizer_sigset_t *set, int signum);
+uptr internal_execve(const char *filename, char *const argv[],
+                     char *const envp[]);
 #if defined(__x86_64__) || defined(__mips__) || defined(__aarch64__) \
   || defined(__powerpc64__)
 uptr internal_clone(int (*fn)(void *), void *child_stack, int flags, void *arg,
                     int *parent_tidptr, void *newtls, int *child_tidptr);
 #endif
 #endif  // SANITIZER_LINUX
 
 // This class reads thread IDs from /proc/<pid>/task using only syscalls.

lib/sanitizer_common/sanitizer_linux_libcdep.cc

 #if SANITIZER_ANDROID || SANITIZER_FREEBSD
 #include <dlfcn.h>  // for dlsym()
 #endif
 
 #include <link.h>
 #include <pthread.h>
 #include <signal.h>
 #include <sys/resource.h>
+#include <sys/wait.h>
 
 #if SANITIZER_FREEBSD
 #include <pthread_np.h>
 #include <osreldate.h>
 #define pthread_getattr_np pthread_attr_get_np
 #endif
 
 #if SANITIZER_LINUX

 #endif
 }
 
 void LogMessageOnPrintf(const char *str) {
   if (common_flags()->log_to_syslog && ShouldLogAfterPrintf())
     WriteToSyslog(str);
 }
 
+int StartSubprocess(const char *program, char *const argv[], fd_t std_in_fd,
+                    fd_t std_out_fd, fd_t std_err_fd) {
+  auto file_closer = at_scope_exit([&] {
+    if (std_in_fd >= 0) {
+      internal_close(std_in_fd);
+    }
+    if (std_out_fd >= 0) {
+      internal_close(std_out_fd);
+    }
+    if (std_err_fd >= 0) {
+      internal_close(std_err_fd);
+    }
+  });
+
+  if (!FileExists(program)) {
+    Report("WARNING: Program %s not found!\n", program);
+    return -1;
+  }
+
+  int pid = internal_fork();
+
+  if (pid < 0) {
+    int rverrno;
+    if (internal_iserror(pid, &rverrno)) {
+      Report("WARNING: failed to fork (errno %d)\n", rverrno);
+    }
+    return pid;
+  }
+
+  if (pid == 0) {
+    // Child subprocess
+    if (std_in_fd >= 0) {
+      internal_close(STDIN_FILENO);
+      internal_dup2(std_in_fd, STDIN_FILENO);
+      internal_close(std_in_fd);
+    }
+    if (std_out_fd >= 0) {
+      internal_close(STDOUT_FILENO);
+      internal_dup2(std_out_fd, STDOUT_FILENO);
+      internal_close(std_out_fd);
+    }
+    if (std_err_fd >= 0) {
+      internal_close(STDERR_FILENO);
+      internal_dup2(std_err_fd, STDERR_FILENO);
+      internal_close(std_err_fd);
+    }
+
+    for (int fd = sysconf(_SC_OPEN_MAX); fd > 2; fd--) internal_close(fd);
+
+    internal_execve(program, argv, nullptr);
+    internal__exit(1);
+  }
+
+  return pid;
+}
+
+bool IsProcessRunning(int 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;
+}
+
 #endif // SANITIZER_LINUX
 
 } // namespace __sanitizer
 
 #endif // SANITIZER_FREEBSD || SANITIZER_LINUX

lib/sanitizer_common/sanitizer_mac.cc

void MaybeReexec() {
   if (new_env_pos == new_env + env_name_len + 1) new_env = NULL;
   LeakyResetEnv(kDyldInsertLibraries, new_env);
 }
 
 char **GetArgv() {
   return *_NSGetArgv();
 }
 
+int StartSubprocess(const char *program, char *const argv[], fd_t std_in_fd,
+                    fd_t std_out_fd, fd_t std_err_fd) {
+  // FIXME: implement on this platform.
+  return -1;
+}
+
+bool IsProcessRunning(int pid) {
+  // FIXME: implement on this platform.
+  return false;
+}
+
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_MAC

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

for (int i = 0; i < 5; i++) {
           }
           break;
         }
       }
     }
     CHECK(infd);
     CHECK(outfd);
 
-    // Real fork() may call user callbacks registered with pthread_atfork().
-    pid = internal_fork();
-    if (pid == -1) {
-      // Fork() failed.
+    const char *argv[kArgVMax];
+    GetArgV(path_, argv);
+    pid = StartSubprocess(path_, const_cast<char **>(&argv[0]),
+                          outfd[0] /* stdin */, infd[1] /* stdout */);
+    if (pid < 0) {
       internal_close(infd[0]);
-      internal_close(infd[1]);
       internal_close(outfd[0]);
-      internal_close(outfd[1]);
-      Report("WARNING: failed to fork external symbolizer "
-             " (errno: %d)\n", errno);
       return false;
-    } else if (pid == 0) {
-      // Child subprocess.
-      internal_close(STDOUT_FILENO);
-      internal_close(STDIN_FILENO);
-      internal_dup2(outfd[0], STDIN_FILENO);
-      internal_dup2(infd[1], STDOUT_FILENO);
-      internal_close(outfd[0]);
-      internal_close(outfd[1]);
-      internal_close(infd[0]);
-      internal_close(infd[1]);
-      for (int fd = sysconf(_SC_OPEN_MAX); fd > 2; fd--)
-        internal_close(fd);
-      const char *argv[kArgVMax];
-      GetArgV(path_, argv);
-      execv(path_, const_cast<char **>(&argv[0]));
-      internal__exit(1);
     }
 
-    // Continue execution in parent process.
-    internal_close(outfd[0]);
-    internal_close(infd[1]);
     input_fd_ = infd[0];
     output_fd_ = outfd[1];
   }
 
   // Check that symbolizer subprocess started successfully.
-  int pid_status;
   SleepForMillis(kSymbolizerStartupTimeMillis);
-  int exited_pid = waitpid(pid, &pid_status, WNOHANG);
-  if (exited_pid != 0) {
+  if (!IsProcessRunning(pid)) {
     // Either waitpid failed, or child has already exited.
     Report("WARNING: external symbolizer didn't start up correctly!\n");
     return false;
   }
 
   return true;
 }
 

lib/sanitizer_common/sanitizer_win.cc

void MaybeReexec() {
   // No need to re-exec on Windows.
 }
 
 char **GetArgv() {
   // FIXME: Actually implement this function.
   return 0;
 }
 
+int StartSubprocess(const char *program, char *const argv[], fd_t std_in_fd,
+                    fd_t std_out_fd, fd_t std_err_fd) {
+  // FIXME: implement on this platform.
+  return -1;
+}
+
+bool IsProcessRunning(int pid) {
+  // FIXME: implement on this platform.
+  return false;
+}
+
 }  // namespace __sanitizer
 
 #endif  // _WIN32

lib/sanitizer_common/tests/sanitizer_linux_test.cc

 #if defined(__mips64)
 // Effectively, this is a test for ThreadDescriptorSize() which is used to
 // compute ThreadSelf().
 TEST(SanitizerLinux, ThreadSelfTest) {
   ASSERT_EQ(pthread_self(), ThreadSelf());
 }
 #endif
 
+TEST(SanitizerCommon, StartSubprocessTest) {
+  int pipe_fds[2];
+  ASSERT_EQ(0, pipe(pipe_fds));
+  const char *argv[] = {"/bin/sh", "-c", "echo -n 'hello'"};
+  int pid = StartSubprocess("/bin/sh", const_cast<char **>(&argv[0]),
+                            kInvalidFd /* stdin */, pipe_fds[1] /* stdout */);
+  ASSERT_GT(pid, 0);
+
+  // wait for process to finish.
+  while (IsProcessRunning(pid)) {
+  }
+  ASSERT_FALSE(IsProcessRunning(pid));
+
+  char buffer[256];
+  char *ptr = buffer;
+  uptr bytes_read;
+  while (ReadFromFile(pipe_fds[0], ptr, 256, &bytes_read)) {
+    if (!bytes_read) {
+      break;
+    }
+    ptr += bytes_read;
+  }
+  ASSERT_EQ(5, ptr - buffer);
+  *ptr = 0;
+  ASSERT_EQ(0, strcmp(buffer, "hello")) << "Buffer: " << buffer;
+  internal_close(pipe_fds[0]);
+}
+
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_LINUX