Use socketpair on all Unix platforms

[DemiMarie]

Summary

Using TCP sockets is insecure against local attackers, and possibly against remote attackers too (some vulnerabilities may allow tricking a browser to make a request to localhost). Use socketpair (which is immune to such attacks) on all Unix platforms.

[clayborg]

Looks fine to me, lets make sure Pavel is OK with this. On MacOS socketpair is also much faster. Please run the following command while stopped at a breakpoint with and without your change:

(lldb) process plugin packet speed-test

It will send and receive packets using a variety of sizes and report the number of packets per second.

[labath]

Have you tried that this actually works? (It doesn't work for me)

As far as I can tell, you will have to teach lldb-server to understand the --fd argument before you can flip this switch.

[clayborg]

Ah yes, I thought there was something missing...

[DemiMarie]

Make --fd an alias for --pipe

[DemiMarie]

In D33213#806807, @clayborg wrote:

Ah yes, I thought there was something missing...

I just made --fd an alias for --pipe.

[clayborg]

Let Pavel try this and verify and this is good to go.

[clayborg]

Any news on any speed changes? Please post the output of:

(lldb) process plugin packet speed-test

Both without and with this change. Nice to track any perf improvements

[labath]

This is completely wrong. The --pipe argument is used to write the port that lldb-server listens on. Making --fd an alias to that will not help.

Please make sure you test your change before submitting it for review. You should at least make sure you are able to debug a hello world program with your modifications.

[clayborg]

Before any changes are submitted, we assume you are getting a clean test suite run.

[DemiMarie]

In D33213#813037, @clayborg wrote:

Before any changes are submitted, we assume you are getting a clean test suite run.

On my system, I get build failures in clang.

[labath]

In D33213#814041, @DemiMarie wrote:

In D33213#813037, @clayborg wrote:

Before any changes are submitted, we assume you are getting a clean test suite run.

On my system, I get build failures in clang.

I suggest you write an email to cfe-dev http://lists.llvm.org/mailman/listinfo/cfe-dev and ask for help (or just google around). It's kinda hard to develop a feature without even being able to build the code :)

[clayborg]

I am not sure how you expect to submit any patches then. Patches must be tested prior to submission.

[DemiMarie]

Fix connection when there is no host:port

[DemiMarie]

Get rid of silly and bogus #error

[clayborg]

Please init fd

tools/lldb-server/lldb-gdbserver.cpp

391

This must be initialized to -1 since only the 'F' case will store to this. Pretty much all of the time random memory would be used for this and incorrectly try to use a random integer as a file descriptor.

[DemiMarie]

Initalize connection_fd

Otherwise undefined behavior ensues whenever --fd is not passed to
lldb-server -g.

[clayborg]

So where did the other changes go where we use "--fd" for non Apple builds? Did those changes get lost? They will be needed.

Are you able to run the test suite now?

[DemiMarie]

Actually use socketpair :)

[DemiMarie]

In D33213#820238, @clayborg wrote:

So where did the other changes go where we use "--fd" for non Apple builds? Did those changes get lost? They will be needed.

Are you able to run the test suite now?

I did. Not all tests passed, but none of the failures appeared to be related to this change, and the compiled lldb can debug /usr/bin/cat.

[clayborg]

I'm ok as long as Pavel is ok. Please wait for the OK from him as well.

[labath]

I am not able to test this right now. Eugene, can you take a look?

tools/lldb-server/lldb-gdbserver.cpp

241

you should set the O_CLOEXEC flag on the file descriptor, to avoid leakage.

[DemiMarie]

Set FD_CLOEXEC on the communication fd

[eugene]

LGTM. Test run on Linux is clear. I also see a bit of perf bump.

Before checking in please remove outdated comment from ProcessGDBRemote::LaunchAndConnectToDebugserver.

// Use a socketpair on Apple for now until other platforms can verify it
// works and is fast enough

[clayborg]

Remove the outdated comment and we are good to go.

[DemiMarie]

Delete misleading comment

[DemiMarie]

Fix misleading comment for real

[DemiMarie]

Does this need any further action on my part?

[clayborg]

This should be good to go. Watch the buildbots for failures.

[jingham]

Pavel & Eugene haven't marked it accepted yet, but from the comments is looks like they are both okay with the change. So it looks to me like everything is done but checking the code in.

If you have checkin privileges, then just check it in and as Greg says keep an eye on the bots. If you don't then speak up and one of us will check it in for you.

[eugene]

I did mark it Accepted.

[DemiMarie]

