When parsing a multiline controller command, be careful with linebreaks

The first line break in particular was mishandled: it was discarded
if no arguments came before it, which made it impossible to
distinguish arguments from the first line of the body.

To solve this, we need to allocate a copy of the command rather than
using NUL to separate it, since we might have "COMMAND\n" as our input.

Fixes ticket 29984.

7 files changed, 48 insertions, 23 deletions

diff --git a/changes/ticket29984 b/changes/ticket29984
new file mode 100644
index 0000000000..8631dff27b
--- /dev/null
+++ b/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.
diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt
index 7d03bf27d6..7582395fea 100644
--- a/scripts/maint/practracker/exceptions.txt
+++ b/scripts/maint/practracker/exceptions.txt
@@ -56,7 +56,7 @@ 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 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
diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c
index 51c19b4c4c..30504e4edb 100644
--- a/src/core/mainloop/connection.c
+++ b/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));
diff --git a/src/feature/control/control.c b/src/feature/control/control.c
index 308f1936b9..23ef83ef95 100644
--- a/src/feature/control/control.c
+++ b/src/feature/control/control.c
@@ -276,25 +276,38 @@ peek_connection_has_http_command(connection_t *conn)
 }
 
 /**
- * Helper: take a nul-terminated command of given length, and find where
- * the command starts and the argument begins.  Separate them with a NUL,
- * and return a pointer to the arguments.
+ * 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)
+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;
 
-  incoming_cmd[cmd_len]='\0';
-  char *args = incoming_cmd+cmd_len+1;
-  tor_assert(*data_len>cmd_len);
-  *data_len -= (cmd_len+1); /* skip the command and NUL we added after it */
-  while (TOR_ISSPACE(*args)) {
-    ++args;
-    --*data_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;
@@ -429,7 +442,11 @@ connection_control_process_inbuf(control_connection_t *conn)
   /* Okay, we now have a command sitting on conn->incoming_cmd. See if we
    * recognize it.
    */
-  args = control_split_incoming_command(conn->incoming_cmd, &data_len);
+  tor_free(conn->current_cmd);
+  args = control_split_incoming_command(conn->incoming_cmd, &data_len,
+                                        &conn->current_cmd);
+  if (BUG(!conn->current_cmd))
+    return -1;
 
   /* If the connection is already closing, ignore further commands */
   if (TO_CONN(conn)->marked_for_close) {
@@ -437,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;
diff --git a/src/feature/control/control.h b/src/feature/control/control.h
index 6fc1c40cac..8d3595d2ed 100644
--- a/src/feature/control/control.h
+++ b/src/feature/control/control.h
@@ -62,7 +62,8 @@ void set_cached_network_liveness(int liveness);
 
 #ifdef CONTROL_PRIVATE
 STATIC char *control_split_incoming_command(char *incoming_cmd,
-                                            size_t *data_len);
+                                            size_t *data_len,
+                                            char **current_cmd_out);
 #endif
 
 #endif /* !defined(TOR_CONTROL_H) */
diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c
index aa0bef5135..53cbf2bd0f 100644
--- a/src/feature/control/control_cmd.c
+++ b/src/feature/control/control_cmd.c
@@ -2299,7 +2299,7 @@ handle_control_obsolete(control_connection_t *conn,
 {
   (void)arg_len;
   (void)args;
-  char *command = tor_strdup(conn->incoming_cmd);
+  char *command = tor_strdup(conn->current_cmd);
   tor_strupper(command);
   connection_printf_to_buf(conn, "511 %s is obsolete.\r\n", command);
   tor_free(command);
@@ -2490,14 +2490,14 @@ handle_single_control_command(const control_cmd_def_t *def,
       control_cmd_args_t *parsed_args;
       char *err=NULL;
       tor_assert(def->syntax);
-      parsed_args = control_cmd_parse_args(conn->incoming_cmd,
+      parsed_args = control_cmd_parse_args(conn->current_cmd,
                                            def->syntax,
                                            cmd_data_len, args,
                                            &err);
       if (!parsed_args) {
         connection_printf_to_buf(conn,
                                  "512 Bad arguments to %s: %s\r\n",
-                                 conn->incoming_cmd, err?err:"");
+                                 conn->current_cmd, err?err:"");
         tor_free(err);
       } else {
         if (BUG(err))
@@ -2519,7 +2519,7 @@ handle_single_control_command(const control_cmd_def_t *def,
 }
 
 /**
- * Run a given controller command, as selected by the incoming_cmd field of
+ * Run a given controller command, as selected by the current_cmd field of
  * <b>conn</b>.
  */
 int
@@ -2533,13 +2533,13 @@ handle_control_command(control_connection_t *conn,
 
   for (unsigned i = 0; i < N_CONTROL_COMMANDS; ++i) {
     const control_cmd_def_t *def = &CONTROL_COMMANDS[i];
-    if (!strcasecmp(conn->incoming_cmd, def->name)) {
+    if (!strcasecmp(conn->current_cmd, def->name)) {
       return handle_single_control_command(def, conn, cmd_data_len, args);
     }
   }
 
   connection_printf_to_buf(conn, "510 Unrecognized command \"%s\"\r\n",
-                           conn->incoming_cmd);
+                           conn->current_cmd);
 
   return 0;
 }
diff --git a/src/feature/control/control_connection_st.h b/src/feature/control/control_connection_st.h
index 177a916257..cace6bb36f 100644
--- a/src/feature/control/control_connection_st.h
+++ b/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
-