[posix] remove setgid(2)/setsid(2) from ProcessSpawner (#146)

## Purpose
Remove the call to `setsid(2)` on every process spawn and make debug
inferior process setup items specific to `DebugSession` launched
process. This PR makes ds2 process launch behavior consistent with
lldb-server. Addresses issue #137 along with PR #143.

## Overview
* Eliminate the call to `setsid(2)` from `ProcessSpawner`
* Call `setpgid(2)` from the `preExecAction` callback for `DebugSession`
process launch. Do not make this call for generic process launch (which
again matches `lldb-server`'s behavior).
* Move the call to `setgid(2)` from the `ProcessSpawner` implementation
into the `preExecAction` callback for `DebugSession` process launch.
This move avoids making the call when launching processes that are not
debug targets.

## Background
It is unclear why there was a call to `setsid(2)` on every process
spawn. The implementation of `lldb-server` does not do this.

When launching a new gdb server instance of ds2 via the
`qLaunchGDBServer`, it always passes along the `--setsid` argument so
spawned gdb servers already call `setsid(2)`. When spawning other
processes, including debug inferiors, there is no need to make this
call; doing so can cause some test failures/hangs due to undelivered
signals because the inferior is in a different session.

## Validation
Once this PR and #143 are merged, most of the TestGdbRemote* and
TestLldbGdbServer* lldb tests succeed with ds2 running on Android and
Linux and can be enabled in CI. They do not all pass with this PR alone.

Author: andrurogerz

Sources/Host/POSIX/ProcessSpawner.cpp

@@ -415,79 +415,75 @@ ErrorCode ProcessSpawner::run(std::function<bool()> preExecAction) {
   }
 
   if (_pid == 0) {
-    if (::setgid(::getgid()) == 0) {
-      ::setsid();
-
-      for (size_t n = 0; n < 3; n++) {
-        switch (_descriptors[n].mode) {
-        case kRedirectConsole:
-          // do nothing
-          break;
+    for (size_t n = 0; n < 3; n++) {
+      switch (_descriptors[n].mode) {
+      case kRedirectConsole:
+        // do nothing
+        break;
 
-        case kRedirectDelegate:
-        case kRedirectTerminal:
-          //
-          // We are using the same virtual terminal for all delegate
-          // redirections, so dup2() only, do not close. We will close when all
-          // FDs have been dup2()'d.
-          //
-          ::dup2(fds[n][WR], n);
-          break;
+      case kRedirectDelegate:
+      case kRedirectTerminal:
+        //
+        // We are using the same virtual terminal for all delegate
+        // redirections, so dup2() only, do not close. We will close when all
+        // FDs have been dup2()'d.
+        //
+        ::dup2(fds[n][WR], n);
+        break;
 
-        default:
-          if (n == 0) {
-            if (fds[n][WR] != -1) {
-              ::close(fds[n][WR]);
-            }
-            ::dup2(fds[n][RD], n);
-            ::close(fds[n][RD]);
-          } else {
-            if (fds[n][RD] != -1) {
-              ::close(fds[n][RD]);
-            }
-            ::dup2(fds[n][WR], n);
+      default:
+        if (n == 0) {
+          if (fds[n][WR] != -1) {
             ::close(fds[n][WR]);
           }
-          break;
+          ::dup2(fds[n][RD], n);
+          ::close(fds[n][RD]);
+        } else {
+          if (fds[n][RD] != -1) {
+            ::close(fds[n][RD]);
+          }
+          ::dup2(fds[n][WR], n);
+          ::close(fds[n][WR]);
         }
+        break;
       }
+    }
 
-      close_terminal(term);
+    close_terminal(term);
 
-      if (!_workingDirectory.empty()) {
-        int res = ::chdir(_workingDirectory.c_str());
-        if (res != 0) {
-          return Platform::TranslateError();
-        }
+    if (!_workingDirectory.empty()) {
+      int res = ::chdir(_workingDirectory.c_str());
+      if (res != 0) {
+        return Platform::TranslateError();
       }
+    }
 
-      std::vector<char *> args;
-      args.push_back(const_cast<char *>(_executablePath.c_str()));
-      for (auto const &e : _arguments)
-        args.push_back(const_cast<char *>(e.c_str()));
-      args.push_back(nullptr);
-
-      std::vector<char *> environment;
-      for (auto const &env : _environment)
-        environment.push_back(const_cast<char *>(
-            (new std::string(env.first + '=' + env.second))->c_str()));
-      environment.push_back(nullptr);
-
-      if (!preExecAction()) {
-        DS2LOG(Error, "pre exec action failed");
-        return kErrorUnknown;
-      }
+    std::vector<char *> args;
+    args.push_back(const_cast<char *>(_executablePath.c_str()));
+    for (auto const &e : _arguments)
+      args.push_back(const_cast<char *>(e.c_str()));
+    args.push_back(nullptr);
+
+    std::vector<char *> environment;
+    for (auto const &env : _environment)
+      environment.push_back(const_cast<char *>(
+          (new std::string(env.first + '=' + env.second))->c_str()));
+    environment.push_back(nullptr);
+
+    if (!preExecAction()) {
+      DS2LOG(Error, "pre exec action failed");
+      return kErrorUnknown;
+    }
 
-      if (_shell) {
-        if (::execvp(_executablePath.c_str(), &args[0]) < 0) {
-          DS2LOG(Error, "cannot run shell command %s, error=%s",
-                 _executablePath.c_str(), strerror(errno));
-        }
-      } else {
-        if (::execve(_executablePath.c_str(), &args[0], &environment[0]) < 0) {
-          DS2LOG(Error, "cannot spawn executable %s, error=%s",
-                 _executablePath.c_str(), strerror(errno));
-        }
+    if (_shell) {
+      if (::execvp(_executablePath.c_str(), &args[0]) < 0) {
+        DS2LOG(Error, "cannot run shell command %s, error=%s",
+                _executablePath.c_str(), strerror(errno));
+      }
+    } else {
+      if (::execve(_executablePath.c_str(), &args[0], &environment[0]) < 0) {
+        DS2LOG(Error, "cannot spawn executable %s, error=%s",
+                _executablePath.c_str(), strerror(errno));
       }
     }
     ::_exit(127);

Sources/Target/POSIX/Process.cpp

@@ -148,6 +148,14 @@ ds2::Target::Process *Process::Create(ProcessSpawner &spawner) {
   auto process = ds2::make_unique<Target::Process>();
 
   if (spawner.run([&process]() {
+        // move debug targets to their own process group
+        if (::setpgid(0, 0) != 0)
+          return false;
+
+        // drop the setgid ability of debug targets
+        if (::setgid(::getgid()) != 0)
+          return false;
+
         return process->ptrace().traceMe(true) == kSuccess;
       }) != kSuccess) {
     return nullptr;