In D33213#873962, @jingham wrote:

Pavel & Eugene haven't marked it accepted yet, but from the comments is looks like they are both okay with the change. So it looks to me like everything is done but checking the code in.

If you have checkin privileges, then just check it in and as Greg says keep an eye on the bots. If you don't then speak up and one of us will check it in for you.

I don’t have checkin privileges. Would one of you mind checking the code in?

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

ProcessGDBRemote::EstablishConnectionIfNeeded(const ProcessInfo &process_info) {
   auto error = LaunchAndConnectToDebugserver(process_info);
   if (error.Fail()) {
     const char *error_string = error.AsCString();
     if (error_string == nullptr)
       error_string = "unable to launch " DEBUGSERVER_BASENAME;
   }
   return error;
 }
-#if defined(__APPLE__)
+#if !defined(_WIN32)
 #define USE_SOCKETPAIR_FOR_LOCAL_CONNECTION 1
 #endif
 
 #ifdef USE_SOCKETPAIR_FOR_LOCAL_CONNECTION
 static bool SetCloexecFlag(int fd) {
 #if defined(FD_CLOEXEC)
   int flags = ::fcntl(fd, F_GETFD);
   if (flags == -1)

tools/lldb-server/lldb-gdbserver.cpp

static struct option g_long_options[] = {
      'r'}, // Specify to use the native registers instead of the gdb defaults
            // for the architecture.  NOTE: this is a do-nothing arg as it's
            // behavior is default now.  FIXME remove call from lldb-platform.
     {"reverse-connect", no_argument, NULL,
      'R'}, // Specifies that llgs attaches to the client address:port rather
            // than llgs listening for a connection from address on port.
     {"setsid", no_argument, NULL,
      'S'}, // Call setsid() to make llgs run in its own session.
+    {"fd", required_argument, NULL, 'F'},
     {NULL, 0, NULL, 0}};
 
 //----------------------------------------------------------------------
 // Watch for signals
 //----------------------------------------------------------------------
 static int g_sighup_received_count = 0;
 
 #ifndef _WIN32

 }
 #endif // #ifndef _WIN32
 
 static void display_usage(const char *progname, const char *subcommand) {
   fprintf(stderr, "Usage:\n  %s %s "
                   "[--log-file log-file-name] "
                   "[--log-channels log-channel-list] "
                   "[--setsid] "
+                  "[--fd file-descriptor]"
                   "[--named-pipe named-pipe-path] "
                   "[--native-regs] "
                   "[--attach pid] "
                   "[[HOST]:PORT] "
                   "[-- PROGRAM ARG1 ARG2 ...]\n",
           progname, subcommand);
-  exit(0);
 }
 
 void handle_attach_to_pid(GDBRemoteCommunicationServerLLGS &gdb_server,
                           lldb::pid_t pid) {
   Status error = gdb_server.AttachToProcess(pid);
   if (error.Fail()) {
     fprintf(stderr, "error: failed to attach to pid %" PRIu64 ": %s\n", pid,
             error.AsCString());
#else
   return writeSocketIdToPipe(port_pipe, socket_id);
 #endif
 }
 
 void ConnectToRemote(MainLoop &mainloop,
                      GDBRemoteCommunicationServerLLGS &gdb_server,
                      bool reverse_connect, const char *const host_and_port,
                      const char *const progname, const char *const subcommand,
-                     const char *const named_pipe_path, int unnamed_pipe_fd) {
+                     const char *const named_pipe_path, int unnamed_pipe_fd,
+                     int connection_fd) {
   Status error;
 
-  if (host_and_port && host_and_port[0]) {
+  std::unique_ptr<Connection> connection_up;
+  if (connection_fd != -1) {
+    // Build the connection string.
+    char connection_url[512];
+    snprintf(connection_url, sizeof(connection_url), "fd://%d", connection_fd);
+
+    // Create the connection.
+#if !defined LLDB_DISABLE_POSIX && !defined _WIN32
+    ::fcntl(connection_fd, F_SETFD, FD_CLOEXEC);
+#endif
+    connection_up.reset(new ConnectionFileDescriptor);
+    auto connection_result = connection_up->Connect(connection_url, &error);
+    if (connection_result != eConnectionStatusSuccess) {
+      fprintf(stderr, "error: failed to connect to client at '%s' "
+                      "(connection status: %d)",
+              connection_url, static_cast<int>(connection_result));
+      exit(-1);
+    }
+    if (error.Fail()) {
+      fprintf(stderr, "error: failed to connect to client at '%s': %s",
+              connection_url, error.AsCString());
+      exit(-1);
+    }
+  } else if (host_and_port && host_and_port[0]) {
     // Parse out host and port.
     std::string final_host_and_port;
     std::string connection_host;
     std::string connection_port;
     uint32_t connection_portno = 0;
 
     // If host_and_port starts with ':', default the host to be "localhost" and
     // expect the remainder to be the port.
     if (host_and_port[0] == ':')
       final_host_and_port.append("localhost");
     final_host_and_port.append(host_and_port);
 
     const std::string::size_type colon_pos = final_host_and_port.find(':');
     if (colon_pos != std::string::npos) {
       connection_host = final_host_and_port.substr(0, colon_pos);
       connection_port = final_host_and_port.substr(colon_pos + 1);
       connection_portno = StringConvert::ToUInt32(connection_port.c_str(), 0);
     }
 
-    std::unique_ptr<Connection> connection_up;
 
     if (reverse_connect) {
       // llgs will connect to the gdb-remote client.
 
       // Ensure we have a port number for the connection.
       if (connection_portno == 0) {
         fprintf(stderr, "error: port number must be specified on when using "
                         "reverse connect");
if (reverse_connect) {
       Connection *conn = nullptr;
       error = acceptor_up->Accept(false, conn);
       if (error.Fail()) {
         printf("failed to accept new connection: %s\n", error.AsCString());
         exit(1);
       }
       connection_up.reset(conn);
     }
-    error = gdb_server.InitializeConnection(std::move(connection_up));
-    if (error.Fail()) {
-      fprintf(stderr, "Failed to initialize connection: %s\n",
-              error.AsCString());
-      exit(-1);
-    }
-    printf("Connection established.\n");
-  }
-}
+  }
 
 //----------------------------------------------------------------------
 // main
 //----------------------------------------------------------------------
 int main_gdbserver(int argc, char *argv[]) {
   Status error;
   MainLoop mainloop;
 #ifndef _WIN32
#endif
   int ch;
   std::string attach_target;
   std::string named_pipe_path;
   std::string log_file;
   StringRef
       log_channels; // e.g. "lldb process threads:gdb-remote default:linux all"
   int unnamed_pipe_fd = -1;
   bool reverse_connect = false;
+  int connection_fd = -1;
 
   // ProcessLaunchInfo launch_info;
   ProcessAttachInfo attach_info;
 
   bool show_usage = false;
   int option_error = 0;
 #if __GLIBC__
   optind = 0;
while ((ch = getopt_long_only(argc, argv, short_options.c_str(),
     case 'r':
       // Do nothing, native regs is the default these days
       break;
 
     case 'R':
       reverse_connect = true;
       break;
 
+    case 'F':
+      connection_fd = StringConvert::ToUInt32(optarg, -1);
+      break;
+
 #ifndef _WIN32
     case 'S':
       // Put llgs into a new session. Terminals group processes
       // into sessions and when a special terminal key sequences
       // (like control+c) are typed they can cause signals to go out to
       // all processes in a session. Using this --setsid (-S) option
       // will cause debugserver to run in its own sessions and be free
       // from such issues.
for (int i = 0; i < argc; i++) {
       log->Printf("argv[%i] = '%s'", i, argv[i]);
     }
   }
 
   // Skip any options we consumed with getopt_long_only.
   argc -= optind;
   argv += optind;
 
-  if (argc == 0) {
+  if (argc == 0 && connection_fd == -1) {
+    fputs("No arguments\n", stderr);
     display_usage(progname, subcommand);
     exit(255);
   }
 
   NativeProcessFactory factory;
   GDBRemoteCommunicationServerLLGS gdb_server(mainloop, factory);
 
   const char *const host_and_port = argv[0];
#endif
   else if (argc > 0)
     handle_launch(gdb_server, argc, argv);
 
   // Print version info.
   printf("%s-%s", LLGS_PROGRAM_NAME, LLGS_VERSION_STR);
 
   ConnectToRemote(mainloop, gdb_server, reverse_connect, host_and_port,
                   progname, subcommand, named_pipe_path.c_str(),
-                  unnamed_pipe_fd);
+                  unnamed_pipe_fd, connection_fd);
 
   if (!gdb_server.IsConnected()) {
     fprintf(stderr, "no connection information provided, unable to run\n");
     display_usage(progname, subcommand);
     return 1;
   }
 
   mainloop.Run();
   fprintf(stderr, "lldb-server exiting...\n");
 
   return 0;
 }