Windows Pageant and PuTTYgen: spiff up option parsing.

These two tools had ad-hoc command loops with similar options, and I
want to extend both (in particular, in a way that introduces options
with arguments). So I've started by throwing together some common code
to do all the tedious bits like finding option arguments wherever they
might be, throwing errors, handling "--" and so on.

Should be no functional change to the existing command-line syntax,
except that now all long options are recognised in both "-foo" and
"--foo" form.

windows/CMakeLists.txt

@@ -4,6 +4,7 @@ add_sources_from_current_dir(utils
   utils/agent_mutex_name.c
   utils/agent_named_pipe_name.c
   utils/arm_arch_queries.c
+  utils/aux_match_opt.c
   utils/cryptoapi.c
   utils/defaults.c
   utils/dll_hijacking_protection.c

windows/pageant.c

@@ -1371,12 +1371,24 @@ static struct winpgnt_client wpc[1];
 
 HINSTANCE hinst;
 
+static NORETURN void opt_error(const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    char *msg = dupvprintf(fmt, ap);
+    va_end(ap);
+
+    MessageBox(NULL, msg, "Pageant command line error", MB_ICONERROR | MB_OK);
+
+    exit(1);
+}
+
 int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
 {
     MSG msg;
     const char *command = NULL;
     bool show_keylist_on_startup = false;
-    int argc, i;
+    int argc;
     char **argv, **argstart;
 
     typedef struct CommandLineKey {
@@ -1444,57 +1456,49 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
      * stored in the 'clkeys' array.
      */
     split_into_argv(cmdline, &argc, &argv, &argstart);
-    bool doing_opts = true;
     bool add_keys_encrypted = false;
-    for (i = 0; i < argc; i++) {
-        char *p = argv[i];
-        if (*p == '-' && doing_opts) {
-            if (!strcmp(p, "-pgpfp")) {
-                pgp_fingerprints_msgbox(NULL);
-                return 1;
-            } else if (!strcmp(p, "-restrict-acl") ||
-                       !strcmp(p, "-restrict_acl") ||
-                       !strcmp(p, "-restrictacl")) {
-                restrict_process_acl();
-            } else if (!strcmp(p, "-restrict-putty-acl") ||
-                       !strcmp(p, "-restrict_putty_acl")) {
-                restrict_putty_acl = true;
-            } else if (!strcmp(p, "--no-decrypt") ||
-                       !strcmp(p, "-no-decrypt") ||
-                       !strcmp(p, "--no_decrypt") ||
-                       !strcmp(p, "-no_decrypt") ||
-                       !strcmp(p, "--nodecrypt") ||
-                       !strcmp(p, "-nodecrypt") ||
-                       !strcmp(p, "--encrypted") ||
-                       !strcmp(p, "-encrypted")) {
-                add_keys_encrypted = true;
-            } else if (!strcmp(p, "-keylist") || !strcmp(p, "--keylist")) {
-                show_keylist_on_startup = true;
-            } else if (!strcmp(p, "-c")) {
-                /*
-                 * If we see `-c', then the rest of the
-                 * command line should be treated as a
-                 * command to be spawned.
-                 */
-                if (i < argc-1)
-                    command = argstart[i+1];
-                else
-                    command = "";
-                break;
-            } else if (!strcmp(p, "--")) {
-                doing_opts = false;
-            } else {
-                char *msg = dupprintf("unrecognised command-line option\n"
-                                      "'%s'", p);
-                MessageBox(NULL, msg, "Pageant command-line syntax error",
-                           MB_ICONERROR | MB_OK);
-                exit(1);
-            }
-        } else {
+    AuxMatchOpt amo = aux_match_opt_init(argc, argv, 0, opt_error);
+    while (!aux_match_done(&amo)) {
+        char *val;
+        #define match_opt(...) aux_match_opt( \
+            &amo, NULL, __VA_ARGS__, (const char *)NULL)
+        #define match_optval(...) aux_match_opt( \
+            &amo, &val, __VA_ARGS__, (const char *)NULL)
+
+        if (aux_match_arg(&amo, &val)) {
+            /*
+             * Non-option arguments are expected to be key files, and
+             * added to clkeys.
+             */
             sgrowarray(clkeys, clkeysize, nclkeys);
             CommandLineKey *clkey = &clkeys[nclkeys++];
-            clkey->fn = filename_from_str(p);
+            clkey->fn = filename_from_str(val);
             clkey->add_encrypted = add_keys_encrypted;
+        } else if (match_opt("-pgpfp")) {
+            pgp_fingerprints_msgbox(NULL);
+            return 1;
+        } else if (match_opt("-restrict-acl", "-restrict_acl",
+                             "-restrictacl")) {
+            restrict_process_acl();
+        } else if (match_opt("-restrict-putty-acl", "-restrict_putty_acl")) {
+            restrict_putty_acl = true;
+        } else if (match_opt("-no-decrypt", "-no_decrypt",
+                             "-nodecrypt", "-encrypted")) {
+            add_keys_encrypted = true;
+        } else if (match_opt("-keylist")) {
+            show_keylist_on_startup = true;
+        } else if (match_opt("-c")) {
+            /*
+             * If we see `-c', then the rest of the command line
+             * should be treated as a command to be spawned.
+             */
+            if (amo.index < amo.argc-1)
+                command = argstart[amo.index + 1];
+            else
+                command = "";
+            break;
+        } else {
+            opt_error("unrecognised option '%s'\n", amo.argv[amo.index]);
         }
     }
 

windows/platform.h

@@ -744,4 +744,17 @@ SeatPromptResult make_spr_sw_abort_winerror(const char *prefix, DWORD error);
 HANDLE lock_interprocess_mutex(const char *mutexname, char **error);
 void unlock_interprocess_mutex(HANDLE mutex);
 
+typedef void (*aux_opt_error_fn_t)(const char *, ...);
+typedef struct AuxMatchOpt {
+    int index, argc;
+    char **argv;
+    bool doing_opts;
+    aux_opt_error_fn_t error;
+} AuxMatchOpt;
+AuxMatchOpt aux_match_opt_init(int argc, char **argv, int start_index,
+                               aux_opt_error_fn_t opt_error);
+bool aux_match_arg(AuxMatchOpt *amo, char **val);
+bool aux_match_opt(AuxMatchOpt *amo, char **val, const char *optname, ...);
+bool aux_match_done(AuxMatchOpt *amo);
+
 #endif /* PUTTY_WINDOWS_PLATFORM_H */

windows/puttygen.c

@@ -1996,9 +1996,21 @@ void cleanup_exit(int code)
 
 HINSTANCE hinst;
 
+static NORETURN void opt_error(const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    char *msg = dupvprintf(fmt, ap);
+    va_end(ap);
+
+    MessageBox(NULL, msg, "PuTTYgen command line error", MB_ICONERROR | MB_OK);
+
+    exit(1);
+}
+
 int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
 {
-    int argc, i;
+    int argc;
     char **argv;
     int ret;
 
@@ -2014,21 +2026,34 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
 
     split_into_argv(cmdline, &argc, &argv, NULL);
 
-    for (i = 0; i < argc; i++) {
-        if (!strcmp(argv[i], "-pgpfp")) {
+    int argbits = -1;
+    AuxMatchOpt amo = aux_match_opt_init(argc, argv, 0, opt_error);
+    while (!aux_match_done(&amo)) {
+        char *val;
+        #define match_opt(...) aux_match_opt( \
+            &amo, NULL, __VA_ARGS__, (const char *)NULL)
+        #define match_optval(...) aux_match_opt( \
+            &amo, &val, __VA_ARGS__, (const char *)NULL)
+
+        if (aux_match_arg(&amo, &val)) {
+            if (!cmdline_keyfile) {
+                /*
+                 * Assume the first argument to be a private key file, and
+                 * attempt to load it.
+                 */
+                cmdline_keyfile = val;
+                continue;
+            } else {
+                opt_error("unexpected extra argument '%s'\n", val);
+            }
+        } else if (match_opt("-pgpfp")) {
             pgp_fingerprints_msgbox(NULL);
             return 1;
-        } else if (!strcmp(argv[i], "-restrict-acl") ||
-                   !strcmp(argv[i], "-restrict_acl") ||
-                   !strcmp(argv[i], "-restrictacl")) {
+        } else if (match_opt("-restrict-acl", "-restrict_acl",
+                             "-restrictacl")) {
             restrict_process_acl();
         } else {
-            /*
-             * Assume the first argument to be a private key file, and
-             * attempt to load it.
-             */
-            cmdline_keyfile = argv[i];
-            break;
+            opt_error("unrecognised option '%s'\n", amo.argv[amo.index]);
         }
     }
 

windows/utils/aux_match_opt.c

@@ -0,0 +1,117 @@
+/*
+ * Helper function for matching command-line options in the Windows
+ * auxiliary tools (PuTTYgen and Pageant).
+ *
+ * Supports basically the usual kinds of option, including GNUish
+ * --foo long options and simple -f short options. But historically
+ * those tools have also supported long options with a single dash, so
+ * we don't go full GNU and report those as syntax errors.
+ */
+
+#include "putty.h"
+
+/*
+ * Call this to initialise the state structure.
+ */
+AuxMatchOpt aux_match_opt_init(int argc, char **argv, int start_index,
+                               aux_opt_error_fn_t opt_error)
+{
+    AuxMatchOpt amo[1];
+
+    amo->argc = argc;
+    amo->argv = argv;
+    amo->index = start_index;
+    amo->doing_opts = true;
+    amo->error = opt_error;
+
+    return amo[0];
+}
+
+/*
+ * Call this with a NULL-terminated list of synonymous option names.
+ * Point 'val' at a char * to receive the option argument, if one is
+ * expected. Set 'val' to NULL if no argument is expected.
+ */
+bool aux_match_opt(AuxMatchOpt *amo, char **val, const char *optname, ...)
+{
+    assert(amo->index < amo->argc);
+
+    /* Find the end of the option name */
+    char *opt = amo->argv[amo->index];
+    ptrlen argopt;
+    argopt.ptr = opt;
+    argopt.len = strcspn(opt, "=");
+
+    /* Normalise GNU-style --foo long options to the -foo that this
+     * tool has historically used */
+    ptrlen argopt2 = make_ptrlen(NULL, 0);
+    if (ptrlen_startswith(argopt, PTRLEN_LITERAL("--"), NULL))
+        ptrlen_startswith(argopt, PTRLEN_LITERAL("-"), &argopt2);
+
+    /* See if this option matches any of our synonyms */
+    va_list ap;
+    va_start(ap, optname);
+    bool matched = false;
+    while (optname) {
+        if (ptrlen_eq_string(argopt, optname)) {
+            matched = true;
+            break;
+        }
+        if (argopt2.ptr && strlen(optname) > 2 &&
+            ptrlen_eq_string(argopt2, optname)) {
+            matched = true;
+            break;
+        }
+        optname = va_arg(ap, const char *);
+    }
+    va_end(ap);
+    if (!matched)
+        return false;
+
+    /* Check for a value */
+    if (opt[argopt.len]) {
+        if (!val)
+            amo->error("option '%s' does not expect a value", opt);
+        *val = opt + argopt.len + 1;
+        amo->index++;
+    } else if (!val) {
+        amo->index++;
+    } else {
+        if (amo->index + 1 >= amo->argc)
+            amo->error("option '%s' expects a value", opt);
+        *val = amo->argv[amo->index + 1];
+        amo->index += 2;
+    }
+
+    return true;
+}
+
+/*
+ * Call this to return a non-option argument in *val.
+ */
+bool aux_match_arg(AuxMatchOpt *amo, char **val)
+{
+    assert(amo->index < amo->argc);
+    char *opt = amo->argv[amo->index];
+
+    if (!amo->doing_opts || opt[0] != '-' || !strcmp(opt, "-")) {
+        *val = opt;
+        amo->index++;
+        return true;
+    }
+
+    return false;
+}
+
+/*
+ * And call this to test whether we're done. Also handles '--'.
+ */
+bool aux_match_done(AuxMatchOpt *amo)
+{
+    if (amo->index < amo->argc && !strcmp(amo->argv[amo->index], "--")) {
+        amo->doing_opts = false;
+        amo->index++;
+    }
+
+    return amo->index >= amo->argc;
+}