[sanitizers] extracted process management functions

[aizatsky]

nits

[aizatsky]

Updated test.

[samsonov]

lib/sanitizer_common/sanitizer_common.h

283 ↗

(On Diff #45912)

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

286 ↗

(On Diff #45912)

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

764 ↗

(On Diff #45912)

Why is fn_ public?

767 ↗

(On Diff #45912)

Why do you need this?

773 ↗

(On Diff #45912)

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 ↗

(On Diff #45912)

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 ↗

(On Diff #45912)

See above: delete this.

lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc

142 ↗

(On Diff #45912)

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

146 ↗

(On Diff #45912)

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

lib/sanitizer_common/sanitizer_win.cc

780 ↗

(On Diff #45912)

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 ↗

(On Diff #45912)

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

286 ↗

(On Diff #45912)

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 ↗

(On Diff #45912)

Because otherwise it is impossible to use AutoRunner with lambdas.

773 ↗

(On Diff #45912)

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 ↗

(On Diff #45912)

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 ↗

(On Diff #45922)

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 ↗

(On Diff #45922)

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

768 ↗

(On Diff #45922)

Ack.

774 ↗

(On Diff #45922)

... 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 ↗

(On Diff #45922)

Yeah, let's do the latter for now.

[aizatsky]

review

[aizatsky]

All done. PTAL.

lib/sanitizer_common/sanitizer_common.h

283 ↗

(On Diff #45922)

I didn't know that. Using kInvalidFd.

286 ↗

(On Diff #45922)

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

774 ↗

(On Diff #45922)

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 ↗

(On Diff #45936)

Nit: s/would/will/g

285 ↗

(On Diff #45936)

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

290 ↗

(On Diff #45936)

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 ↗

(On Diff #45936)

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 ↗

(On Diff #45936)

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 ↗

(On Diff #45936)

"will be redirected"

290 ↗

(On Diff #45936)

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 ↗

(On Diff #45936)

Introduced pid_t typedef.

compiler-rt/trunk/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 not kInvalidFd their corresponding input/output
+// streams will be redirect to the file. The files will always be closed
+// in parent process even in case of an error.
+// The child process will close all fds after STDERR_FILENO
+// before passing control to a program.
+pid_t StartSubprocess(const char *filename, const char *const argv[],
+                      fd_t stdin_fd = kInvalidFd, fd_t stdout_fd = kInvalidFd,
+                      fd_t stderr_fd = kInvalidFd);
+// Checks if specified process is still running
+bool IsProcessRunning(pid_t 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>
+class RunOnDestruction {
+ public:
+  explicit RunOnDestruction(Fn fn) : fn_(fn) {}
+  ~RunOnDestruction() { fn_(); }
+
+ private:
+  Fn fn_;
+};
+
+// A simple scope guard. Usage:
+// auto cleanup = at_scope_exit([]{ do_cleanup; });
+template <typename Fn>
+RunOnDestruction<Fn> at_scope_exit(Fn fn) {
+  return RunOnDestruction<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

compiler-rt/trunk/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);

compiler-rt/trunk/lib/sanitizer_common/sanitizer_internal_defs.h

 // On Windows, files are HANDLE, which is a synonim of void*.
 // Use void* to avoid including <windows.h> everywhere.
 typedef void* fd_t;
 typedef unsigned error_t;
 #else
 typedef int fd_t;
 typedef int error_t;
 #endif
+typedef int pid_t;
 
 // WARNING: OFF_T may be different from OS type off_t, depending on the value of
 // _FILE_OFFSET_BITS. This definition of OFF_T matches the ABI of system calls
 // like pread and mmap, as opposed to pread64 and mmap64.
 // FreeBSD, Mac and Linux/x86-64 are special.
 #if SANITIZER_FREEBSD || SANITIZER_MAC || \
   (SANITIZER_LINUX && defined(__x86_64__))
 typedef u64 OFF_T;

compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix.h

#define DEFINE_REAL_PTHREAD_FUNCTIONS                                          \
   }                                                                            \
   }  // namespace __sanitizer
 
 int my_pthread_attr_getstack(void *attr, void **addr, uptr *size);
 
 int internal_sigaction(int signum, const void *act, void *oldact);
 void internal_sigfillset(__sanitizer_sigset_t *set);
 
+uptr internal_execve(const char *filename, char *const argv[],
+                     char *const envp[]);
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_POSIX_H

compiler-rt/trunk/lib/sanitizer_common/sanitizer_posix_libcdep.cc

 #include <pthread.h>
 #include <signal.h>
 #include <stdlib.h>
 #include <sys/mman.h>
 #include <sys/resource.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <unistd.h>
 
 #if SANITIZER_FREEBSD
 // The MAP_NORESERVE define has been removed in FreeBSD 11.x, and even before
 // that, it was never implemented.  So just define it to zero.
 #undef  MAP_NORESERVE
 #define MAP_NORESERVE 0
 #endif
if (!stack_set) {
       Printf("Sanitizer: pre-allocated stack size is insufficient: "
              "%zu < %zu\n", stacksize, minstacksize);
       Printf("Sanitizer: pthread_create is likely to fail.\n");
     }
   }
 }
 #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();
+
+  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 (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);
+
+    internal_execve(program, const_cast<char **>(&argv[0]), nullptr);
+    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;
+}
+
 } // namespace __sanitizer
 
 #endif // SANITIZER_POSIX

compiler-rt/trunk/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_, argv, /* stdin */ outfd[0],
+                          /* stdout */ infd[1]);
+    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;
 }
 

compiler-rt/trunk/lib/sanitizer_common/sanitizer_win.cc

void MaybeReexec() {
   // No need to re-exec on Windows.
 }
 
 char **GetArgv() {
   // FIXME: Actually implement this function.
   return 0;
 }
 
+pid_t StartSubprocess(const char *program, const char *const argv[],
+                      fd_t stdin_fd, fd_t stdout_fd, fd_t stderr_fd) {
+  // FIXME: implement on this platform
+  // Should be implemented based on
+  // SymbolizerProcess::StarAtSymbolizerSubprocess
+  // from lib/sanitizer_common/sanitizer_symbolizer_win.cc.
+  return -1;
+}
+
+bool IsProcessRunning(pid_t pid) {
+  // FIXME: implement on this platform.
+  return false;
+}
+
 }  // namespace __sanitizer
 
 #endif  // _WIN32

compiler-rt/trunk/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