[PATCH 2/2] Fix race condition when using ControlMaster=auto with simultaneous connections

[Jens Rosenboom]

Fix race condition that occurs when multiple new connections are made 
simultaneously and with ControlMaster=auto.
(See https://bugzilla.mindrot.org/show_bug.cgi?id=3971 .)

Tested-by: Baptiste Jonglez <git at bitsofnetworks.org>
Tested-by: Oliver Freyermuth <o.freyermuth at googlemail.com>
Signed-off-by: Jens Rosenboom <jens.rosenboom at web.de>
---
  clientloop.h         |  1 +
  mux.c                | 68 +++++++++++++++++++++-----------------------
  regress/multiplex.sh | 19 +++++++++++++
  ssh.c                |  4 ++-
  4 files changed, 56 insertions(+), 36 deletions(-)

diff --git a/clientloop.h b/clientloop.h
index ed3c54fa7..e31b4e1d9 100644
--- a/clientloop.h
+++ b/clientloop.h
@@ -78,6 +78,7 @@ void client_expect_confirm(struct ssh *, int, const 
char *,
  #define SSHMUX_COMMAND_CONNINFO        9    /* Show connection 
information */
  #define SSHMUX_COMMAND_CHANINFO        10    /* Show channels 
information */

+int    muxserver(const char *);
  void    muxserver_listen(struct ssh *);
  int    muxclient(const char *);
  void    mux_exit_message(struct ssh *, Channel *, int);
diff --git a/mux.c b/mux.c
index 0cd169732..03766d623 100644
--- a/mux.c
+++ b/mux.c
@@ -1315,21 +1315,17 @@ mux_tty_alloc_failed(struct ssh *ssh, Channel *c)
      sshbuf_free(m);
  }

-/* Prepare a mux master to listen on a Unix domain socket. */
-void
-muxserver_listen(struct ssh *ssh)
+/* Create a listening Unix domain socket for mux master or find out 
that one exists already. */
+int
+muxserver(const char *path)
  {
      mode_t old_umask;
-    char *orig_control_path = options.control_path;
+    char *tmp_path;
      char rbuf[16+1];
      u_int i, r;
      int oerrno;

-    if (options.control_path == NULL ||
-        options.control_master == SSHCTL_MASTER_NO)
-        return;
-
-    debug("setting up multiplex master socket");
+    debug("trying to set-up a multiplex master socket");

      /*
       * Use a temporary path before listen so we can pseudo-atomically
@@ -1344,51 +1340,53 @@ muxserver_listen(struct ssh *ssh)
              '0' + r - 26 - 26;
      }
      rbuf[sizeof(rbuf) - 1] = '\0';
-    options.control_path = NULL;
-    xasprintf(&options.control_path, "%s.%s", orig_control_path, rbuf);
-    debug3_f("temporary control path %s", options.control_path);
+    xasprintf(&tmp_path, "%s.%s", path, rbuf);
+    debug3_f("temporary control path %s", tmp_path);

      old_umask = umask(0177);
-    muxserver_sock = unix_listener(options.control_path, 64, 0);
+    muxserver_sock = unix_listener(tmp_path, 64, 0);
      oerrno = errno;
      umask(old_umask);
      if (muxserver_sock < 0) {
          if (oerrno == EINVAL || oerrno == EADDRINUSE) {
-            error("ControlSocket %s already exists, "
-                "disabling multiplexing", options.control_path);
- disable_mux_master:
-            if (muxserver_sock != -1) {
-                close(muxserver_sock);
-                muxserver_sock = -1;
-            }
-            free(orig_control_path);
-            free(options.control_path);
-            options.control_path = NULL;
-            options.control_master = SSHCTL_MASTER_NO;
-            return;
+            error("Temporary ControlSocket %s already exists", tmp_path);
+            free(tmp_path);
+            return -1;
          } else {
              /* unix_listener() logs the error */
              cleanup_exit(255);
          }
      }
+    set_nonblock(muxserver_sock);

      /* Now atomically "move" the mux socket into position */
