Merge remote-tracking branch 'tor-github/pr/1505'

10 files changed, 464 insertions, 113 deletions

diff --git a/Doxyfile.in b/Doxyfile.in
index c99f536e60..503c1302db 100644
--- a/Doxyfile.in
+++ b/Doxyfile.in
@@ -2117,7 +2117,8 @@ PREDEFINED             = "MOCK_IMPL(a,b,c)=a b c" \
                          __attribute__(x)= \
                          "BEGIN_CONF_STRUCT(x)=struct x {" \
                          "END_CONF_STRUCT(x)=};" \
-                         "CONF_VAR(a,b,c,d)=b a;"
+                         "CONF_VAR(a,b,c,d)=b a;" \
+                         "CHECK_PRINTF(a, b)="
 
 
 # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
diff --git a/changes/ticket30984 b/changes/ticket30984
new file mode 100644
index 0000000000..de7d055415
--- /dev/null
+++ b/changes/ticket30984
@@ -0,0 +1,4 @@
+  o Code simplification and refactoring:
+    - Create a new abstraction for formatting control protocol reply
+      lines based on key-value pairs.  Refactor some existing control
+      protocol code to take advantage of this.  Closes ticket 30984.
diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c
index 656ddf5ca1..cc4375112f 100644
--- a/src/feature/control/control_cmd.c
+++ b/src/feature/control/control_cmd.c
@@ -289,26 +289,23 @@ handle_control_getconf(control_connection_t *conn,
   const smartlist_t *questions = args->args;
   smartlist_t *answers = smartlist_new();
   smartlist_t *unrecognized = smartlist_new();
-  char *msg = NULL;
-  size_t msg_len;
   const or_options_t *options = get_options();
-  int i, len;
 
   SMARTLIST_FOREACH_BEGIN(questions, const char *, q) {
     if (!option_is_recognized(q)) {
-      smartlist_add(unrecognized, (char*) q);
+      control_reply_add_printf(unrecognized, 552,
+                               "Unrecognized configuration key \"%s\"", q);
     } else {
       config_line_t *answer = option_get_assignment(options,q);
       if (!answer) {
         const char *name = option_get_canonical_name(q);
-        smartlist_add_asprintf(answers, "250-%s\r\n", name);
+        control_reply_add_one_kv(answers, 250, KV_OMIT_VALS, name, "");
       }
 
       while (answer) {
         config_line_t *next;
-        smartlist_add_asprintf(answers, "250-%s=%s\r\n",
-                     answer->key, answer->value);
-
+        control_reply_add_one_kv(answers, 250, KV_RAW, answer->key,
+                                 answer->value);
         next = answer->next;
         tor_free(answer->key);
         tor_free(answer->value);
@@ -318,20 +315,10 @@ handle_control_getconf(control_connection_t *conn,
     }
   } SMARTLIST_FOREACH_END(q);
 
-  if ((len = smartlist_len(unrecognized))) {
-    for (i=0; i < len-1; ++i)
-      control_printf_midreply(conn, 552,
-                              "Unrecognized configuration key \"%s\"",
-                              (char*)smartlist_get(unrecognized, i));
-    control_printf_endreply(conn, 552,
-                            "Unrecognized configuration key \"%s\"",
-                            (char*)smartlist_get(unrecognized, len-1));
-  } else if ((len = smartlist_len(answers))) {
-    char *tmp = smartlist_get(answers, len-1);
-    tor_assert(strlen(tmp)>4);
-    tmp[3] = ' ';
-    msg = smartlist_join_strings(answers, "", 0, &msg_len);
-    connection_buf_add(msg, msg_len, TO_CONN(conn));
+  if (smartlist_len(unrecognized)) {
+    control_write_reply_lines(conn, unrecognized);
+  } else if (smartlist_len(answers)) {
+    control_write_reply_lines(conn, answers);
   } else {
     send_control_done(conn);
   }
@@ -340,8 +327,6 @@ handle_control_getconf(control_connection_t *conn,
   smartlist_free(answers);
   smartlist_free(unrecognized);
 
-  tor_free(msg);
-
   return 0;
 }
 
@@ -1257,6 +1242,66 @@ static const control_cmd_syntax_t protocolinfo_syntax = {
   .max_args = UINT_MAX
 };
 
+/** Return a comma-separated list of authentication methods for
+    handle_control_protocolinfo().  Caller must free this string. */
+static char *
+get_authmethods(const or_options_t *options)
+{
+  int cookies = options->CookieAuthentication;
+  char *methods;
+  int passwd = (options->HashedControlPassword != NULL ||
+                options->HashedControlSessionPassword != NULL);
+  smartlist_t *mlist = smartlist_new();
+
+  if (cookies) {
+    smartlist_add(mlist, (char*)"COOKIE");
+    smartlist_add(mlist, (char*)"SAFECOOKIE");
+  }
+  if (passwd)
+    smartlist_add(mlist, (char*)"HASHEDPASSWORD");
+  if (!cookies && !passwd)
+    smartlist_add(mlist, (char*)"NULL");
+  methods = smartlist_join_strings(mlist, ",", 0, NULL);
+  smartlist_free(mlist);
+
+  return methods;
+}
+
+/** Return escaped cookie filename.  Caller must free this string.
+    Return NULL if cookie authentication is disabled. */
+static char *
+get_esc_cfile(const or_options_t *options)
+{
+  char *cfile = NULL, *abs_cfile = NULL, *esc_cfile = NULL;
+
+  if (!options->CookieAuthentication)
+    return NULL;
+
+  cfile = get_controller_cookie_file_name();
+  abs_cfile = make_path_absolute(cfile);
+  esc_cfile = esc_for_log(abs_cfile);
+  tor_free(cfile);
+  tor_free(abs_cfile);
+  return esc_cfile;
+}
+
+/** Compose the auth methods line of a PROTOCOLINFO reply. */
+static void
+add_authmethods(smartlist_t *reply)
+{
+  const or_options_t *options = get_options();
+  char *methods = get_authmethods(options);
+  char *esc_cfile = get_esc_cfile(options);
+
+  control_reply_add_str(reply, 250, "AUTH");
+  control_reply_append_kv(reply, "METHODS", methods);
+  if (esc_cfile)
+    control_reply_append_kv(reply, "COOKIEFILE", esc_cfile);
+
+  tor_free(methods);
+  tor_free(esc_cfile);
+}
+
 /** Called when we get a PROTOCOLINFO command: send back a reply. */
 static int
 handle_control_protocolinfo(control_connection_t *conn,
@@ -1264,6 +1309,7 @@ handle_control_protocolinfo(control_connection_t *conn,
 {
   const char *bad_arg = NULL;
   const smartlist_t *args = cmd_args->args;
+  smartlist_t *reply = NULL;
 
   conn->have_sent_protocolinfo = 1;
 
@@ -1281,45 +1327,17 @@ handle_control_protocolinfo(control_connection_t *conn,
     /* Don't tolerate bad arguments when not authenticated. */
     if (!STATE_IS_OPEN(TO_CONN(conn)->state))
       connection_mark_for_close(TO_CONN(conn));
-    goto done;
-  } else {
-    const or_options_t *options = get_options();
-    int cookies = options->CookieAuthentication;
-    char *cfile = get_controller_cookie_file_name();
-    char *abs_cfile;
-    char *esc_cfile;
-    char *methods;
-    abs_cfile = make_path_absolute(cfile);
-    esc_cfile = esc_for_log(abs_cfile);
-    {
-      int passwd = (options->HashedControlPassword != NULL ||
-                    options->HashedControlSessionPassword != NULL);
-      smartlist_t *mlist = smartlist_new();
-      if (cookies) {
-        smartlist_add(mlist, (char*)"COOKIE");
-        smartlist_add(mlist, (char*)"SAFECOOKIE");
-      }
-      if (passwd)
-        smartlist_add(mlist, (char*)"HASHEDPASSWORD");
-      if (!cookies && !passwd)
-        smartlist_add(mlist, (char*)"NULL");
-      methods = smartlist_join_strings(mlist, ",", 0, NULL);
-      smartlist_free(mlist);
-    }
-
-    control_write_midreply(conn, 250, "PROTOCOLINFO 1");
-    control_printf_midreply(conn, 250, "AUTH METHODS=%s%s%s", methods,
-                            cookies?" COOKIEFILE=":"",
-                            cookies?esc_cfile:"");
-    control_printf_midreply(conn, 250, "VERSION Tor=%s", escaped(VERSION));
-    send_control_done(conn);
-
-    tor_free(methods);
-    tor_free(cfile);
-    tor_free(abs_cfile);
-    tor_free(esc_cfile);
+    return 0;
   }
- done:
+  reply = smartlist_new();
+  control_reply_add_str(reply, 250, "PROTOCOLINFO 1");
+  add_authmethods(reply);
+  control_reply_add_str(reply, 250, "VERSION");
+  control_reply_append_kv(reply, "Tor", escaped(VERSION));
+  control_reply_add_done(reply);
+
+  control_write_reply_lines(conn, reply);
+  control_reply_free(reply);
   return 0;
 }
 
diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c
index 979fa4480d..cff4a08793 100644
--- a/src/feature/control/control_getinfo.c
+++ b/src/feature/control/control_getinfo.c
@@ -50,6 +50,7 @@
 #include "feature/stats/geoip_stats.h"
 #include "feature/stats/predict_ports.h"
 #include "lib/version/torversion.h"
+#include "lib/encoding/kvline.h"
 
 #include "core/or/entry_connection_st.h"
 #include "core/or/or_connection_st.h"
@@ -1632,7 +1633,6 @@ handle_control_getinfo(control_connection_t *conn,
   smartlist_t *answers = smartlist_new();
   smartlist_t *unrecognized = smartlist_new();
   char *ans = NULL;
-  int i;
 
   SMARTLIST_FOREACH_BEGIN(questions, const char *, q) {
     const char *errmsg = NULL;
@@ -1644,43 +1644,32 @@ handle_control_getinfo(control_connection_t *conn,
       goto done;
     }
     if (!ans) {
-      if (errmsg) /* use provided error message */
-        smartlist_add_strdup(unrecognized, errmsg);
-      else /* use default error message */
-        smartlist_add_asprintf(unrecognized, "Unrecognized key \"%s\"", q);
+      if (errmsg) {
+        /* use provided error message */
+        control_reply_add_str(unrecognized, 552, errmsg);
+      } else {
+        /* use default error message */
+        control_reply_add_printf(unrecognized, 552,
+                                 "Unrecognized key \"%s\"", q);
+      }
     } else {
-      smartlist_add_strdup(answers, q);
-      smartlist_add(answers, ans);
+      control_reply_add_one_kv(answers, 250, KV_RAW, q, ans);
     }
   } SMARTLIST_FOREACH_END(q);
 
-  if (smartlist_len(unrecognized)) {
-    /* control-spec section 2.3, mid-reply '-' or end of reply ' ' */
-    for (i=0; i < smartlist_len(unrecognized)-1; ++i)
-      control_write_midreply(conn, 552,
-                             (char *)smartlist_get(unrecognized, i));
+  control_reply_add_done(answers);
 
-    control_write_endreply(conn, 552, (char *)smartlist_get(unrecognized, i));
+  if (smartlist_len(unrecognized)) {
+    control_write_reply_lines(conn, unrecognized);
+    /* If there were any unrecognized queries, don't write real answers */
     goto done;
   }
 
-  for (i = 0; i < smartlist_len(answers); i += 2) {
-    char *k = smartlist_get(answers, i);
-    char *v = smartlist_get(answers, i+1);
-    if (!strchr(v, '\n') && !strchr(v, '\r')) {
-      control_printf_midreply(conn, 250, "%s=%s", k, v);
-    } else {
-      control_printf_datareply(conn, 250, "%s=", k);
-      control_write_data(conn, v);
-    }
-  }
-  send_control_done(conn);
+  control_write_reply_lines(conn, answers);
 
  done:
-  SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp));
-  smartlist_free(answers);
-  SMARTLIST_FOREACH(unrecognized, char *, cp, tor_free(cp));
-  smartlist_free(unrecognized);
+  control_reply_free(answers);
+  control_reply_free(unrecognized);
 
   return 0;
 }
diff --git a/src/feature/control/control_proto.c b/src/feature/control/control_proto.c
index 5dec87491d..7d04fea6a7 100644
--- a/src/feature/control/control_proto.c
+++ b/src/feature/control/control_proto.c
@@ -22,6 +22,8 @@
 #include "core/or/origin_circuit_st.h"
 #include "core/or/socks_request_st.h"
 #include "feature/control/control_connection_st.h"
+#include "lib/container/smartlist.h"
+#include "lib/encoding/kvline.h"
 
 /** Append a NUL-terminated string <b>s</b> to the end of
  * <b>conn</b>-\>outbuf.
@@ -275,3 +277,158 @@ control_write_data(control_connection_t *conn, const char *data)
   connection_buf_add(esc, esc_len, TO_CONN(conn));
   tor_free(esc);
 }
+
+/** Write a single reply line to @a conn.
+ *
+ * @param conn control connection
+ * @param line control reply line to write
+ * @param lastone true if this is the last reply line of a multi-line reply
+ */
+void
+control_write_reply_line(control_connection_t *conn,
+                         const control_reply_line_t *line, bool lastone)
+{
+  const config_line_t *kvline = line->kvline;
+  char *s = NULL;
+
+  if (strpbrk(kvline->value, "\r\n") != NULL) {
+    /* If a key-value pair needs to be encoded as CmdData, it can be
+       the only key-value pair in that reply line */
+    tor_assert(kvline->next == NULL);
+    control_printf_datareply(conn, line->code, "%s=", kvline->key);
+    control_write_data(conn, kvline->value);
+    return;
+  }
+  s = kvline_encode(kvline, line->flags);
+  if (lastone) {
+    control_write_endreply(conn, line->code, s);
+  } else {
+    control_write_midreply(conn, line->code, s);
+  }
+  tor_free(s);
+}
+
+/** Write a set of reply lines to @a conn.
+ *
+ * @param conn control connection
+ * @param lines smartlist of pointers to control_reply_line_t to write
+ */
+void
+control_write_reply_lines(control_connection_t *conn, smartlist_t *lines)
+{
+  bool lastone = false;
+
+  SMARTLIST_FOREACH_BEGIN(lines, control_reply_line_t *, line) {
+    if (line_sl_idx >= line_sl_len - 1)
+      lastone = true;
+    control_write_reply_line(conn, line, lastone);
+  } SMARTLIST_FOREACH_END(line);
+}
+
+/** Add a single key-value pair as a new reply line to a control reply
+ * line list.
+ *
+ * @param reply smartlist of pointers to control_reply_line_t
+ * @param code numeric control reply code
+ * @param flags kvline encoding flags
+ * @param key key
+ * @param val value
+ */
+void
+control_reply_add_one_kv(smartlist_t *reply, int code, int flags,
+                         const char *key, const char *val)
+{
+  control_reply_line_t *line = tor_malloc_zero(sizeof(*line));
+
+  line->code = code;
+  line->flags = flags;
+  config_line_append(&line->kvline, key, val);
+  smartlist_add(reply, line);
+}
+
+/** Append a single key-value pair to last reply line in a control
+ * reply line list.
+ *
+ * @param reply smartlist of pointers to control_reply_line_t
+ * @param key key
+ * @param val value
+ */
+void
+control_reply_append_kv(smartlist_t *reply, const char *key, const char *val)
+{
+  int len = smartlist_len(reply);
+  control_reply_line_t *line;
+
+  tor_assert(len > 0);
+
+  line = smartlist_get(reply, len - 1);
+  config_line_append(&line->kvline, key, val);
+}
+
+/** Add new reply line consisting of the string @a s
+ *
+ * @param reply smartlist of pointers to control_reply_line_t
+ * @param code numeric control reply code
+ * @param s string containing the rest of the reply line
+ */
+void
+control_reply_add_str(smartlist_t *reply, int code, const char *s)
+{
+  control_reply_add_one_kv(reply, code, KV_OMIT_KEYS|KV_RAW, "", s);
+}
+
+/** Format a new reply line
+ *
+ * @param reply smartlist of pointers to control_reply_line_t
+ * @param code numeric control reply code
+ * @param fmt format string
+ */
+void
+control_reply_add_printf(smartlist_t *reply, int code, const char *fmt, ...)
+{
+  va_list ap;
+  char *buf = NULL;
+
+  va_start(ap, fmt);
+  (void)tor_vasprintf(&buf, fmt, ap);
+  va_end(ap);
+  control_reply_add_str(reply, code, buf);
+  tor_free(buf);
+}
+
+/** Add a "250 OK" line to a set of control reply lines */
+void
+control_reply_add_done(smartlist_t *reply)
+{
+  control_reply_add_str(reply, 250, "OK");
+}
+
+/** Free a control_reply_line_t.  Don't call this directly; use the
+ * control_reply_line_free() macro instead. */
+void
+control_reply_line_free_(control_reply_line_t *line)
+{
+  if (!line)
+    return;
+  config_free_lines(line->kvline);
+  tor_free_(line);
+}
+
+/** Clear a smartlist of control_reply_line_t.  Doesn't free the
+ * smartlist, but does free each individual line. */
+void
+control_reply_clear(smartlist_t *reply)
+{
+  SMARTLIST_FOREACH(reply, control_reply_line_t *, line,
+                    control_reply_line_free(line));
+  smartlist_clear(reply);
+}
+
+/** Free a smartlist of control_reply_line_t. Don't call this
+ * directly; use the control_reply_free() macro instead. */
+void
+control_reply_free_(smartlist_t *reply)
+{
+  control_reply_clear(reply);
+  smartlist_free_(reply);
+}
diff --git a/src/feature/control/control_proto.h b/src/feature/control/control_proto.h
index 3182f3d415..cf7c000439 100644
--- a/src/feature/control/control_proto.h
+++ b/src/feature/control/control_proto.h
@@ -7,11 +7,56 @@
 /**
  * \file control_proto.h
  * \brief Header file for control_proto.c.
+ *
+ * See @ref replylines for details about the key-value abstraction for
+ * generating reply lines.
  **/
 
 #ifndef TOR_CONTROL_PROTO_H
 #define TOR_CONTROL_PROTO_H
 
+#include "lib/encoding/confline.h"
+
+/**
+ * @defgroup replylines Control reply lines
+ * @brief Key-value structures for control reply lines
+ *
+ * Control reply lines are config_line_t key-value structures with
+ * some additional information to help formatting, such as the numeric
+ * result code specified in the control protocol and flags affecting
+ * the way kvline_encode() formats the @a kvline.
+ *
+ * Generally, modules implementing control commands will work with
+ * smartlists of these structures, using functions like
+ * control_reply_add_str() for adding a reply line consisting of a
+ * single string, or control_reply_add_one_kv() and
+ * control_reply_append_kv() for composing a line containing one or
+ * more key-value pairs.
+ *
+ * @{
+ */
+/** @brief A reply line for the control protocol.
+ *
+ * This wraps config_line_t with some additional information that's
+ * useful when generating control reply lines.
+ */
+typedef struct control_reply_line_t {
+  int code;                     /**< numeric code */
+  int flags;                    /**< kvline encoding flags */
+  config_line_t *kvline;        /**< kvline */
+} control_reply_line_t;
+
+void control_reply_line_free_(control_reply_line_t *line);
+/**
+ * @brief Free and null a control_reply_line_t
+ *
+ * @param line pointer to control_reply_line_t to free
+ */
+#define control_reply_line_free(line)                   \
+  FREE_AND_NULL(control_reply_line_t,                   \
+                control_reply_line_free_, (line))
+/** @} */
+
 void connection_write_str_to_buf(const char *s, control_connection_t *conn);
 void connection_printf_to_buf(control_connection_t *conn,
                                      const char *format, ...)
@@ -45,4 +90,31 @@ void control_printf_datareply(control_connection_t *conn, int code,
   CHECK_PRINTF(3, 4);
 void control_write_data(control_connection_t *conn, const char *data);
 
+/** @addtogroup replylines
+ * @{
+ */
+void control_write_reply_line(control_connection_t *conn,
+                              const control_reply_line_t *line, bool lastone);
+void control_write_reply_lines(control_connection_t *conn, smartlist_t *lines);
+
+void control_reply_add_one_kv(smartlist_t *reply, int code, int flags,
+                              const char *key, const char *val);
+void control_reply_append_kv(smartlist_t *reply, const char *key,
+                             const char *val);
+void control_reply_add_str(smartlist_t *reply, int code, const char *s);
+void control_reply_add_printf(smartlist_t *reply, int code,
+                              const char *fmt, ...)
+  CHECK_PRINTF(3, 4);
+void control_reply_add_done(smartlist_t *reply);
+
+void control_reply_clear(smartlist_t *reply);
+void control_reply_free_(smartlist_t *reply);
+
+/** @brief Free and null a smartlist of control_reply_line_t.
+ *
+ * @param r pointer to smartlist_t of control_reply_line_t to free */
+#define control_reply_free(r) \
+  FREE_AND_NULL(smartlist_t, control_reply_free_, (r))
+/** @} */
+
 #endif /* !defined(TOR_CONTROL_PROTO_H) */
diff --git a/src/lib/encoding/kvline.c b/src/lib/encoding/kvline.c
index d4a8f510ba..f55e3d966f 100644
--- a/src/lib/encoding/kvline.c
+++ b/src/lib/encoding/kvline.c
@@ -29,12 +29,20 @@
 #include <string.h>
 
 /** Return true iff we need to quote and escape the string <b>s</b> to encode
- * it. */
+ * it.
+ *
+ * kvline_can_encode_lines() also uses this (with
+ * <b>as_keyless_val</b> true) to check whether a key would require
+ * quoting.
+ */
 static bool
 needs_escape(const char *s, bool as_keyless_val)
 {
   if (as_keyless_val && *s == 0)
     return true;
+  /* Keyless values containing '=' need to be escaped. */
+  if (as_keyless_val && strchr(s, '='))
+    return true;
 
   for (; *s; ++s) {
     if (*s >= 127 || TOR_ISSPACE(*s) || ! TOR_ISPRINT(*s) ||
@@ -72,23 +80,17 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags)
 {
   for ( ; line; line = line->next) {
     const bool keyless = line_has_no_key(line);
-    if (keyless) {
-      if (! (flags & KV_OMIT_KEYS)) {
-        /* If KV_OMIT_KEYS is not set, we can't encode a line with no key. */
-        return false;
-      }
-      if (strchr(line->value, '=') && !( flags & KV_QUOTED)) {
-        /* We can't have a keyless value with = without quoting it. */
-        return false;
-      }
+    if (keyless && ! (flags & KV_OMIT_KEYS)) {
+      /* If KV_OMIT_KEYS is not set, we can't encode a line with no key. */
+      return false;
     }
 
-    if (needs_escape(line->value, keyless) && ! (flags & KV_QUOTED)) {
-      /* If KV_QUOTED is false, we can't encode a value that needs quotes. */
+    if (needs_escape(line->value, keyless) && ! (flags & (KV_QUOTED|KV_RAW))) {
+      /* If both KV_QUOTED and KV_RAW are false, we can't encode a
+         value that needs quotes. */
       return false;
     }
-    if (line->key && strlen(line->key) &&
-        (needs_escape(line->key, false) || strchr(line->key, '='))) {
+    if (!keyless && needs_escape(line->key, true)) {
       /* We can't handle keys that need quoting. */
       return false;
     }
@@ -103,7 +105,7 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags)
  *
  * If KV_QUOTED is set in <b>flags</b>, then all values that contain
  * spaces or unusual characters are escaped and quoted.  Otherwise, such
- * values are not allowed.
+ * values are not allowed.  Mutually exclusive with KV_RAW.
  *
  * 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
@@ -113,6 +115,11 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags)
  * encoded as 'Key', not as 'Key=' or 'Key=""'.  Mutually exclusive with
  * KV_OMIT_KEYS.
  *
+ * If KV_RAW is set in <b>flags</b>, then don't apply any quoting to
+ * the value, and assume that the caller has adequately quoted it.
+ * (The control protocol has some quirks that make this necessary.)
+ * Mutually exclusive with KV_QUOTED.
+ *
  * KV_QUOTED_QSTRING is not supported.
  */
 char *
@@ -121,11 +128,12 @@ kvline_encode(const config_line_t *line,
 {
   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));
+  tor_assert((flags & (KV_QUOTED|KV_RAW)) != (KV_QUOTED|KV_RAW));
+
+  if (!kvline_can_encode_lines(line, flags))
+    return NULL;
 
   smartlist_t *elements = smartlist_new();
 
@@ -142,15 +150,12 @@ kvline_encode(const config_line_t *line,
       k = line->key;
     } else {
       eq = "";
-      if (strchr(line->value, '=')) {
-        esc = true;
-      }
     }
 
     if ((flags & KV_OMIT_VALS) && line_has_no_val(line)) {
       eq = "";
       v = "";
-    } else if (esc) {
+    } else if (!(flags & KV_RAW) && esc) {
       tmp = esc_for_log(line->value);
       v = tmp;
     } else {
@@ -187,12 +192,15 @@ kvline_encode(const config_line_t *line,
  * 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.
+ *
+ * KV_RAW is not supported.
  */
 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));
+  tor_assert(!(flags & KV_RAW));
 
   const char *cp = line, *cplast = NULL;
   const bool omit_keys = (flags & KV_OMIT_KEYS) != 0;
diff --git a/src/lib/encoding/kvline.h b/src/lib/encoding/kvline.h
index dea2ce1809..9d36902ad1 100644
--- a/src/lib/encoding/kvline.h
+++ b/src/lib/encoding/kvline.h
@@ -19,6 +19,7 @@ struct config_line_t;
 #define KV_OMIT_KEYS (1u<<1)
 #define KV_OMIT_VALS (1u<<2)
 #define KV_QUOTED_QSTRING (1u<<3)
+#define KV_RAW       (1u<<4)
 
 struct config_line_t *kvline_parse(const char *line, unsigned flags);
 char *kvline_encode(const struct config_line_t *line, unsigned flags);
diff --git a/src/test/test_config.c b/src/test/test_config.c
index a75a862739..8f705da7e0 100644
--- a/src/test/test_config.c
+++ b/src/test/test_config.c
@@ -6050,6 +6050,36 @@ test_config_kvline_parse(void *arg)
   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");
+  tor_free(enc);
+  config_free_lines(lines);
+
+  lines = kvline_parse("AB=CD \"EF=GH\"", KV_OMIT_KEYS|KV_QUOTED);
+  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, "");
+  tt_str_op(lines->next->value, OP_EQ, "EF=GH");
+  enc = kvline_encode(lines, KV_OMIT_KEYS);
+  tt_assert(!enc);
+  enc = kvline_encode(lines, KV_OMIT_KEYS|KV_QUOTED);
+  tt_assert(enc);
+  tt_str_op(enc, OP_EQ, "AB=CD \"EF=GH\"");
+  tor_free(enc);
+  config_free_lines(lines);
+
+  lines = tor_malloc_zero(sizeof(*lines));
+  lines->key = tor_strdup("A=B");
+  lines->value = tor_strdup("CD");
+  enc = kvline_encode(lines, 0);
+  tt_assert(!enc);
+  config_free_lines(lines);
+
+  config_line_append(&lines, "A", "B C");
+  enc = kvline_encode(lines, 0);
+  tt_assert(!enc);
+  enc = kvline_encode(lines, KV_RAW);
+  tt_assert(enc);
+  tt_str_op(enc, OP_EQ, "A=B C");
 
  done:
   config_free_lines(lines);
diff --git a/src/test/test_controller.c b/src/test/test_controller.c
index d07ec5d0f0..b3023130ae 100644
--- a/src/test/test_controller.c
+++ b/src/test/test_controller.c
@@ -1957,6 +1957,76 @@ test_getinfo_md_all(void *arg)
   return;
 }
 
+static smartlist_t *reply_strs;
+
+static void
+mock_control_write_reply_list(control_connection_t *conn, int code, int c,
+                              const char *s)
+{
+  (void)conn;
+  /* To make matching easier, don't append "\r\n" */
+  smartlist_add_asprintf(reply_strs, "%03d%c%s", code, c, s);
+}
+
+static void
+test_control_reply(void *arg)
+{
+  (void)arg;
+  smartlist_t *lines = smartlist_new();
+
+  MOCK(control_write_reply, mock_control_write_reply);
+
+  tor_free(reply_str);
+  control_reply_clear(lines);
+  control_reply_add_str(lines, 250, "FOO");
+  control_write_reply_lines(NULL, lines);
+  tt_str_op(reply_str, OP_EQ, "FOO");
+
+  tor_free(reply_str);
+  control_reply_clear(lines);
+  control_reply_add_done(lines);
+  control_write_reply_lines(NULL, lines);
+  tt_str_op(reply_str, OP_EQ, "OK");
+
+  tor_free(reply_str);
+  control_reply_clear(lines);
+  UNMOCK(control_write_reply);
+  MOCK(control_write_reply, mock_control_write_reply_list);
+  reply_strs = smartlist_new();
+  control_reply_add_one_kv(lines, 250, 0, "A", "B");
+  control_reply_add_one_kv(lines, 250, 0, "C", "D");
+  control_write_reply_lines(NULL, lines);
+  tt_int_op(smartlist_len(reply_strs), OP_EQ, 2);
+  tt_str_op((char *)smartlist_get(reply_strs, 0), OP_EQ, "250-A=B");
+  tt_str_op((char *)smartlist_get(reply_strs, 1), OP_EQ, "250 C=D");
+
+  control_reply_clear(lines);
+  SMARTLIST_FOREACH(reply_strs, char *, p, tor_free(p));
+  smartlist_clear(reply_strs);
+  control_reply_add_printf(lines, 250, "PROTOCOLINFO %d", 1);
+  control_reply_add_one_kv(lines, 250, KV_OMIT_VALS|KV_RAW, "AUTH", "");
+  control_reply_append_kv(lines, "METHODS", "COOKIE");
+  control_reply_append_kv(lines, "COOKIEFILE", escaped("/tmp/cookie"));
+  control_reply_add_done(lines);
+  control_write_reply_lines(NULL, lines);
+  tt_int_op(smartlist_len(reply_strs), OP_EQ, 3);
+  tt_str_op((char *)smartlist_get(reply_strs, 0),
+            OP_EQ, "250-PROTOCOLINFO 1");
+  tt_str_op((char *)smartlist_get(reply_strs, 1),
+            OP_EQ, "250-AUTH METHODS=COOKIE COOKIEFILE=\"/tmp/cookie\"");
+  tt_str_op((char *)smartlist_get(reply_strs, 2),
+            OP_EQ, "250 OK");
+
+ done:
+  UNMOCK(control_write_reply);
+  tor_free(reply_str);
+  control_reply_free(lines);
+  if (reply_strs)
+    SMARTLIST_FOREACH(reply_strs, char *, p, tor_free(p));
+  smartlist_free(reply_strs);
+  return;
+}
+
 #ifndef COCCI
 #define PARSER_TEST(type)                                             \
   { "parse/" #type, test_controller_parse_cmd, 0, &passthrough_setup, \
@@ -1989,5 +2059,6 @@ struct testcase_t controller_tests[] = {
   { "download_status_bridge", test_download_status_bridge, 0, NULL, NULL },
   { "current_time", test_current_time, 0, NULL, NULL },
   { "getinfo_md_all", test_getinfo_md_all, 0, NULL, NULL },
+  { "control_reply", test_control_reply, 0, NULL, NULL },
   END_OF_TESTCASES
 };