Windows Pageant: make atomic client/server decision.

In the previous state of the code, we first tested agent_exists() to
decide whether to be the long-running Pageant server or a short-lived
client; then, during the command-line parsing loop, we prompted for
passphrases to add keys presented on the command line (to ourself or
the server, respectively); *then* we set up the named pipe and
WM_COPYDATA receiver window to actually start functioning as a server,
if we decided that was our role.

A consequence is that if a user started up two Pageants each with an
encrypted key on the command line, there would be a race condition:
each one would decide that it was _going_ to be the server, then
prompt for a passphrase, and then try to set itself up as the server
once the passphrase is entered. So whichever one's passphrase prompt
was answered second would add its key to its own internal data
structures, then fail to set up the server's named pipe, terminate
with an error, and end up not having added its key to the _surviving_
server.

This change reorders the setup steps so that the command-line parsing
loop does not add the keys immediately; instead it merely caches the
key filenames provided. Then we make the decision about whether we're
the server, and set up both the named pipe and WM_COPYDATA window if
we are; and finally, we go back to our list of key filenames and
actually add them, either to ourself (if we're the server) or to some
other Pageant (if we're a client).

Moreover, the decision about whether to be the server is now wrapped
in an interprocess mutex similar to the one used in connection
sharing, which means that even if two or more Pageants are started up
as close to simultaneously as possible, they should avoid a race
condition and successfully manage to agree on exactly one of
themselves to be the server. For example, a user reported that this
could occur if you put shortcuts to multiple private key files in your
Windows Startup folder, so that they were all launched simultaneously
at startup.

One slightly odd behaviour that remains: if the server Pageant has to
prompt for private key passphrases at startup, then it won't actually
start _servicing_ requests from other Pageants until it's finished
dealing with its own prompts. As a result, if you do start up two
Pageants at once each with an encrypted key file on its command line,
the second one won't even manage to present its passphrase prompt
until the first one's prompt is dismissed, because it will block
waiting for the initial check of the key list. But it will get there
in the end, so that's only a cosmetic oddity.

It would be nice to arrange that Pageant GUI operations don't block
unrelated agent requests (e.g. by having the GUI and the agent code
run in separate threads). But that's a bigger problem, not specific to
startup time - the same thing happens if you interactively load a key
via Pageant's file dialog. And it would require a major reorganisation
to fix that fully, because currently the GUI code depends on being
able to call _internal_ Pageant query functions like
pageant_count_ssh2_keys() that don't work by constructing an agent
request at all.

windows/CMakeLists.txt

@@ -1,6 +1,7 @@
 set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR})
 
 add_sources_from_current_dir(utils
+  utils/agent_mutex_name.c
   utils/agent_named_pipe_name.c
   utils/arm_arch_queries.c
   utils/cryptoapi.c

windows/pageant.c

@@ -1375,11 +1375,18 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
 {
     MSG msg;
     const char *command = NULL;
-    bool added_keys = false;
     bool show_keylist_on_startup = false;
     int argc, i;
     char **argv, **argstart;
 
+    typedef struct CommandLineKey {
+        Filename *fn;
+        bool add_encrypted;
+    } CommandLineKey;
+
+    CommandLineKey *clkeys = NULL;
+    size_t nclkeys = 0, clkeysize = 0;
+
     dll_hijacking_protection();
 
     hinst = inst;
@@ -1431,19 +1438,10 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
     }
 
     /*
-     * Find out if Pageant is already running.
-     */
-    already_running = agent_exists();
-
-    /*
-     * Initialise the cross-platform Pageant code.
-     */
-    if (!already_running) {
-        pageant_init();
-    }
-
-    /*
-     * Process the command line and add keys as listed on it.
+     * Process the command line, handling anything that can be done
+     * immediately, but deferring adding keys until after we've
+     * started up the main agent. Details of keys to be added are
+     * stored in the 'clkeys' array.
      */
     split_into_argv(cmdline, &argc, &argv, &argstart);
     bool doing_opts = true;
@@ -1493,19 +1491,122 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
                 exit(1);
             }
         } else {
-            Filename *fn = filename_from_str(p);
-            win_add_keyfile(fn, add_keys_encrypted);
-            filename_free(fn);
-            added_keys = true;
+            sgrowarray(clkeys, clkeysize, nclkeys);
+            CommandLineKey *clkey = &clkeys[nclkeys++];
+            clkey->fn = filename_from_str(p);
+            clkey->add_encrypted = add_keys_encrypted;
         }
     }
 
     /*
-     * Forget any passphrase that we retained while going over
-     * command line keyfiles.
+     * Create and lock an interprocess mutex while we figure out
+     * whether we're going to be the Pageant server or a client. That
+     * way, two Pageant processes started up simultaneously will be
+     * able to agree on which one becomes the server without a race
+     * condition.
      */
