Merge branch 'tor-github/pr/980'

Signed-off-by: David Goulet <dgoulet@torproject.org>

changes/ticket29984

@@ -0,0 +1,5 @@
+  o Minor bugfixes (controller protocol):
+    - Teach the controller parser to correctly distinguish an object
+      preceded by an argument list from one without. Previously, it
+      couldn't distinguish an argument list from the first line of a
+      multiline object. Fixes bug 29984; bugfix on 0.2.3.8-alpha.

changes/ticket30091

@@ -0,0 +1,4 @@
+  o Major features (controller protocol):
+    - Controller commands are now parsed using a generalized parsing
+      subsystem. Previously, each controller command was responsible for
+      parsing its own input.  Closes ticket 30091.

scripts/maint/practracker/exceptions.txt

@@ -54,9 +54,9 @@ problem function-size /src/app/main/main.c:sandbox_init_filter() 291
 problem function-size /src/app/main/main.c:run_tor_main_loop() 105
 problem function-size /src/app/main/ntmain.c:nt_service_install() 125
 problem include-count /src/app/main/shutdown.c 52
-problem file-size /src/core/mainloop/connection.c 5558
+problem file-size /src/core/mainloop/connection.c 5559
 problem include-count /src/core/mainloop/connection.c 61
-problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 184
+problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 185
 problem function-size /src/core/mainloop/connection.c:connection_listener_new() 328
 problem function-size /src/core/mainloop/connection.c:connection_handle_listener_read() 161
 problem function-size /src/core/mainloop/connection.c:connection_connect_sockaddr() 103
@@ -152,7 +152,7 @@ problem function-size /src/feature/control/control_cmd.c:handle_control_add_onio
 problem function-size /src/feature/control/control_cmd.c:add_onion_helper_keyarg() 125
 problem function-size /src/feature/control/control_cmd.c:handle_control_command() 104
 problem function-size /src/feature/control/control_events.c:control_event_stream_status() 119
-problem include-count /src/feature/control/control_getinfo.c 52
+problem include-count /src/feature/control/control_getinfo.c 53
 problem function-size /src/feature/control/control_getinfo.c:getinfo_helper_misc() 109
 problem function-size /src/feature/control/control_getinfo.c:getinfo_helper_dir() 304
 problem function-size /src/feature/control/control_getinfo.c:getinfo_helper_events() 236

src/core/include.am

@@ -298,6 +298,7 @@ noinst_HEADERS +=					\
 	src/feature/control/control.h			\
 	src/feature/control/control_auth.h		\
 	src/feature/control/control_cmd.h		\
+	src/feature/control/control_cmd_args_st.h	\
 	src/feature/control/control_connection_st.h	\
 	src/feature/control/control_events.h	        \
 	src/feature/control/control_fmt.h		\

src/core/mainloop/connection.c

@@ -697,6 +697,7 @@ connection_free_minimal(connection_t *conn)
     control_connection_t *control_conn = TO_CONTROL_CONN(conn);
     tor_free(control_conn->safecookie_client_hash);
     tor_free(control_conn->incoming_cmd);