-    if (link(options.control_path, orig_control_path) != 0) {
+    if (link(tmp_path, path) != 0) {
          if (errno != EEXIST) {
              fatal_f("link mux listener %s => %s: %s",
-                options.control_path, orig_control_path,
+                tmp_path, path,
                  strerror(errno));
          }
-        error("ControlSocket %s already exists, disabling multiplexing",
-            orig_control_path);
-        unlink(options.control_path);
-        goto disable_mux_master;
+        debug("ControlSocket %s already exists", path);
+        close(muxserver_sock);
+        muxserver_sock = -1;
      }
-    unlink(options.control_path);
-    free(options.control_path);
-    options.control_path = orig_control_path;
+    unlink(tmp_path);
+    free(tmp_path);
+    return muxserver_sock;
+}

-    set_nonblock(muxserver_sock);
+/* Prepare a mux master to listen on the previously created Unix domain 
socket. */
+void
+muxserver_listen(struct ssh *ssh)
+{
+    if (options.control_path == NULL ||
+        options.control_master == SSHCTL_MASTER_NO)
+        return;
+
+    debug("setting up multiplex master socket");
+    if (muxserver_sock < 0) {
+        muxserver_sock = muxserver(options.control_path);
+    }

      mux_listener_channel = channel_new(ssh, "mux listener",
          SSH_CHANNEL_MUX_LISTENER, muxserver_sock, muxserver_sock, -1,
diff --git a/regress/multiplex.sh b/regress/multiplex.sh
index 239346863..efabffb4e 100644
--- a/regress/multiplex.sh
+++ b/regress/multiplex.sh
@@ -48,6 +48,24 @@ if [ $? -ne 0 ]; then
      fail "environment not found"
  fi

+start_auto_mux_master()
+{
+    trace "start master (with ControlMaster=auto), fork to background"
+    ${SSH} -Nn2 -o ControlMaster=auto -S$CTL -F $OBJ/ssh_config 
-oSendEnv="_XXX_TEST" somehost \
+        -E $TEST_REGRESS_LOGFILE 2>&1 &
+    # NB. $SSH_PID will be killed by test-exec.sh:cleanup on fatal errors.
+    SSH_PID=$!
+    sleep 2
+    wait_for_mux_master_ready
+}
+
+verbose "test $tid: stale control socket"
+trace "correctly handle stale control socket"
+kill -9 ${SSH_PID} 2>/dev/null
+wait ${SSH_PID}
+test -e "$CTL" || fail "control socket did not remain (after killing 
ssh command)"
+start_auto_mux_master
+
  verbose "test $tid: envpass"
  trace "env passing over multiplexed connection"
  ${SSH} -F $OBJ/ssh_config -oSetEnv="_XXX_TEST=foo" -S$CTL otherhost sh 
<< 'EOF'
@@ -195,6 +213,7 @@ ${SSH} -F $OBJ/ssh_config -S $CTL -Oexit otherhost 
 >>$TEST_REGRESS_LOGFILE 2>&1
  # Wait for master to exit
  wait $SSH_PID
  kill -0 $SSH_PID >/dev/null 2>&1 && fail "exit command failed"
+test ! -e "$CTL" || fail "control socket still exists after exit command"

  # Enable compression and alternative kex for next conninfo test.
  if $SSH -Q compression | grep zlib at openssh.com >/dev/null; then
diff --git a/ssh.c b/ssh.c
index e9f99c433..bd380b8ad 100644
--- a/ssh.c
+++ b/ssh.c
@@ -1634,7 +1634,9 @@ main(int ac, char **av)
          fatal("No ControlPath specified for \"-O\" command");
      if (options.control_path != NULL) {
          int sock;
-        if ((sock = muxclient(options.control_path)) >= 0) {
+        if (muxclient_command == 0 && (sock = 
muxserver(options.control_path)) >= 0) {
+            debug("We will be multiplex master, not client.");
+        } else if ((sock = muxclient(options.control_path)) >= 0) {
              if (ssh_packet_set_connection(ssh, sock, sock) == NULL)
                  fatal("ssh_packet_set_connection failed");
              ssh_packet_set_mux(ssh);
-- 
2.55.0