+    HANDLE mutex;
+    {
+        char *err;
+        char *mutexname = agent_mutex_name();
+        mutex = lock_interprocess_mutex(mutexname, &err);
+        sfree(mutexname);
+        if (!mutex) {
+            MessageBox(NULL, err, "Pageant Error", MB_ICONERROR | MB_OK);
+            return 1;
+        }
+    }
+
+    /*
+     * Find out if Pageant is already running.
+     */
+    already_running = agent_exists();
+
+    /*
+     * If it isn't, we're going to be the primary Pageant that stays
+     * running, so set up all the machinery to answer requests.
+     */
+    if (!already_running) {
+        /*
+         * Initialise the cross-platform Pageant code.
+         */
+        pageant_init();
+
+        /*
+         * Set up a named-pipe listener.
+         */
+        Plug *pl_plug;
+        wpc->plc.vt = &winpgnt_vtable;
+        wpc->plc.suppress_logging = true;
+        struct pageant_listen_state *pl =
+            pageant_listener_new(&pl_plug, &wpc->plc);
+        char *pipename = agent_named_pipe_name();
+        Socket *sock = new_named_pipe_listener(pipename, pl_plug);
+        if (sk_socket_error(sock)) {
+            char *err = dupprintf("Unable to open named pipe at %s "
+                                  "for SSH agent:\n%s", pipename,
+                                  sk_socket_error(sock));
+            MessageBox(NULL, err, "Pageant Error", MB_ICONERROR | MB_OK);
+            return 1;
+        }
+        pageant_listener_got_socket(pl, sock);
+        sfree(pipename);
+
+        /*
+         * Set up the window class for the hidden window that receives
+         * the WM_COPYDATA message used by the old-style Pageant IPC
+         * system.
+         */
+        if (!prev) {
+            WNDCLASS wndclass;
+
+            memset(&wndclass, 0, sizeof(wndclass));
+            wndclass.lpfnWndProc = wm_copydata_WndProc;
+            wndclass.hInstance = inst;
+            wndclass.lpszClassName = IPCCLASSNAME;
+
+            RegisterClass(&wndclass);
+        }
+
+        /*
+         * And launch the subthread which will open that hidden window and
+         * handle WM_COPYDATA messages on it.
+         */
+        wmcpc.vt = &wmcpc_vtable;
+        wmcpc.suppress_logging = true;
+        pageant_register_client(&wmcpc);
+        DWORD wm_copydata_threadid;
+        wmct.ev_msg_ready = CreateEvent(NULL, false, false, NULL);
+        wmct.ev_reply_ready = CreateEvent(NULL, false, false, NULL);
+        HANDLE hThread = CreateThread(NULL, 0, wm_copydata_threadfunc,
+                                      &inst, 0, &wm_copydata_threadid);
+        if (hThread)
+            CloseHandle(hThread); /* we don't need the thread handle */
+        add_handle_wait(wmct.ev_msg_ready, wm_copydata_got_msg, NULL);
+    }
+
+    /*
+     * Now we're either a fully set up Pageant server, or we know one
+     * is running somewhere else. Either way, now it's safe to unlock
+     * the mutex.
+     */
+    unlock_interprocess_mutex(mutex);
+
+    /*
+     * Add any keys provided on the command line.
+     */
+    for (size_t i = 0; i < nclkeys; i++) {
+        CommandLineKey *clkey = &clkeys[i];
+        win_add_keyfile(clkey->fn, clkey->add_encrypted);
+        filename_free(clkey->fn);
+    }
+    sfree(clkeys);
+    /* And forget any passphrases we stashed during that loop. */
     pageant_forget_passphrases();
 