+    tor_free(control_conn->current_cmd);
     if (control_conn->ephemeral_onion_services) {
       SMARTLIST_FOREACH(control_conn->ephemeral_onion_services, char *, cp, {
         memwipe(cp, 0, strlen(cp));

src/feature/control/control.c

@@ -33,6 +33,7 @@
  **/
 
 #define CONTROL_MODULE_PRIVATE
+#define CONTROL_PRIVATE
 
 #include "core/or/or.h"
 #include "app/config/config.h"
@@ -274,6 +275,44 @@ peek_connection_has_http_command(connection_t *conn)
   return peek_buf_has_http_command(conn->inbuf);
 }
 
+/**
+ * Helper: take a nul-terminated command of given length, and find where the
+ * command starts and the arguments begin.  Separate them, allocate a new
+ * string in <b>current_cmd_out</b> for the command, and return a pointer
+ * to the arguments.
+ **/
+STATIC char *
+control_split_incoming_command(char *incoming_cmd,
+                               size_t *data_len,
+                               char **current_cmd_out)
+{
+  const bool is_multiline = *data_len && incoming_cmd[0] == '+';
+  size_t cmd_len = 0;
+  while (cmd_len < *data_len
+         && !TOR_ISSPACE(incoming_cmd[cmd_len]))
+    ++cmd_len;
+
+  *current_cmd_out = tor_memdup_nulterm(incoming_cmd, cmd_len);
+  char *args = incoming_cmd+cmd_len;
+  tor_assert(*data_len>=cmd_len);
+  *data_len -= cmd_len;
+  if (is_multiline) {
+    // Only match horizontal space: any line after the first is data,
+    // not arguments.
+    while ((*args == '\t' || *args == ' ') && *data_len) {
+      ++args;
+      --*data_len;
+    }
+  } else {
+    while (TOR_ISSPACE(*args) && *data_len) {
+      ++args;
+      --*data_len;
+    }
+  }
+
+  return args;
+}
+
 static const char CONTROLPORT_IS_NOT_AN_HTTP_PROXY_MSG[] =
   "HTTP/1.0 501 Tor ControlPort is not an HTTP proxy"
   "\r\nContent-Type: text/html; charset=iso-8859-1\r\n\r\n"
@@ -308,7 +347,6 @@ connection_control_process_inbuf(control_connection_t *conn)
 {
   size_t data_len;
   uint32_t cmd_data_len;
-  int cmd_len;
   char *args;
 
   tor_assert(conn);
@@ -400,22 +438,15 @@ connection_control_process_inbuf(control_connection_t *conn)
     /* Otherwise, read another line. */
   }
   data_len = conn->incoming_cmd_cur_len;
+
   /* Okay, we now have a command sitting on conn->incoming_cmd. See if we
    * recognize it.
    */
-  cmd_len = 0;
+  tor_free(conn->current_cmd);
-  while ((size_t)cmd_len < data_len
+  args = control_split_incoming_command(conn->incoming_cmd, &data_len,
-         && !TOR_ISSPACE(conn->incoming_cmd[cmd_len]))
+                                        &conn->current_cmd);
-    ++cmd_len;
+  if (BUG(!conn->current_cmd))
-
+    return -1;
-  conn->incoming_cmd[cmd_len]='\0';
-  args = conn->incoming_cmd+cmd_len+1;
-  tor_assert(data_len>(size_t)cmd_len);
-  data_len -= (cmd_len+1); /* skip the command and NUL we added after it */
-  while (TOR_ISSPACE(*args)) {
-    ++args;
-    --data_len;
-  }
 
   /* If the connection is already closing, ignore further commands */
   if (TO_CONN(conn)->marked_for_close) {
@@ -423,14 +454,14 @@ connection_control_process_inbuf(control_connection_t *conn)
   }
 
   /* Otherwise, Quit is always valid. */
-  if (!strcasecmp(conn->incoming_cmd, "QUIT")) {
+  if (!strcasecmp(conn->current_cmd, "QUIT")) {
     connection_write_str_to_buf("250 closing connection\r\n", conn);
     connection_mark_and_flush(TO_CONN(conn));
     return 0;
   }
 
   if (conn->base_.state == CONTROL_CONN_STATE_NEEDAUTH &&
-      !is_valid_initial_command(conn, conn->incoming_cmd)) {
+      !is_valid_initial_command(conn, conn->current_cmd)) {
     connection_write_str_to_buf("514 Authentication required.\r\n", conn);
     connection_mark_for_close(TO_CONN(conn));
     return 0;

src/feature/control/control.h

@@ -60,4 +60,10 @@ int get_cached_network_liveness(void);
 void set_cached_network_liveness(int liveness);
 #endif /* defined(CONTROL_MODULE_PRIVATE) */
 
+#ifdef CONTROL_PRIVATE
+STATIC char *control_split_incoming_command(char *incoming_cmd,
+                                            size_t *data_len,
+                                            char **current_cmd_out);
+#endif
+
 #endif /* !defined(TOR_CONTROL_H) */

src/feature/control/control_auth.c

@@ -11,12 +11,16 @@
 #include "app/config/config.h"
 #include "core/mainloop/connection.h"
 #include "feature/control/control.h"
+#include "feature/control/control_cmd.h"
 #include "feature/control/control_auth.h"
+#include "feature/control/control_cmd_args_st.h"
 #include "feature/control/control_connection_st.h"
 #include "feature/control/control_fmt.h"
 #include "lib/crypt_ops/crypto_rand.h"
 #include "lib/crypt_ops/crypto_util.h"
 #include "lib/encoding/confline.h"
+#include "lib/encoding/kvline.h"
+#include "lib/encoding/qstring.h"
 
 #include "lib/crypt_ops/crypto_s2k.h"
 
@@ -116,12 +120,19 @@ decode_hashed_passwords(config_line_t *passwords)
   return NULL;
 }
 
+const control_cmd_syntax_t authchallenge_syntax = {
+   .min_args = 1,
+   .max_args = 1,
+   .accept_keywords=true,
+   .kvline_flags=KV_OMIT_KEYS|KV_QUOTED_QSTRING,
+   .store_raw_body=true
+};
+
 /** Called when we get an AUTHCHALLENGE command. */
 int
-handle_control_authchallenge(control_connection_t *conn, uint32_t len,
+handle_control_authchallenge(control_connection_t *conn,
-                             const char *body)
+                             const control_cmd_args_t *args)
 {
-  const char *cp = body;
   char *client_nonce;
   size_t client_nonce_len;
   char server_hash[DIGEST256_LEN];
@@ -129,63 +140,50 @@ handle_control_authchallenge(control_connection_t *conn, uint32_t len,
   char server_nonce[SAFECOOKIE_SERVER_NONCE_LEN];
   char server_nonce_encoded[(2*SAFECOOKIE_SERVER_NONCE_LEN) + 1];
 
-  cp += strspn(cp, " \t\n\r");
+  if (strcasecmp(smartlist_get(args->args, 0), "SAFECOOKIE")) {
-  if (!strcasecmpstart(cp, "SAFECOOKIE")) {
-    cp += strlen("SAFECOOKIE");
-  } else {
     connection_write_str_to_buf("513 AUTHCHALLENGE only supports SAFECOOKIE "
                                 "authentication\r\n", conn);
-    connection_mark_for_close(TO_CONN(conn));
+    goto fail;
-    return -1;
   }
-
   if (!authentication_cookie_is_set) {
     connection_write_str_to_buf("515 Cookie authentication is disabled\r\n",
                                 conn);
-    connection_mark_for_close(TO_CONN(conn));
+    goto fail;
-    return -1;
+  }
+  if (args->kwargs == NULL || args->kwargs->next != NULL) {
+    /*    connection_write_str_to_buf("512 AUTHCHALLENGE requires exactly "
+                                "2 arguments.\r\n", conn);
+    */
+    connection_printf_to_buf(conn,
+                             "512 AUTHCHALLENGE dislikes argument list %s\r\n",
+                             escaped(args->raw_body));
+    goto fail;
+  }
+  if (strcmp(args->kwargs->key, "")) {
+    connection_write_str_to_buf("512 AUTHCHALLENGE does not accept keyword "
+                                "arguments.\r\n", conn);
+    goto fail;
   }
 
-  cp += strspn(cp, " \t\n\r");
+  bool contains_quote = strchr(args->raw_body, '\"');
-  if (*cp == '"') {
+  if (contains_quote) {
-    const char *newcp =
+    /* The nonce was quoted */
-      decode_escaped_string(cp, len - (cp - body),
+    client_nonce = tor_strdup(args->kwargs->value);
-                            &client_nonce, &client_nonce_len);
+    client_nonce_len = strlen(client_nonce);
-    if (newcp == NULL) {
-      connection_write_str_to_buf("513 Invalid quoted client nonce\r\n",
-                                  conn);
-      connection_mark_for_close(TO_CONN(conn));
-      return -1;
-    }
-    cp = newcp;
   } else {
-    size_t client_nonce_encoded_len = strspn(cp, "0123456789ABCDEFabcdef");
+    /* The nonce was should be in hex. */
-
+    const char *hex_nonce = args->kwargs->value;
-    client_nonce_len = client_nonce_encoded_len / 2;
+    client_nonce_len = strlen(hex_nonce) / 2;
-    client_nonce = tor_malloc_zero(client_nonce_len);
+    client_nonce = tor_malloc(client_nonce_len);
-
+    if (base16_decode(client_nonce, client_nonce_len, hex_nonce,
-    if (base16_decode(client_nonce, client_nonce_len,
+                      strlen(hex_nonce)) != (int)client_nonce_len) {
-                      cp, client_nonce_encoded_len)
-                      != (int) client_nonce_len) {
       connection_write_str_to_buf("513 Invalid base16 client nonce\r\n",
                                   conn);
-      connection_mark_for_close(TO_CONN(conn));
       tor_free(client_nonce);
-      return -1;
+      goto fail;
     }
-
-    cp += client_nonce_encoded_len;
   }
 
-  cp += strspn(cp, " \t\n\r");
-  if (*cp != '\0' ||
-      cp != body + len) {
-    connection_write_str_to_buf("513 Junk at end of AUTHCHALLENGE command\r\n",
-                                conn);
-    connection_mark_for_close(TO_CONN(conn));
-    tor_free(client_nonce);
-    return -1;
-  }
   crypto_rand(server_nonce, SAFECOOKIE_SERVER_NONCE_LEN);
 
   /* Now compute and send the server-to-controller response, and the
@@ -233,38 +231,56 @@ handle_control_authchallenge(control_connection_t *conn, uint32_t len,
 
   tor_free(client_nonce);
   return 0;
+ fail:
+  connection_mark_for_close(TO_CONN(conn));
+  return -1;
 }
 
+const control_cmd_syntax_t authenticate_syntax = {
+   .max_args = 0,
+   .accept_keywords=true,
+   .kvline_flags=KV_OMIT_KEYS|KV_QUOTED_QSTRING,
+   .store_raw_body=true
+};
+
 /** Called when we get an AUTHENTICATE message.  Check whether the
  * authentication is valid, and if so, update the connection's state to
  * OPEN.  Reply with DONE or ERROR.
  */
 int
-handle_control_authenticate(control_connection_t *conn, uint32_t len,
+handle_control_authenticate(control_connection_t *conn,
-                            const char *body)
+                            const control_cmd_args_t *args)
 {
-  int used_quoted_string = 0;
+  bool used_quoted_string = false;
   const or_options_t *options = get_options();
   const char *errstr = "Unknown error";
   char *password;
   size_t password_len;
-  const char *cp;
-  int i;
   int bad_cookie=0, bad_password=0;
   smartlist_t *sl = NULL;
 
-  if (!len) {
+  if (args->kwargs == NULL) {
     password = tor_strdup("");
     password_len = 0;
-  } else if (TOR_ISXDIGIT(body[0])) {
+  } else if (args->kwargs->next) {
-    cp = body;
+    connection_write_str_to_buf(
-    while (TOR_ISXDIGIT(*cp))
+             "512 Too many arguments to AUTHENTICATE.\r\n", conn);
-      ++cp;
+    connection_mark_for_close(TO_CONN(conn));
-    i = (int)(cp - body);
+    return 0;
-    tor_assert(i>0);
+  } else if (strcmp(args->kwargs->key, "")) {
-    password_len = i/2;
+    connection_write_str_to_buf(
-    password = tor_malloc(password_len + 1);
+             "512 AUTHENTICATE does not accept keyword arguments.\r\n", conn);
-    if (base16_decode(password, password_len+1, body, i)
+    connection_mark_for_close(TO_CONN(conn));
+    return 0;
+  } else if (strchr(args->raw_body, '\"')) {
+    used_quoted_string = true;
+    password = tor_strdup(args->kwargs->value);
+    password_len = strlen(password);
+  } else {
+    const char *hex_passwd = args->kwargs->value;
+    password_len = strlen(hex_passwd) / 2;
+    password = tor_malloc(password_len+1);
+    if (base16_decode(password, password_len+1, hex_passwd, strlen(hex_passwd))
                       != (int) password_len) {
       connection_write_str_to_buf(
             "551 Invalid hexadecimal encoding.  Maybe you tried a plain text "
@@ -274,14 +290,6 @@ handle_control_authenticate(control_connection_t *conn, uint32_t len,
       tor_free(password);
       return 0;
     }
-  } else {
-    if (!decode_escaped_string(body, len, &password, &password_len)) {
-      connection_write_str_to_buf("551 Invalid quoted string.  You need "
-            "to put the password in double quotes.\r\n", conn);
-      connection_mark_for_close(TO_CONN(conn));
-      return 0;
-    }
-    used_quoted_string = 1;
   }
 
   if (conn->safecookie_client_hash != NULL) {

src/feature/control/control_auth.h

@@ -12,16 +12,21 @@
 #ifndef TOR_CONTROL_AUTH_H
 #define TOR_CONTROL_AUTH_H
 
+struct control_cmd_args_t;
+struct control_cmd_syntax_t;
+
 int init_control_cookie_authentication(int enabled);
 char *get_controller_cookie_file_name(void);
 struct config_line_t;
 smartlist_t *decode_hashed_passwords(struct config_line_t *passwords);
 
-int handle_control_authchallenge(control_connection_t *conn, uint32_t len,
+int handle_control_authchallenge(control_connection_t *conn,
-                                 const char *body);
+                                 const struct control_cmd_args_t *args);
 int handle_control_authenticate(control_connection_t *conn,
-                           uint32_t cmd_data_len,
+                                const struct control_cmd_args_t *args);
-                           const char *args);
 void control_auth_free_all(void);
 
+extern const struct control_cmd_syntax_t authchallenge_syntax;
+extern const struct control_cmd_syntax_t authenticate_syntax;
+
 #endif /* !defined(TOR_CONTROL_AUTH_H) */

src/feature/control/control_cmd.h

@@ -12,11 +12,68 @@
 #ifndef TOR_CONTROL_CMD_H
 #define TOR_CONTROL_CMD_H
 
+#include "lib/malloc/malloc.h"
+
 int handle_control_command(control_connection_t *conn,
                            uint32_t cmd_data_len,
                            char *args);
 void control_cmd_free_all(void);
 
+typedef struct control_cmd_args_t control_cmd_args_t;
+void control_cmd_args_free_(control_cmd_args_t *args);
+void control_cmd_args_wipe(control_cmd_args_t *args);
+
+#define control_cmd_args_free(v) \
+  FREE_AND_NULL(control_cmd_args_t, control_cmd_args_free_, (v))
+
+/**
+ * Definition for the syntax of a controller command, as parsed by
+ * control_cmd_parse_args.
+ *
+ * WORK IN PROGRESS: This structure is going to get more complex as this
+ * branch goes on.
+ **/
+typedef struct control_cmd_syntax_t {
+  /**
+   * Lowest number of positional arguments that this command accepts.
+   * 0 for "it's okay not to have positional arguments."
+   **/
+  unsigned int min_args;
+  /**
+   * Highest number of positional arguments that this command accepts.
+   * UINT_MAX for no limit.
+   **/
+  unsigned int max_args;
+  /**
+   * If true, we should parse options after the positional arguments
+   * as a set of unordered flags and key=value arguments.
+   *
+   * Requires that max_args is not UINT_MAX.
+   **/
+  bool accept_keywords;
+  /**
+   * If accept_keywords is true, then only the keywords listed in this
+   * (NULL-terminated) array are valid keywords for this command.
+   **/
+  const char **allowed_keywords;
+  /**
+   * If accept_keywords is true, this option is passed to kvline_parse() as
+   * its flags.
+   **/
+  unsigned kvline_flags;
+  /**
+   * True iff this command wants to be followed by a multiline object.
+   **/
+  bool want_cmddata;
+  /**
+   * True iff this command needs access to the raw body of the input.
+   *
+   * This should not be needed for pure commands; it is purely a legacy
+   * option.
+   **/
+  bool store_raw_body;
+} control_cmd_syntax_t;
+
 #ifdef CONTROL_CMD_PRIVATE
 #include "lib/crypt_ops/crypto_ed25519.h"
 
@@ -39,6 +96,13 @@ STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk,
 STATIC rend_authorized_client_t *add_onion_helper_clientauth(const char *arg,
                                    int *created, char **err_msg_out);
 
+STATIC control_cmd_args_t *control_cmd_parse_args(
+                                   const char *command,
+                                   const control_cmd_syntax_t *syntax,
+                                   size_t body_len,
+                                   const char *body,
+                                   char **error_out);
+
 #endif /* defined(CONTROL_CMD_PRIVATE) */
 
 #ifdef CONTROL_MODULE_PRIVATE

src/feature/control/control_cmd_args_st.h

@@ -0,0 +1,52 @@
+/* Copyright (c) 2001 Matej Pfajfar.
+ * Copyright (c) 2001-2004, Roger Dingledine.
+ * Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2019, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+/**
+ * \file control_cmd_args_st.h
+ * \brief Definition for control_cmd_args_t
+ **/
+
+#ifndef TOR_CONTROL_CMD_ST_H
+#define TOR_CONTROL_CMD_ST_H
+
+struct smartlist_t;
+struct config_line_t;
+
+/**
+ * Parsed arguments for a control command.
+ *
+ * WORK IN PROGRESS: This structure is going to get more complex as this
+ * branch goes on.
+ **/
+struct control_cmd_args_t {
+  /**
+   * The command itself, as provided by the controller.  Not owned by this
+   * structure.
+   **/
+  const char *command;
+  /**
+   * Positional arguments to the command.
+   **/
+  struct smartlist_t *args;
+  /**
+   * Keyword arguments to the command.
+   **/
+  struct config_line_t *kwargs;
+  /**
+   * Number of bytes in <b>cmddata</b>; 0 if <b>cmddata</b> is not set.
+   **/
+  size_t cmddata_len;
+  /**
+   * A multiline object passed with this command.
+   **/
+  char *cmddata;
+  /**
+   * If set, a nul-terminated string containing the raw unparsed arguments.
+   **/
+  const char *raw_body;
+};
+
+#endif /* !defined(TOR_CONTROL_CMD_ST_H) */

src/feature/control/control_connection_st.h

@@ -40,7 +40,8 @@ struct control_connection_t {
   /** A control command that we're reading from the inbuf, but which has not
    * yet arrived completely. */
   char *incoming_cmd;
+  /** The control command that we are currently processing. */
+  char *current_cmd;
 };
 
 #endif
-

src/feature/control/control_fmt.c

@@ -305,94 +305,6 @@ send_control_done(control_connection_t *conn)
   connection_write_str_to_buf("250 OK\r\n", conn);
 }
 
-/** If the first <b>in_len_max</b> characters in <b>start</b> contain a
- * double-quoted string with escaped characters, return the length of that
- * string (as encoded, including quotes).  Otherwise return -1. */
-static inline int
-get_escaped_string_length(const char *start, size_t in_len_max,
-                          int *chars_out)
-{
-  const char *cp, *end;
-  int chars = 0;
-
-  if (*start != '\"')
-    return -1;
-
-  cp = start+1;
-  end = start+in_len_max;
-
-  /* Calculate length. */
-  while (1) {
-    if (cp >= end) {
-      return -1; /* Too long. */
-    } else if (*cp == '\\') {
-      if (++cp == end)
-        return -1; /* Can't escape EOS. */
-      ++cp;
-      ++chars;
-    } else if (*cp == '\"') {
-      break;
-    } else {
-      ++cp;
-      ++chars;
-    }
-  }
-  if (chars_out)
-    *chars_out = chars;
-  return (int)(cp - start+1);
-}
-
-/** As decode_escaped_string, but does not decode the string: copies the
- * entire thing, including quotation marks. */
-const char *
-extract_escaped_string(const char *start, size_t in_len_max,
-                       char **out, size_t *out_len)
-{
-  int length = get_escaped_string_length(start, in_len_max, NULL);
-  if (length<0)
-    return NULL;
-  *out_len = length;
-  *out = tor_strndup(start, *out_len);
-  return start+length;
-}
-
-/** Given a pointer to a string starting at <b>start</b> containing
- * <b>in_len_max</b> characters, decode a string beginning with one double
- * quote, containing any number of non-quote characters or characters escaped
- * with a backslash, and ending with a final double quote.  Place the resulting
- * string (unquoted, unescaped) into a newly allocated string in *<b>out</b>;
- * store its length in <b>out_len</b>.  On success, return a pointer to the
- * character immediately following the escaped string.  On failure, return
- * NULL. */
-const char *
-decode_escaped_string(const char *start, size_t in_len_max,
-                   char **out, size_t *out_len)
-{
-  const char *cp, *end;
-  char *outp;
-  int len, n_chars = 0;
-
-  len = get_escaped_string_length(start, in_len_max, &n_chars);
-  if (len<0)
-    return NULL;
-
-  end = start+len-1; /* Index of last quote. */
-  tor_assert(*end == '\"');
-  outp = *out = tor_malloc(len+1);
-  *out_len = n_chars;
-
-  cp = start+1;
-  while (cp < end) {
-    if (*cp == '\\')
-      ++cp;
-    *outp++ = *cp++;
-  }
-  *outp = '\0';
-  tor_assert((outp - *out) == (int)*out_len);
-
-  return end+1;
-}
-
 /** Return a longname the node whose identity is <b>id_digest</b>. If
  * node_get_by_id() returns NULL, base 16 encoding of <b>id_digest</b> is
  * returned instead.

src/feature/control/control_fmt.h

@@ -25,10 +25,6 @@ char *circuit_describe_status_for_controller(origin_circuit_t *circ);
 
 size_t write_escaped_data(const char *data, size_t len, char **out);
 size_t read_escaped_data(const char *data, size_t len, char **out);
-const char *extract_escaped_string(const char *start, size_t in_len_max,
-                                   char **out, size_t *out_len);
-const char *decode_escaped_string(const char *start, size_t in_len_max,
-                                  char **out, size_t *out_len);
 void send_control_done(control_connection_t *conn);
 
 MOCK_DECL(const char *, node_describe_longname_by_id,(const char *id_digest));

src/feature/control/control_getinfo.c

@@ -55,6 +55,7 @@
 #include "core/or/origin_circuit_st.h"
 #include "core/or/socks_request_st.h"
 #include "feature/control/control_connection_st.h"
+#include "feature/control/control_cmd_args_st.h"
 #include "feature/dircache/cached_dir_st.h"
 #include "feature/nodelist/extrainfo_st.h"
 #include "feature/nodelist/microdesc_st.h"
@@ -1584,21 +1585,22 @@ handle_getinfo_helper(control_connection_t *control_conn,
   return 0; /* unrecognized */
 }
 
+const control_cmd_syntax_t getinfo_syntax = {
+  .max_args = UINT_MAX,
+};
+
 /** Called when we receive a GETINFO command.  Try to fetch all requested
  * information, and reply with information or error message. */
 int
-handle_control_getinfo(control_connection_t *conn, uint32_t len,
+handle_control_getinfo(control_connection_t *conn,
-                       const char *body)
+                       const control_cmd_args_t *args)
 {
-  smartlist_t *questions = smartlist_new();
+  const smartlist_t *questions = args->args;
   smartlist_t *answers = smartlist_new();
   smartlist_t *unrecognized = smartlist_new();
   char *ans = NULL;
   int i;
-  (void) len; /* body is NUL-terminated, so it's safe to ignore the length. */
 
-  smartlist_split_string(questions, body, " ",
-                         SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0);
   SMARTLIST_FOREACH_BEGIN(questions, const char *, q) {
     const char *errmsg = NULL;
 
@@ -1653,8 +1655,6 @@ handle_control_getinfo(control_connection_t *conn, uint32_t len,
  done:
   SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp));
   smartlist_free(answers);
-  SMARTLIST_FOREACH(questions, char *, cp, tor_free(cp));
-  smartlist_free(questions);
   SMARTLIST_FOREACH(unrecognized, char *, cp, tor_free(cp));
   smartlist_free(unrecognized);
 

src/feature/control/control_getinfo.h

@@ -12,8 +12,12 @@
 #ifndef TOR_CONTROL_GETINFO_H
 #define TOR_CONTROL_GETINFO_H
 
-int handle_control_getinfo(control_connection_t *conn, uint32_t len,
+struct control_cmd_syntax_t;
-                           const char *body);
+struct control_cmd_args_t;
+extern const struct control_cmd_syntax_t getinfo_syntax;
+
+int handle_control_getinfo(control_connection_t *conn,
+                           const struct control_cmd_args_t *args);
 
 #ifdef CONTROL_GETINFO_PRIVATE
 STATIC int getinfo_helper_onions(

src/lib/encoding/confline.c

@@ -82,6 +82,19 @@ config_line_find(const config_line_t *lines,
   return NULL;
 }
 
+/** As config_line_find(), but perform a case-insensitive comparison. */
+const config_line_t *
+config_line_find_case(const config_line_t *lines,
+                      const char *key)
+{
+  const config_line_t *cl;
+  for (cl = lines; cl; cl = cl->next) {
+    if (!strcasecmp(cl->key, key))
+      return cl;
+  }
+  return NULL;
+}
+
 /** Auxiliary function that does all the work of config_get_lines.
  * <b>recursion_level</b> is the count of how many nested %includes we have.
  * <b>opened_lst</b> will have a list of opened files if provided.

src/lib/encoding/confline.h

@@ -48,6 +48,8 @@ config_line_t *config_lines_dup_and_filter(const config_line_t *inp,
                                            const char *key);
 const config_line_t *config_line_find(const config_line_t *lines,
                                       const char *key);
+const config_line_t *config_line_find_case(const config_line_t *lines,
+                                           const char *key);
 int config_lines_eq(config_line_t *a, config_line_t *b);
 int config_count_key(const config_line_t *a, const char *key);
 void config_free_lines_(config_line_t *front);

src/lib/encoding/include.am

@@ -11,6 +11,7 @@ src_lib_libtor_encoding_a_SOURCES =			\
 	src/lib/encoding/keyval.c			\
 	src/lib/encoding/kvline.c			\
 	src/lib/encoding/pem.c				\
+	src/lib/encoding/qstring.c			\
 	src/lib/encoding/time_fmt.c
 
 src_lib_libtor_encoding_testing_a_SOURCES = \
@@ -25,4 +26,5 @@ noinst_HEADERS +=					\
 	src/lib/encoding/keyval.h			\
 	src/lib/encoding/kvline.h			\
 	src/lib/encoding/pem.h				\
+	src/lib/encoding/qstring.h			\
 	src/lib/encoding/time_fmt.h

src/lib/encoding/kvline.c

@@ -16,6 +16,7 @@
 #include "lib/encoding/confline.h"
 #include "lib/encoding/cstring.h"
 #include "lib/encoding/kvline.h"
+#include "lib/encoding/qstring.h"
 #include "lib/malloc/malloc.h"
 #include "lib/string/compat_ctype.h"
 #include "lib/string/printf.h"
@@ -53,6 +54,15 @@ line_has_no_key(const config_line_t *line)
   return line->key == NULL || strlen(line->key) == 0;
 }
 
+/**
+ * Return true iff the value in <b>line</b> is not set.
+ **/
+static bool
+line_has_no_val(const config_line_t *line)
+{
+  return line->value == NULL || strlen(line->value) == 0;
+}
+
 /**
  * Return true iff the all the lines in <b>line</b> can be encoded
  * using <b>flags</b>.
@@ -98,14 +108,25 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags)
  * If KV_OMIT_KEYS is set in <b>flags</b>, then pairs with empty keys are
  * allowed, and are encoded as 'Value'.  Otherwise, such pairs are not
  * allowed.
+ *
+ * If KV_OMIT_VALS is set in <b>flags</b>, then an empty value is
+ * encoded as 'Key', not as 'Key=' or 'Key=""'.  Mutually exclusive with
+ * KV_OMIT_KEYS.
+ *
+ * KV_QUOTED_QSTRING is not supported.
  */
 char *
 kvline_encode(const config_line_t *line,
               unsigned flags)
 {
+  tor_assert(! (flags & KV_QUOTED_QSTRING));
+
   if (!kvline_can_encode_lines(line, flags))
     return NULL;
 
+  tor_assert((flags & (KV_OMIT_KEYS|KV_OMIT_VALS)) !=
+             (KV_OMIT_KEYS|KV_OMIT_VALS));
+
   smartlist_t *elements = smartlist_new();
 
   for (; line; line = line->next) {
@@ -126,7 +147,10 @@ kvline_encode(const config_line_t *line,
       }
     }
 
-    if (esc) {
+    if ((flags & KV_OMIT_VALS) && line_has_no_val(line)) {
+      eq = "";
+      v = "";
+    } else if (esc) {
       tmp = esc_for_log(line->value);
       v = tmp;
     } else {
@@ -151,17 +175,30 @@ kvline_encode(const config_line_t *line,
  * allocated list of pairs on success, or NULL on failure.
  *
  * If KV_QUOTED is set in <b>flags</b>, then (double-)quoted values are
- * allowed. Otherwise, such values are not allowed.
+ * allowed and handled as C strings. Otherwise, such values are not allowed.
  *
  * If KV_OMIT_KEYS is set in <b>flags</b>, then values without keys are
  * allowed.  Otherwise, such values are not allowed.
+ *
+ * If KV_OMIT_VALS is set in <b>flags</b>, then keys without values are
+ * allowed.  Otherwise, such keys are not allowed.  Mutually exclusive with
+ * KV_OMIT_KEYS.
+ *
+ * If KV_QUOTED_QSTRING is set in <b>flags</b>, then double-quoted values
+ * are allowed and handled as QuotedStrings per qstring.c.  Do not add
+ * new users of this flag.
  */
 config_line_t *
 kvline_parse(const char *line, unsigned flags)
 {
+  tor_assert((flags & (KV_OMIT_KEYS|KV_OMIT_VALS)) !=
+             (KV_OMIT_KEYS|KV_OMIT_VALS));
+
   const char *cp = line, *cplast = NULL;
-  bool omit_keys = (flags & KV_OMIT_KEYS) != 0;
+  const bool omit_keys = (flags & KV_OMIT_KEYS) != 0;
-  bool quoted = (flags & KV_QUOTED) != 0;
+  const bool omit_vals = (flags & KV_OMIT_VALS) != 0;
+  const bool quoted = (flags & (KV_QUOTED|KV_QUOTED_QSTRING)) != 0;
+  const bool c_quoted = (flags & (KV_QUOTED)) != 0;
 
   config_line_t *result = NULL;
   config_line_t **next_line = &result;
@@ -171,27 +208,33 @@ kvline_parse(const char *line, unsigned flags)
 
   while (*cp) {
     key = val = NULL;
+    /* skip all spaces */
     {
       size_t idx = strspn(cp, " \t\r\v\n");
       cp += idx;
     }
     if (BUG(cp == cplast)) {
-      /* If we didn't parse anything, this code is broken. */
+      /* If we didn't parse anything since the last loop, this code is
+       * broken. */
       goto err; // LCOV_EXCL_LINE
     }
     cplast = cp;
     if (! *cp)
       break; /* End of string; we're done. */
 
-    /* Possible formats are K=V, K="V", V, and "V", depending on flags. */
+    /* Possible formats are K=V, K="V", K, V, and "V", depending on flags. */
 
-    /* Find the key. */
+    /* Find where the key ends */
     if (*cp != '\"') {
       size_t idx = strcspn(cp, " \t\r\v\n=");
 
       if (cp[idx] == '=') {
         key = tor_memdup_nulterm(cp, idx);
         cp += idx + 1;
+      } else if (omit_vals) {
+        key = tor_memdup_nulterm(cp, idx);
+        cp += idx;
+        goto commit;
       } else {
         if (!omit_keys)
           goto err;
@@ -203,7 +246,11 @@ kvline_parse(const char *line, unsigned flags)
       if (!quoted)
         goto err;
       size_t len=0;
-      cp = unescape_string(cp, &val, &len);
+      if (c_quoted) {
+        cp = unescape_string(cp, &val, &len);
+      } else {
+        cp = decode_qstring(cp, strlen(cp), &val, &len);
+      }
       if (cp == NULL || len != strlen(val)) {
         // The string contains a NUL or is badly coded.
         goto err;
@@ -214,6 +261,7 @@ kvline_parse(const char *line, unsigned flags)
       cp += idx;
     }
 
+  commit:
     if (key && strlen(key) == 0) {
       /* We don't allow empty keys. */
       goto err;
@@ -221,13 +269,15 @@ kvline_parse(const char *line, unsigned flags)
 
     *next_line = tor_malloc_zero(sizeof(config_line_t));
     (*next_line)->key = key ? key : tor_strdup("");
-    (*next_line)->value = val;
+    (*next_line)->value = val ? val : tor_strdup("");
     next_line = &(*next_line)->next;
     key = val = NULL;
   }
 
-  if (!kvline_can_encode_lines(result, flags)) {
+  if (! (flags & KV_QUOTED_QSTRING)) {
-    goto err;
+    if (!kvline_can_encode_lines(result, flags)) {
+      goto err;
+    }
   }
   return result;
 

src/lib/encoding/kvline.h

@@ -17,6 +17,8 @@ struct config_line_t;
 
 #define KV_QUOTED    (1u<<0)
 #define KV_OMIT_KEYS (1u<<1)
+#define KV_OMIT_VALS (1u<<2)
+#define KV_QUOTED_QSTRING (1u<<3)
 
 struct config_line_t *kvline_parse(const char *line, unsigned flags);
 char *kvline_encode(const struct config_line_t *line, unsigned flags);

src/lib/encoding/qstring.c

@@ -0,0 +1,90 @@
+/* Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2019, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+/**
+ * \file qstring.c
+ * \brief Implement QuotedString parsing.
+ *
+ * Note that this is only used for controller authentication; do not
+ * create new users for this.  Instead, prefer the cstring.c functions.
+ **/
+
+#include "orconfig.h"
+#include "lib/encoding/qstring.h"
+#include "lib/malloc/malloc.h"
+#include "lib/log/util_bug.h"
+
+/** If the first <b>in_len_max</b> characters in <b>start</b> contain a
+ * QuotedString, return the length of that
+ * string (as encoded, including quotes).  Otherwise return -1. */
+static inline int
+get_qstring_length(const char *start, size_t in_len_max,
+                          int *chars_out)
+{
+  const char *cp, *end;
+  int chars = 0;
+
+  if (*start != '\"')
+    return -1;
+
+  cp = start+1;
+  end = start+in_len_max;
+
+  /* Calculate length. */
+  while (1) {
+    if (cp >= end) {
+      return -1; /* Too long. */
+    } else if (*cp == '\\') {
+      if (++cp == end)
+        return -1; /* Can't escape EOS. */
+      ++cp;
+      ++chars;
+    } else if (*cp == '\"') {
+      break;
+    } else {
+      ++cp;
+      ++chars;
+    }
+  }
+  if (chars_out)
+    *chars_out = chars;
+  return (int)(cp - start+1);
+}
+
+/** Given a pointer to a string starting at <b>start</b> containing
+ * <b>in_len_max</b> characters, decode a string beginning with one double
+ * quote, containing any number of non-quote characters or characters escaped
+ * with a backslash, and ending with a final double quote.  Place the resulting
+ * string (unquoted, unescaped) into a newly allocated string in *<b>out</b>;
+ * store its length in <b>out_len</b>.  On success, return a pointer to the
+ * character immediately following the escaped string.  On failure, return
+ * NULL. */
+const char *
+decode_qstring(const char *start, size_t in_len_max,
+               char **out, size_t *out_len)
+{
+  const char *cp, *end;
+  char *outp;
+  int len, n_chars = 0;
+
+  len = get_qstring_length(start, in_len_max, &n_chars);
+  if (len<0)
+    return NULL;
+
+  end = start+len-1; /* Index of last quote. */
+  tor_assert(*end == '\"');
+  outp = *out = tor_malloc(len+1);
+  *out_len = n_chars;
+
+  cp = start+1;
+  while (cp < end) {
+    if (*cp == '\\')
+      ++cp;
+    *outp++ = *cp++;
+  }
+  *outp = '\0';
+  tor_assert((outp - *out) == (int)*out_len);
+
+  return end+1;
+}

src/lib/encoding/qstring.h

@@ -0,0 +1,18 @@
+/* Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2019, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+/**
+ * \file qstring.h
+ * \brief Header for qstring.c
+ */
+
+#ifndef TOR_ENCODING_QSTRING_H
+#define TOR_ENCODING_QSTRING_H
+
+#include <stddef.h>
+
+const char *decode_qstring(const char *start, size_t in_len_max,
+                           char **out, size_t *out_len);
+
+#endif

src/test/fuzz/fuzz_strops.c

@@ -235,6 +235,18 @@ fuzz_main(const uint8_t *stdin_buf, size_t data_size)
       kv_flags = 0;
       ENCODE_ROUNDTRIP(kv_enc, kv_dec, config_free_lines_);
       break;
+    case 7:
+      kv_flags = KV_OMIT_VALS;
+      ENCODE_ROUNDTRIP(kv_enc, kv_dec, config_free_lines_);
+      break;
+    case 8:
+      kv_flags = KV_QUOTED;
+      ENCODE_ROUNDTRIP(kv_enc, kv_dec, config_free_lines_);
+      break;
+    case 9:
+      kv_flags = KV_QUOTED|KV_OMIT_VALS;
+      ENCODE_ROUNDTRIP(kv_enc, kv_dec, config_free_lines_);
+      break;
     }
 
   return 0;

src/test/test_config.c

@@ -5886,6 +5886,61 @@ test_config_kvline_parse(void *arg)
   tt_assert(lines);
   tt_str_op(lines->key, OP_EQ, "AB");
   tt_str_op(lines->value, OP_EQ, "");
+  config_free_lines(lines);
+
+  lines = kvline_parse("AB=", KV_OMIT_VALS);
+  tt_assert(lines);
+  tt_str_op(lines->key, OP_EQ, "AB");
+  tt_str_op(lines->value, OP_EQ, "");
+  config_free_lines(lines);
+
+  lines = kvline_parse(" AB ", KV_OMIT_VALS);
+  tt_assert(lines);
+  tt_str_op(lines->key, OP_EQ, "AB");
+  tt_str_op(lines->value, OP_EQ, "");
+  config_free_lines(lines);
+
+  lines = kvline_parse("AB", KV_OMIT_VALS);
+  tt_assert(lines);
+  tt_str_op(lines->key, OP_EQ, "AB");
+  tt_str_op(lines->value, OP_EQ, "");
+  enc = kvline_encode(lines, KV_OMIT_VALS);
+  tt_str_op(enc, OP_EQ, "AB");
+  tor_free(enc);
+  config_free_lines(lines);
+
+  lines = kvline_parse("AB=CD", KV_OMIT_VALS);
+  tt_assert(lines);
+  tt_str_op(lines->key, OP_EQ, "AB");
+  tt_str_op(lines->value, OP_EQ, "CD");
+  enc = kvline_encode(lines, KV_OMIT_VALS);
+  tt_str_op(enc, OP_EQ, "AB=CD");
+  tor_free(enc);
+  config_free_lines(lines);
+
+  lines = kvline_parse("AB=CD DE FGH=I", KV_OMIT_VALS);
+  tt_assert(lines);
+  tt_str_op(lines->key, OP_EQ, "AB");
+  tt_str_op(lines->value, OP_EQ, "CD");
+  tt_str_op(lines->next->key, OP_EQ, "DE");
+  tt_str_op(lines->next->value, OP_EQ, "");
+  tt_str_op(lines->next->next->key, OP_EQ, "FGH");
+  tt_str_op(lines->next->next->value, OP_EQ, "I");
+  enc = kvline_encode(lines, KV_OMIT_VALS);
+  tt_str_op(enc, OP_EQ, "AB=CD DE FGH=I");
+  tor_free(enc);
+  config_free_lines(lines);
+
+  lines = kvline_parse("AB=\"CD E\" DE FGH=\"I\"", KV_OMIT_VALS|KV_QUOTED);
+  tt_assert(lines);
+  tt_str_op(lines->key, OP_EQ, "AB");
+  tt_str_op(lines->value, OP_EQ, "CD E");
+  tt_str_op(lines->next->key, OP_EQ, "DE");
+  tt_str_op(lines->next->value, OP_EQ, "");
+  tt_str_op(lines->next->next->key, OP_EQ, "FGH");
+  tt_str_op(lines->next->next->value, OP_EQ, "I");
+  enc = kvline_encode(lines, KV_OMIT_VALS|KV_QUOTED);
+  tt_str_op(enc, OP_EQ, "AB=\"CD E\" DE FGH=I");
 
  done:
   config_free_lines(lines);

src/test/test_controller.c

@@ -18,12 +18,189 @@
 #include "test/test.h"
 #include "test/test_helpers.h"
 #include "lib/net/resolve.h"
+#include "lib/encoding/confline.h"
+#include "lib/encoding/kvline.h"
 
 #include "feature/control/control_connection_st.h"
+#include "feature/control/control_cmd_args_st.h"
 #include "feature/dirclient/download_status_st.h"
 #include "feature/nodelist/microdesc_st.h"
 #include "feature/nodelist/node_st.h"
 
+typedef struct {
+  const char *input;
+  const char *expected_parse;
+  const char *expected_error;
+} parser_testcase_t;
+
+typedef struct {
+  const control_cmd_syntax_t *syntax;
+  size_t n_testcases;
+  const parser_testcase_t *testcases;
+} parse_test_params_t;
+
+static char *
+control_cmd_dump_args(const control_cmd_args_t *result)
+{
+  buf_t *buf = buf_new();
+  buf_add_string(buf, "{ args=[");
+  if (result->args) {
+    if (smartlist_len(result->args)) {
+        buf_add_string(buf, " ");
+    }
+    SMARTLIST_FOREACH_BEGIN(result->args, const char *, s) {
+      const bool last = (s_sl_idx == smartlist_len(result->args)-1);
+      buf_add_printf(buf, "%s%s ",
+                     escaped(s),
+                     last ? "" : ",");
+    } SMARTLIST_FOREACH_END(s);
+  }
+  buf_add_string(buf, "]");
+  if (result->cmddata) {
+    buf_add_string(buf, ", obj=");
+    buf_add_string(buf, escaped(result->cmddata));
+  }
+  if (result->kwargs) {
+    buf_add_string(buf, ", { ");
+    const config_line_t *line;
+    for (line = result->kwargs; line; line = line->next) {
+      const bool last = (line->next == NULL);
+      buf_add_printf(buf, "%s=%s%s ", line->key, escaped(line->value),
+                     last ? "" : ",");
+    }
+    buf_add_string(buf, "}");
+  }
+  buf_add_string(buf, " }");
+
+  char *encoded = buf_extract(buf, NULL);
+  buf_free(buf);
+  return encoded;
+}
+
+static void
+test_controller_parse_cmd(void *arg)
+{
+  const parse_test_params_t *params = arg;
+  control_cmd_args_t *result = NULL;
+  char *error = NULL;
+  char *encoded = NULL;
+
+  for (size_t i = 0; i < params->n_testcases; ++i) {
+    const parser_testcase_t *t = &params->testcases[i];
+    result = control_cmd_parse_args("EXAMPLE",
+                                    params->syntax,
+                                    strlen(t->input),
+                                    t->input,
+                                    &error);
+    // A valid test should expect exactly one parse or error.
+    tt_int_op((t->expected_parse == NULL), OP_NE,
+              (t->expected_error == NULL));
+    // We get a result or an error, not both.
+    tt_int_op((result == NULL), OP_EQ, (error != NULL));
+    // We got the one we expected.
+    tt_int_op((result == NULL), OP_EQ, (t->expected_parse == NULL));
+
+    if (result) {
+      encoded = control_cmd_dump_args(result);
+      tt_str_op(encoded, OP_EQ, t->expected_parse);
+    } else {
+      tt_str_op(error, OP_EQ, t->expected_error);
+    }
+
+    tor_free(error);
+    tor_free(encoded);
+    control_cmd_args_free(result);
+  }
+
+ done:
+  tor_free(error);
+  tor_free(encoded);
+  control_cmd_args_free(result);
+}
+
+#define OK(inp, out) \
+  { inp "\r\n", out, NULL }
+#define ERR(inp, err) \
+  { inp "\r\n", NULL, err }
+
+#define TESTPARAMS(syntax, array)                \
+  { &syntax,                                     \
+      ARRAY_LENGTH(array),                       \
+      array }
+
+static const parser_testcase_t one_to_three_tests[] = {
+   ERR("", "Need at least 1 argument(s)"),
+   ERR("   \t", "Need at least 1 argument(s)"),
+   OK("hello", "{ args=[ \"hello\" ] }"),
+   OK("hello world", "{ args=[ \"hello\", \"world\" ] }"),
+   OK("hello  world", "{ args=[ \"hello\", \"world\" ] }"),
+   OK("  hello  world", "{ args=[ \"hello\", \"world\" ] }"),
+   OK("  hello  world      ", "{ args=[ \"hello\", \"world\" ] }"),
+   OK("hello there world", "{ args=[ \"hello\", \"there\", \"world\" ] }"),
+   ERR("why hello there world", "Cannot accept more than 3 argument(s)"),
+   ERR("hello\r\nworld.\r\n.", "Unexpected body"),
+};
+
+static const control_cmd_syntax_t one_to_three_syntax = {
+   .min_args=1, .max_args=3
+};
+
+static const parse_test_params_t parse_one_to_three_params =
+  TESTPARAMS( one_to_three_syntax, one_to_three_tests );
+
+// =
+static const parser_testcase_t no_args_one_obj_tests[] = {
+  ERR("Hi there!\r\n.", "Cannot accept more than 0 argument(s)"),
+  ERR("", "Empty body"),
+  OK("\r\n", "{ args=[], obj=\"\\n\" }"),
+  OK("\r\nHello world\r\n", "{ args=[], obj=\"Hello world\\n\\n\" }"),
+  OK("\r\nHello\r\nworld\r\n", "{ args=[], obj=\"Hello\\nworld\\n\\n\" }"),
+  OK("\r\nHello\r\n..\r\nworld\r\n",
+     "{ args=[], obj=\"Hello\\n.\\nworld\\n\\n\" }"),
+};
+static const control_cmd_syntax_t no_args_one_obj_syntax = {
+   .min_args=0, .max_args=0,
+   .want_cmddata=true,
+};
+static const parse_test_params_t parse_no_args_one_obj_params =
+  TESTPARAMS( no_args_one_obj_syntax, no_args_one_obj_tests );
+
+static const parser_testcase_t no_args_kwargs_tests[] = {
+  OK("", "{ args=[] }"),
+  OK(" ", "{ args=[] }"),
+  OK("hello there=world", "{ args=[], { hello=\"\", there=\"world\" } }"),
+  OK("hello there=world today",
+     "{ args=[], { hello=\"\", there=\"world\", today=\"\" } }"),
+  ERR("=Foo", "Cannot parse keyword argument(s)"),
+};
+static const control_cmd_syntax_t no_args_kwargs_syntax = {
+   .min_args=0, .max_args=0,
+   .accept_keywords=true,
+   .kvline_flags=KV_OMIT_VALS
+};
+static const parse_test_params_t parse_no_args_kwargs_params =
+  TESTPARAMS( no_args_kwargs_syntax, no_args_kwargs_tests );
+
+static const char *one_arg_kwargs_allow_keywords[] = {
+  "Hello", "world", NULL
+};
+static const parser_testcase_t one_arg_kwargs_tests[] = {
+  ERR("", "Need at least 1 argument(s)"),
+  OK("Hi", "{ args=[ \"Hi\" ] }"),
+  ERR("hello there=world", "Unrecognized keyword argument \"there\""),
+  OK("Hi HELLO=foo", "{ args=[ \"Hi\" ], { HELLO=\"foo\" } }"),
+  OK("Hi world=\"bar baz\" hello  ",
+     "{ args=[ \"Hi\" ], { world=\"bar baz\", hello=\"\" } }"),
+};
+static const control_cmd_syntax_t one_arg_kwargs_syntax = {
+   .min_args=1, .max_args=1,
+   .accept_keywords=true,
+   .allowed_keywords=one_arg_kwargs_allow_keywords,
+   .kvline_flags=KV_OMIT_VALS|KV_QUOTED,
+};
+static const parse_test_params_t parse_one_arg_kwargs_params =
+  TESTPARAMS( one_arg_kwargs_syntax, one_arg_kwargs_tests );
+
 static void
 test_add_onion_helper_keyarg_v3(void *arg)
 {
@@ -1617,7 +1794,15 @@ test_getinfo_md_all(void *arg)
   return;
 }
 
+#define PARSER_TEST(type)                                             \
+  { "parse/" #type, test_controller_parse_cmd, 0, &passthrough_setup, \
+      (void*)&parse_ ## type ## _params }
+
 struct testcase_t controller_tests[] = {
+  PARSER_TEST(one_to_three),
+  PARSER_TEST(no_args_one_obj),
+  PARSER_TEST(no_args_kwargs),
+  PARSER_TEST(one_arg_kwargs),
   { "add_onion_helper_keyarg_v2", test_add_onion_helper_keyarg_v2, 0,
     NULL, NULL },
   { "add_onion_helper_keyarg_v3", test_add_onion_helper_keyarg_v3, 0,