+    /*
+     * Now our keys are present, spawn a command, if we were asked to.
+     */
     if (command) {
         char *args;
         if (command[0] == '"')
@@ -1525,7 +1626,7 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
      * keys), complain.
      */
     if (already_running) {
-        if (!command && !added_keys) {
+        if (!command && !nclkeys) {
             MessageBox(NULL, "Pageant is already running", "Pageant Error",
                        MB_ICONERROR | MB_OK);
         }
@@ -1533,32 +1634,8 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
     }
 
     /*
-     * Set up a named-pipe listener.
-     */
-    {
-        Plug *pl_plug;
-        wpc->plc.vt = &winpgnt_vtable;
-        wpc->plc.suppress_logging = true;
-        struct pageant_listen_state *pl =
-            pageant_listener_new(&pl_plug, &wpc->plc);
-        char *pipename = agent_named_pipe_name();
-        Socket *sock = new_named_pipe_listener(pipename, pl_plug);
-        if (sk_socket_error(sock)) {
-            char *err = dupprintf("Unable to open named pipe at %s "
-                                  "for SSH agent:\n%s", pipename,
-                                  sk_socket_error(sock));
-            MessageBox(NULL, err, "Pageant Error", MB_ICONERROR | MB_OK);
-            return 1;
-        }
-        pageant_listener_got_socket(pl, sock);
-        sfree(pipename);
-    }
-
-    /*
-     * Set up window classes for two hidden windows: one that receives
-     * all the messages to do with our presence in the system tray,
-     * and one that receives the WM_COPYDATA message used by the
-     * old-style Pageant IPC system.
+     * Set up the window class for the hidden window that receives
+     * all the messages to do with our presence in the system tray.
      */
 
     if (!prev) {
@@ -1571,13 +1648,6 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
         wndclass.lpszClassName = TRAYCLASSNAME;
 
         RegisterClass(&wndclass);
-
-        memset(&wndclass, 0, sizeof(wndclass));
-        wndclass.lpfnWndProc = wm_copydata_WndProc;
-        wndclass.hInstance = inst;
-        wndclass.lpszClassName = IPCCLASSNAME;
-
-        RegisterClass(&wndclass);
     }
 
     keylist = NULL;
@@ -1623,18 +1693,7 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
 
     ShowWindow(traywindow, SW_HIDE);
 
-    wmcpc.vt = &wmcpc_vtable;
-    wmcpc.suppress_logging = true;
-    pageant_register_client(&wmcpc);
-    DWORD wm_copydata_threadid;
-    wmct.ev_msg_ready = CreateEvent(NULL, false, false, NULL);
-    wmct.ev_reply_ready = CreateEvent(NULL, false, false, NULL);
-    HANDLE hThread = CreateThread(NULL, 0, wm_copydata_threadfunc,
-                                  &inst, 0, &wm_copydata_threadid);
-    if (hThread)
-        CloseHandle(hThread);          /* we don't need the thread handle */
-    add_handle_wait(wmct.ev_msg_ready, wm_copydata_got_msg, NULL);
-
+    /* Open the visible key list window, if we've been asked to. */
     if (show_keylist_on_startup)
         create_keylist_window();
 

windows/platform.h

@@ -655,8 +655,9 @@ void handle_wait_activate(HandleWaitList *hwl, int index);
 void handle_wait_list_free(HandleWaitList *hwl);
 
 /*
- * Exports from winpgntc.c.
+ * Pageant-related pathnames.
  */
+char *agent_mutex_name(void);
 char *agent_named_pipe_name(void);
 
 /*

windows/utils/agent_mutex_name.c

@@ -0,0 +1,14 @@
+/*
+ * Return the full pathname of the global mutex that Pageant uses at
+ * startup to atomically decide whether to be a server or a client.
+ */
+
+#include "putty.h"
+
+char *agent_mutex_name(void)
+{
+    char *username = get_username();
+    char *mutexname = dupprintf("Local\\pageant-mutex.%s", username);
+    sfree(username);
+    return mutexname;
+}