Merge branch 'tor-github/pr/984'

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

changes/ticket30007

@@ -0,0 +1,3 @@
+  o Code simplification and refactoring:
+    - Abstract out the low-level formatting of replies on the control
+      port. Implements ticket 30007.

scripts/coccinelle/ctrl-reply-cleanup.cocci

@@ -0,0 +1,43 @@
+// Script to clean up after ctrl-reply.cocci -- run as a separate step
+// because cleanup_write2 (even when disabled) somehow prevents the
+// match rule in ctrl-reply.cocci from matching.
+
+// If it doesn't have to be a printf, turn it into a write
+
+@ cleanup_write @
+expression E;
+constant code, s;
+@@
+-control_printf_endreply(E, code, s)
++control_write_endreply(E, code, s)
+
+// Use send_control_done() instead of explicitly writing it out
+@ cleanup_send_done @
+type T;
+identifier f != send_control_done;
+expression E;
+@@
+ T f(...) {
+<...
+-control_write_endreply(E, 250, "OK")
++send_control_done(E)
+ ...>
+ }
+
+// Clean up more printfs that could be writes
+//
+// For some reason, including this rule, even disabled, causes the
+// match rule in ctrl-reply.cocci to fail to match some code that has
+// %s in its format strings
+
+@ cleanup_write2 @
+expression E1, E2;
+constant code;
+@@
+(
+-control_printf_endreply(E1, code, "%s", E2)
++control_write_endreply(E1, code, E2)
+|
+-control_printf_midreply(E1, code, "%s", E2)
++control_write_midreply(E1, code, E2)
+)

scripts/coccinelle/ctrl-reply.cocci

@@ -0,0 +1,87 @@
+// Script to edit control_*.c for refactored control reply output functions
+
+@ initialize:python @
+@@
+import re
+from coccilib.report import *
+
+# reply strings "NNN-foo", "NNN+foo", "NNN foo", etc.
+r = re.compile(r'^"(\d+)([ +-])(.*)\\r\\n"$')
+
+# Generate name of function to call based on which separator character
+# comes between the numeric code and the text
+def idname(sep, base):
+    if sep == '+':
+        return base + "datareply"
+    elif sep == '-':
+        return base + "midreply"
+    else:
+        return base + "endreply"
+
+# Generate the actual replacements used by the rules
+def gen(s, base, p):
+    pos = p[0]
+    print_report(pos, "%s %s" % (base, s))
+    m = r.match(s)
+    if m is None:
+        # String not correct format, so fail match
+        cocci.include_match(False)
+        print_report(pos, "BAD STRING %s" % s)
+        return
+
+    code, sep, s1 = m.groups()
+
+    if r'\r\n' in s1:
+        # Extra CRLF in string, so fail match
+        cocci.include_match(False)
+        print_report(pos, "extra CRLF in string %s" % s)
+        return
+
+    coccinelle.code = code
+    # Need a string that is a single C token, because Coccinelle only allows
+    # "identifiers" to be output from Python scripts?
+    coccinelle.body = '"%s"' % s1
+    coccinelle.id = idname(sep, base)
+    return
+
+@ match @
+identifier f;
+position p;
+expression E;
+constant s;
+@@
+(
+ connection_printf_to_buf@f@p(E, s, ...)
+|
+ connection_write_str_to_buf@f@p(s, E)
+)
+
+@ script:python sc1 @
+s << match.s;
+p << match.p;
+f << match.f;
+id;
+body;
+code;
+@@
+if f == 'connection_printf_to_buf':
+    gen(s, 'control_printf_', p)
+elif f == 'connection_write_str_to_buf':
+    gen(s, 'control_write_', p)
+else:
+    raise(ValueError("%s: %s" % (f, s)))
+
+@ replace @
+constant match.s;
+expression match.E;
+identifier match.f;
+identifier sc1.body, sc1.id, sc1.code;
+@@
+(
+-connection_write_str_to_buf@f(s, E)
++id(E, code, body)
+|
+-connection_printf_to_buf@f(E, s
++id(E, code, body
+ , ...)
+)

scripts/coccinelle/tor-coccinelle.h

@@ -0,0 +1,3 @@
+#define MOCK_IMPL(a, b, c) a b c
+#define CHECK_PRINTF(a, b)
+#define STATIC static

scripts/maint/practracker/exceptions.txt

@@ -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 53
+problem include-count /src/feature/control/control_getinfo.c 54
 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

@@ -81,6 +81,7 @@ LIBTOR_APP_A_SOURCES = 				\
 	src/feature/control/control_events.c	\
 	src/feature/control/control_fmt.c	\
 	src/feature/control/control_getinfo.c	\
+	src/feature/control/control_proto.c	\
 	src/feature/control/fmt_serverstatus.c  \
 	src/feature/control/getinfo_geoip.c	\
 	src/feature/dircache/conscache.c	\
@@ -307,6 +308,7 @@ noinst_HEADERS +=					\
 	src/feature/control/control_events.h	        \
 	src/feature/control/control_fmt.h		\
 	src/feature/control/control_getinfo.h		\
+	src/feature/control/control_proto.h		\
 	src/feature/control/fmt_serverstatus.h		\
 	src/feature/control/getinfo_geoip.h		\
 	src/feature/dirauth/authmode.h			\

src/feature/control/control.c

@@ -47,7 +47,7 @@
 #include "feature/control/control_auth.h"
 #include "feature/control/control_cmd.h"
 #include "feature/control/control_events.h"
-#include "feature/control/control_fmt.h"
+#include "feature/control/control_proto.h"
 #include "feature/rend/rendcommon.h"
 #include "feature/rend/rendservice.h"
 #include "lib/evloop/procmon.h"
@@ -401,7 +401,7 @@ connection_control_process_inbuf(control_connection_t *conn)
         return 0;
       else if (r == -1) {
         if (data_len + conn->incoming_cmd_cur_len > MAX_COMMAND_LINE_LENGTH) {
-          connection_write_str_to_buf("500 Line too long.\r\n", conn);
+          control_write_endreply(conn, 500, "Line too long.");
           connection_stop_reading(TO_CONN(conn));
           connection_mark_and_flush(TO_CONN(conn));
         }
@@ -455,20 +455,20 @@ connection_control_process_inbuf(control_connection_t *conn)
 
   /* Otherwise, Quit is always valid. */
   if (!strcasecmp(conn->current_cmd, "QUIT")) {
-    connection_write_str_to_buf("250 closing connection\r\n", conn);
+    control_write_endreply(conn, 250, "closing connection");
     connection_mark_and_flush(TO_CONN(conn));
     return 0;
   }
 
   if (conn->base_.state == CONTROL_CONN_STATE_NEEDAUTH &&
       !is_valid_initial_command(conn, conn->current_cmd)) {
-    connection_write_str_to_buf("514 Authentication required.\r\n", conn);
+    control_write_endreply(conn, 514, "Authentication required.");
     connection_mark_for_close(TO_CONN(conn));
     return 0;
   }
 
   if (data_len >= UINT32_MAX) {
-    connection_write_str_to_buf("500 A 4GB command? Nice try.\r\n", conn);
+    control_write_endreply(conn, 500, "A 4GB command? Nice try.");
     connection_mark_for_close(TO_CONN(conn));
     return 0;
   }

src/feature/control/control_auth.c

@@ -15,7 +15,7 @@
 #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 "feature/control/control_proto.h"
 #include "lib/crypt_ops/crypto_rand.h"
 #include "lib/crypt_ops/crypto_util.h"
 #include "lib/encoding/confline.h"
@@ -141,27 +141,28 @@ handle_control_authchallenge(control_connection_t *conn,
   char server_nonce_encoded[(2*SAFECOOKIE_SERVER_NONCE_LEN) + 1];
 
   if (strcasecmp(smartlist_get(args->args, 0), "SAFECOOKIE")) {
-    connection_write_str_to_buf("513 AUTHCHALLENGE only supports SAFECOOKIE "
+    control_write_endreply(conn, 513,
-                                "authentication\r\n", conn);
+                           "AUTHCHALLENGE only supports SAFECOOKIE "
+                           "authentication");
     goto fail;
   }
   if (!authentication_cookie_is_set) {
-    connection_write_str_to_buf("515 Cookie authentication is disabled\r\n",
+    control_write_endreply(conn, 515, "Cookie authentication is disabled");
-                                conn);
     goto fail;
   }
   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,
+    control_printf_endreply(conn, 512,
-                             "512 AUTHCHALLENGE dislikes argument list %s\r\n",
+                            "AUTHCHALLENGE dislikes argument list %s",
                             escaped(args->raw_body));
     goto fail;
   }
   if (strcmp(args->kwargs->key, "")) {
-    connection_write_str_to_buf("512 AUTHCHALLENGE does not accept keyword "
+    control_write_endreply(conn, 512,
-                                "arguments.\r\n", conn);
+                           "AUTHCHALLENGE does not accept keyword "
+                           "arguments.");
     goto fail;
   }
 
@@ -177,8 +178,7 @@ handle_control_authchallenge(control_connection_t *conn,
     client_nonce = tor_malloc(client_nonce_len);
     if (base16_decode(client_nonce, client_nonce_len, hex_nonce,
                       strlen(hex_nonce)) != (int)client_nonce_len) {
-      connection_write_str_to_buf("513 Invalid base16 client nonce\r\n",
+      control_write_endreply(conn, 513, "Invalid base16 client nonce");
-                                  conn);
       tor_free(client_nonce);
       goto fail;
     }
@@ -223,9 +223,8 @@ handle_control_authchallenge(control_connection_t *conn,
   base16_encode(server_nonce_encoded, sizeof(server_nonce_encoded),
                 server_nonce, sizeof(server_nonce));
 
-  connection_printf_to_buf(conn,
+  control_printf_endreply(conn, 250,
-                           "250 AUTHCHALLENGE SERVERHASH=%s "
+                          "AUTHCHALLENGE SERVERHASH=%s SERVERNONCE=%s",
-                           "SERVERNONCE=%s\r\n",
                           server_hash_encoded,
                           server_nonce_encoded);
 
@@ -263,13 +262,12 @@ handle_control_authenticate(control_connection_t *conn,
     password = tor_strdup("");
     password_len = 0;
   } else if (args->kwargs->next) {
-    connection_write_str_to_buf(
+    control_write_endreply(conn, 512, "Too many arguments to AUTHENTICATE.");
-             "512 Too many arguments to AUTHENTICATE.\r\n", conn);
     connection_mark_for_close(TO_CONN(conn));
     return 0;
   } else if (strcmp(args->kwargs->key, "")) {
-    connection_write_str_to_buf(
+    control_write_endreply(conn, 512,
-             "512 AUTHENTICATE does not accept keyword arguments.\r\n", conn);
+                           "AUTHENTICATE does not accept keyword arguments.");
     connection_mark_for_close(TO_CONN(conn));
     return 0;
   } else if (strchr(args->raw_body, '\"')) {
@@ -282,10 +280,10 @@ handle_control_authenticate(control_connection_t *conn,
     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(
+      control_write_endreply(conn, 551,
-            "551 Invalid hexadecimal encoding.  Maybe you tried a plain text "
+            "Invalid hexadecimal encoding.  Maybe you tried a plain text "
             "password?  If so, the standard requires that you put it in "
-            "double quotes.\r\n", conn);
+            "double quotes.");
       connection_mark_for_close(TO_CONN(conn));
       tor_free(password);
       return 0;
@@ -418,7 +416,7 @@ handle_control_authenticate(control_connection_t *conn,
 
  err:
   tor_free(password);
-  connection_printf_to_buf(conn, "515 Authentication failed: %s\r\n", errstr);
+  control_printf_endreply(conn, 515, "Authentication failed: %s", errstr);
   connection_mark_for_close(TO_CONN(conn));
   if (sl) { /* clean up */
     SMARTLIST_FOREACH(sl, char *, str, tor_free(str));

src/feature/control/control_cmd.c

@@ -27,8 +27,8 @@
 #include "feature/control/control_auth.h"
 #include "feature/control/control_cmd.h"
 #include "feature/control/control_events.h"
-#include "feature/control/control_fmt.h"
 #include "feature/control/control_getinfo.h"
+#include "feature/control/control_proto.h"
 #include "feature/hs/hs_control.h"
 #include "feature/nodelist/nodelist.h"
 #include "feature/nodelist/routerinfo.h"
@@ -319,11 +319,11 @@ handle_control_getconf(control_connection_t *conn,
 
   if ((len = smartlist_len(unrecognized))) {
     for (i=0; i < len-1; ++i)
-      connection_printf_to_buf(conn,
+      control_printf_midreply(conn, 552,
-                               "552-Unrecognized configuration key \"%s\"\r\n",
+                              "Unrecognized configuration key \"%s\"",
                               (char*)smartlist_get(unrecognized, i));
-    connection_printf_to_buf(conn,
+    control_printf_endreply(conn, 552,
-                             "552 Unrecognized configuration key \"%s\"\r\n",
+                            "Unrecognized configuration key \"%s\"",
                             (char*)smartlist_get(unrecognized, len-1));
   } else if ((len = smartlist_len(answers))) {
     char *tmp = smartlist_get(answers, len-1);
@@ -332,7 +332,7 @@ handle_control_getconf(control_connection_t *conn,
     msg = smartlist_join_strings(answers, "", 0, &msg_len);
     connection_buf_add(msg, msg_len, TO_CONN(conn));
   } else {
-    connection_write_str_to_buf("250 OK\r\n", conn);
+    send_control_done(conn);
   }
 
   SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp));
@@ -355,7 +355,6 @@ handle_control_loadconf(control_connection_t *conn,
 {
   setopt_err_t retval;
   char *errstring = NULL;
-  const char *msg = NULL;
 
   retval = options_init_from_string(NULL, args->cmddata,
                                     CMD_RUN_TOR, NULL, &errstring);
@@ -365,31 +364,29 @@ handle_control_loadconf(control_connection_t *conn,
              "Controller gave us config file that didn't validate: %s",
              errstring);
 
+#define SEND_ERRMSG(code, msg)                          \
+  control_printf_endreply(conn, code, msg "%s%s",       \
+                          errstring ? ": " : "",        \
+                          errstring ? errstring : "")
   switch (retval) {
   case SETOPT_ERR_PARSE:
-    msg = "552 Invalid config file";
+    SEND_ERRMSG(552, "Invalid config file");
     break;
   case SETOPT_ERR_TRANSITION:
-    msg = "553 Transition not allowed";
+    SEND_ERRMSG(553, "Transition not allowed");
     break;
   case SETOPT_ERR_SETTING:
-    msg = "553 Unable to set option";
+    SEND_ERRMSG(553, "Unable to set option");
     break;
   case SETOPT_ERR_MISC:
   default:
-    msg = "550 Unable to load config";
+    SEND_ERRMSG(550, "Unable to load config");
     break;
   case SETOPT_OK:
+    send_control_done(conn);
     break;
   }
-  if (msg) {
+#undef SEND_ERRMSG
-    if (errstring)
-      connection_printf_to_buf(conn, "%s: %s\r\n", msg, errstring);
-    else
-      connection_printf_to_buf(conn, "%s\r\n", msg);
-  } else {
-    send_control_done(conn);
-  }
   tor_free(errstring);
   return 0;
 }
@@ -427,8 +424,7 @@ handle_control_setevents(control_connection_t *conn,
         }
 
         if (event_code == -1) {
-          connection_printf_to_buf(conn, "552 Unrecognized event \"%s\"\r\n",
+          control_printf_endreply(conn, 552, "Unrecognized event \"%s\"", ev);
-                                   ev);
           return 0;
         }
       }
@@ -458,8 +454,8 @@ handle_control_saveconf(control_connection_t *conn,
   bool force = config_lines_contain_flag(args->kwargs, "FORCE");
   const or_options_t *options = get_options();
   if ((!force && options->IncludeUsed) || options_save_current() < 0) {
-    connection_write_str_to_buf(
+    control_write_endreply(conn, 551,
-      "551 Unable to write configuration to disk.\r\n", conn);
+                           "Unable to write configuration to disk.");
   } else {
     send_control_done(conn);
   }
@@ -492,8 +488,7 @@ handle_control_signal(control_connection_t *conn,
   }
 
   if (sig < 0)
-    connection_printf_to_buf(conn, "552 Unrecognized signal code \"%s\"\r\n",
+    control_printf_endreply(conn, 552, "Unrecognized signal code \"%s\"", s);
-                             s);
   if (sig < 0)
     return 0;
 
@@ -600,30 +595,32 @@ control_setconf_helper(control_connection_t *conn,
 
   opt_err = options_trial_assign(lines, flags, &errstring);
   {
-    const char *msg;
+#define SEND_ERRMSG(code, msg)                                  \
+    control_printf_endreply(conn, code, msg ": %s", errstring);
+
     switch (opt_err) {
       case SETOPT_ERR_MISC:
-        msg = "552 Unrecognized option";
+        SEND_ERRMSG(552, "Unrecognized option");
         break;
       case SETOPT_ERR_PARSE:
-        msg = "513 Unacceptable option value";
+        SEND_ERRMSG(513, "Unacceptable option value");
         break;
       case SETOPT_ERR_TRANSITION:
-        msg = "553 Transition not allowed";
+        SEND_ERRMSG(553, "Transition not allowed");
         break;
       case SETOPT_ERR_SETTING:
       default:
-        msg = "553 Unable to set option";
+        SEND_ERRMSG(553, "Unable to set option");
         break;
       case SETOPT_OK:
         config_free_lines(lines);
         send_control_done(conn);
         return 0;
     }
+#undef SEND_ERRMSG
     log_warn(LD_CONTROL,
              "Controller gave us config lines that didn't validate: %s",
              errstring);
-    connection_printf_to_buf(conn, "%s: %s\r\n", msg, errstring);
     config_free_lines(lines);
     tor_free(errstring);
     return 0;
@@ -777,7 +774,7 @@ handle_control_extendcircuit(control_connection_t *conn,
   if (purpose_line) {
     intended_purpose = circuit_purpose_from_string(purpose_line->value);
     if (intended_purpose == CIRCUIT_PURPOSE_UNKNOWN) {
-      connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n",
+      control_printf_endreply(conn, 552, "Unknown purpose \"%s\"",
                               purpose_line->value);
       goto done;
     }
@@ -788,9 +785,9 @@ handle_control_extendcircuit(control_connection_t *conn,
       // "EXTENDCIRCUIT 0" with no path.
       circ = circuit_launch(intended_purpose, CIRCLAUNCH_NEED_CAPACITY);
       if (!circ) {
-        connection_write_str_to_buf("551 Couldn't start circuit\r\n", conn);
+        control_write_endreply(conn, 551, "Couldn't start circuit");
       } else {
-        connection_printf_to_buf(conn, "250 EXTENDED %lu\r\n",
+        control_printf_endreply(conn, 250, "EXTENDED %lu",
                                 (unsigned long)circ->global_identifier);
       }
       goto done;
@@ -798,12 +795,12 @@ handle_control_extendcircuit(control_connection_t *conn,
   }
 
   if (!zero_circ && !(circ = get_circ(circ_id))) {
-    connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", circ_id);
+    control_printf_endreply(conn, 552, "Unknown circuit \"%s\"", circ_id);
     goto done;
   }
 
   if (!path_str) {
-    connection_printf_to_buf(conn, "512 syntax error: path required.\r\n");
+    control_write_endreply(conn, 512, "syntax error: path required.");
     goto done;
   }
 
@@ -814,11 +811,11 @@ handle_control_extendcircuit(control_connection_t *conn,
   SMARTLIST_FOREACH_BEGIN(router_nicknames, const char *, n) {
     const node_t *node = node_get_by_nickname(n, 0);
     if (!node) {
-      connection_printf_to_buf(conn, "552 No such router \"%s\"\r\n", n);
+      control_printf_endreply(conn, 552, "No such router \"%s\"", n);
       goto done;
     }
     if (!node_has_preferred_descriptor(node, first_node)) {
-      connection_printf_to_buf(conn, "552 No descriptor for \"%s\"\r\n", n);
+      control_printf_endreply(conn, 552, "No descriptor for \"%s\"", n);
       goto done;
     }
     smartlist_add(nodes, (void*)node);
@@ -826,7 +823,7 @@ handle_control_extendcircuit(control_connection_t *conn,
   } SMARTLIST_FOREACH_END(n);
 
   if (!smartlist_len(nodes)) {
-    connection_write_str_to_buf("512 No router names provided\r\n", conn);
+    control_write_endreply(conn, 512, "No router names provided");
     goto done;
   }
 
@@ -864,7 +861,7 @@ handle_control_extendcircuit(control_connection_t *conn,
     int err_reason = 0;
     if ((err_reason = circuit_handle_first_hop(circ)) < 0) {
       circuit_mark_for_close(TO_CIRCUIT(circ), -err_reason);
-      connection_write_str_to_buf("551 Couldn't start circuit\r\n", conn);
+      control_write_endreply(conn, 551, "Couldn't start circuit");
       goto done;
     }
   } else {
@@ -876,13 +873,13 @@ handle_control_extendcircuit(control_connection_t *conn,
         log_info(LD_CONTROL,
                  "send_next_onion_skin failed; circuit marked for closing.");
         circuit_mark_for_close(TO_CIRCUIT(circ), -err_reason);
-        connection_write_str_to_buf("551 Couldn't send onion skin\r\n", conn);
+        control_write_endreply(conn, 551, "Couldn't send onion skin");
         goto done;
       }
     }
   }
 
-  connection_printf_to_buf(conn, "250 EXTENDED %lu\r\n",
+  control_printf_endreply(conn, 250, "EXTENDED %lu",
                           (unsigned long)circ->global_identifier);
   if (zero_circ) /* send a 'launched' event, for completeness */
     circuit_event_status(circ, CIRC_EVENT_LAUNCHED, 0);
@@ -910,26 +907,26 @@ handle_control_setcircuitpurpose(control_connection_t *conn,
   const char *circ_id = smartlist_get(args->args,0);
 
   if (!(circ = get_circ(circ_id))) {
-    connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", circ_id);
+    control_printf_endreply(conn, 552, "Unknown circuit \"%s\"", circ_id);
     goto done;
   }
 
   {
     const config_line_t *purp = config_line_find_case(args->kwargs, "PURPOSE");
     if (!purp) {
-      connection_write_str_to_buf("552 No purpose given\r\n", conn);
+      control_write_endreply(conn, 552, "No purpose given");
       goto done;
     }
     new_purpose = circuit_purpose_from_string(purp->value);
     if (new_purpose == CIRCUIT_PURPOSE_UNKNOWN) {
-      connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n",
+      control_printf_endreply(conn, 552, "Unknown purpose \"%s\"",
                               purp->value);
       goto done;
     }
   }
 
   circuit_change_purpose(TO_CIRCUIT(circ), new_purpose);
-  connection_write_str_to_buf("250 OK\r\n", conn);
+  send_control_done(conn);
 
  done:
   return 0;
@@ -960,17 +957,17 @@ handle_control_attachstream(control_connection_t *conn,
   const config_line_t *hoparg = config_line_find_case(args->kwargs, "HOP");
 
   if (!(ap_conn = get_stream(stream_id))) {
-    connection_printf_to_buf(conn, "552 Unknown stream \"%s\"\r\n", stream_id);
+    control_printf_endreply(conn, 552, "Unknown stream \"%s\"", stream_id);
     return 0;
   } else if (!zero_circ && !(circ = get_circ(circ_id))) {
-    connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", circ_id);
+    control_printf_endreply(conn, 552, "Unknown circuit \"%s\"", circ_id);
     return 0;
   } else if (circ) {
     if (hoparg) {
       hop = (int) tor_parse_ulong(hoparg->value, 10, 0, INT_MAX,
                                   &hop_line_ok, NULL);
       if (!hop_line_ok) { /* broken hop line */
-        connection_printf_to_buf(conn, "552 Bad value hop=%s\r\n",
+        control_printf_endreply(conn, 552, "Bad value hop=%s",
                                 hoparg->value);
         return 0;
       }
@@ -980,9 +977,8 @@ handle_control_attachstream(control_connection_t *conn,
   if (ENTRY_TO_CONN(ap_conn)->state != AP_CONN_STATE_CONTROLLER_WAIT &&
       ENTRY_TO_CONN(ap_conn)->state != AP_CONN_STATE_CONNECT_WAIT &&
       ENTRY_TO_CONN(ap_conn)->state != AP_CONN_STATE_RESOLVE_WAIT) {
-    connection_write_str_to_buf(
+    control_write_endreply(conn, 555,
-                    "555 Connection is not managed by controller.\r\n",
+                           "Connection is not managed by controller.");
-                    conn);
     return 0;
   }
 
@@ -1001,15 +997,14 @@ handle_control_attachstream(control_connection_t *conn,
   }
 
   if (circ && (circ->base_.state != CIRCUIT_STATE_OPEN)) {
-    connection_write_str_to_buf(
+    control_write_endreply(conn, 551,
-                    "551 Can't attach stream to non-open origin circuit\r\n",
+                           "Can't attach stream to non-open origin circuit");
-                    conn);
     return 0;
   }
   /* Is this a single hop circuit? */
   if (circ && (circuit_get_cpath_len(circ)<2 || hop==1)) {
-    connection_write_str_to_buf(
+    control_write_endreply(conn, 551,
-               "551 Can't attach stream to this one-hop circuit.\r\n", conn);
+                           "Can't attach stream to this one-hop circuit.");
     return 0;
   }
 
@@ -1017,13 +1012,12 @@ handle_control_attachstream(control_connection_t *conn,
     /* find this hop in the circuit, and set cpath */
     cpath = circuit_get_cpath_hop(circ, hop);
     if (!cpath) {
-      connection_printf_to_buf(conn,
+      control_printf_endreply(conn, 551, "Circuit doesn't have %d hops.", hop);
-                               "551 Circuit doesn't have %d hops.\r\n", hop);
       return 0;
     }
   }
   if (connection_ap_handshake_rewrite_and_attach(ap_conn, circ, cpath) < 0) {
-    connection_write_str_to_buf("551 Unable to attach stream\r\n", conn);
+    control_write_endreply(conn, 551, "Unable to attach stream");
     return 0;
   }
   send_control_done(conn);
@@ -1055,7 +1049,7 @@ handle_control_postdescriptor(control_connection_t *conn,
   line = config_line_find_case(args->kwargs, "purpose");
   if (line) {
     purpose = router_purpose_from_string(line->value);
-    connection_printf_to_buf(conn, "552 Unknown purpose \"%s\"\r\n",
+    control_printf_endreply(conn, 552, "Unknown purpose \"%s\"",
                             line->value);
     goto done;
   }
@@ -1066,7 +1060,7 @@ handle_control_postdescriptor(control_connection_t *conn,
     else if (!strcasecmp(line->value, "yes"))
       cache = 1;
     else {
-      connection_printf_to_buf(conn, "552 Unknown cache request \"%s\"\r\n",
+      control_printf_endreply(conn, 552, "Unknown cache request \"%s\"",
                               line->value);
       goto done;
     }
@@ -1075,11 +1069,11 @@ handle_control_postdescriptor(control_connection_t *conn,
   switch (router_load_single_router(args->cmddata, purpose, cache, &msg)) {
   case -1:
     if (!msg) msg = "Could not parse descriptor";
-    connection_printf_to_buf(conn, "554 %s\r\n", msg);
+    control_write_endreply(conn, 554, msg);
     break;
   case 0:
     if (!msg) msg = "Descriptor not added";
-    connection_printf_to_buf(conn, "251 %s\r\n",msg);
+    control_write_endreply(conn, 251, msg);
     break;
   case 1:
     send_control_done(conn);
@@ -1108,7 +1102,7 @@ handle_control_redirectstream(control_connection_t *conn,
 
   if (!(ap_conn = get_stream(smartlist_get(args, 0)))
            || !ap_conn->socks_request) {
-    connection_printf_to_buf(conn, "552 Unknown stream \"%s\"\r\n",
+    control_printf_endreply(conn, 552, "Unknown stream \"%s\"",
                             (char*)smartlist_get(args, 0));
   } else {
     int ok = 1;
@@ -1117,7 +1111,7 @@ handle_control_redirectstream(control_connection_t *conn,
                                             10, 1, 65535, &ok, NULL);
     }
     if (!ok) {
-      connection_printf_to_buf(conn, "512 Cannot parse port \"%s\"\r\n",
+      control_printf_endreply(conn, 512, "Cannot parse port \"%s\"",
                               (char*)smartlist_get(args, 2));
     } else {
       new_addr = tor_strdup(smartlist_get(args, 1));
@@ -1156,13 +1150,13 @@ handle_control_closestream(control_connection_t *conn,
   tor_assert(smartlist_len(args) >= 2);
 
   if (!(ap_conn = get_stream(smartlist_get(args, 0))))
-    connection_printf_to_buf(conn, "552 Unknown stream \"%s\"\r\n",
+    control_printf_endreply(conn, 552, "Unknown stream \"%s\"",
                             (char*)smartlist_get(args, 0));
   else {
     reason = (uint8_t) tor_parse_ulong(smartlist_get(args,1), 10, 0, 255,
                                        &ok, NULL);
     if (!ok) {
-      connection_printf_to_buf(conn, "552 Unrecognized reason \"%s\"\r\n",
+      control_printf_endreply(conn, 552, "Unrecognized reason \"%s\"",
                               (char*)smartlist_get(args, 1));
       ap_conn = NULL;
     }
@@ -1193,7 +1187,7 @@ handle_control_closecircuit(control_connection_t *conn,
   origin_circuit_t *circ = NULL;
 
   if (!(circ=get_circ(circ_id))) {
-    connection_printf_to_buf(conn, "552 Unknown circuit \"%s\"\r\n", circ_id);
+    control_printf_endreply(conn, 552, "Unknown circuit \"%s\"", circ_id);
     return 0;
   }
 
@@ -1278,7 +1272,7 @@ handle_control_protocolinfo(control_connection_t *conn,
       }
     });
   if (bad_arg) {
-    connection_printf_to_buf(conn, "513 No such version %s\r\n",
+    control_printf_endreply(conn, 513, "No such version %s",
                             escaped(bad_arg));
     /* Don't tolerate bad arguments when not authenticated. */
     if (!STATE_IS_OPEN(TO_CONN(conn)->state))
@@ -1309,15 +1303,13 @@ handle_control_protocolinfo(control_connection_t *conn,
       smartlist_free(mlist);
     }
 
-    connection_printf_to_buf(conn,
+    control_write_midreply(conn, 250, "PROTOCOLINFO 1");
-                             "250-PROTOCOLINFO 1\r\n"
+    control_printf_midreply(conn, 250, "AUTH METHODS=%s%s%s", methods,
-                             "250-AUTH METHODS=%s%s%s\r\n"
-                             "250-VERSION Tor=%s\r\n"
-                             "250 OK\r\n",
-                             methods,
                             cookies?" COOKIEFILE=":"",
-                             cookies?esc_cfile:"",
+                            cookies?esc_cfile:"");
-                             escaped(VERSION));
+    control_printf_midreply(conn, 250, "VERSION Tor=%s", escaped(VERSION));
+    send_control_done(conn);
+
     tor_free(methods);
     tor_free(cfile);
     tor_free(abs_cfile);
@@ -1345,7 +1337,7 @@ handle_control_usefeature(control_connection_t *conn,
       else if (!strcasecmp(arg, "EXTENDED_EVENTS"))
         ;
       else {
-        connection_printf_to_buf(conn, "552 Unrecognized feature \"%s\"\r\n",
+        control_printf_endreply(conn, 552, "Unrecognized feature \"%s\"",
                                 arg);
         bad = 1;
         break;
@@ -1429,8 +1421,7 @@ handle_control_hsfetch(control_connection_t *conn,
     version = HS_VERSION_THREE;
     hs_parse_address(hsaddress, &v3_pk, NULL, NULL);
   } else {
-    connection_printf_to_buf(conn, "513 Invalid argument \"%s\"\r\n",
+    control_printf_endreply(conn, 513, "Invalid argument \"%s\"", arg1);
-                             arg1);
     goto done;
   }
 
@@ -1440,8 +1431,7 @@ handle_control_hsfetch(control_connection_t *conn,
 
       const node_t *node = node_get_by_hex_id(server, 0);
       if (!node) {
-        connection_printf_to_buf(conn, "552 Server \"%s\" not found\r\n",
+        control_printf_endreply(conn, 552, "Server \"%s\" not found", server);
-                                 server);
         goto done;
       }
       if (!hsdirs) {
@@ -1459,7 +1449,7 @@ handle_control_hsfetch(control_connection_t *conn,
     rend_query = rend_data_client_create(hsaddress, desc_id, NULL,
                                          REND_NO_AUTH);
     if (rend_query == NULL) {
-      connection_printf_to_buf(conn, "551 Error creating the HS query\r\n");
+      control_write_endreply(conn, 551, "Error creating the HS query");
       goto done;
     }
   }
@@ -1467,7 +1457,7 @@ handle_control_hsfetch(control_connection_t *conn,
   /* Using a descriptor ID, we force the user to provide at least one
    * hsdir server using the SERVER= option. */
   if (desc_id && (!hsdirs || !smartlist_len(hsdirs))) {
-    connection_printf_to_buf(conn, "512 SERVER option is required\r\n");
+    control_write_endreply(conn, 512, "SERVER option is required");
     goto done;
   }
 
@@ -1519,7 +1509,7 @@ handle_control_hspost(control_connection_t *conn,
       const node_t *node = node_get_by_hex_id(server, 0);
 
       if (!node || !node->rs) {
-        connection_printf_to_buf(conn, "552 Server \"%s\" not found\r\n",
+        control_printf_endreply(conn, 552, "Server \"%s\" not found",
                                 server);
         goto done;
       }
@@ -1530,7 +1520,7 @@ handle_control_hspost(control_connection_t *conn,
     } else if (!strcasecmpstart(line->key, "HSADDRESS")) {
       const char *address = line->value;
       if (!hs_address_is_valid(address)) {
-        connection_printf_to_buf(conn, "512 Malformed onion address\r\n");
+        control_write_endreply(conn, 512, "Malformed onion address");
         goto done;
       }
       onion_address = address;
@@ -1542,7 +1532,7 @@ handle_control_hspost(control_connection_t *conn,
   /* Handle the v3 case. */
   if (onion_address) {
     if (hs_control_hspost_command(encoded_desc, onion_address, hs_dirs) < 0) {
-      connection_printf_to_buf(conn, "554 Invalid descriptor\r\n");
+      control_write_endreply(conn, 554, "Invalid descriptor");
     } else {
       send_control_done(conn);
     }
@@ -1582,7 +1572,7 @@ handle_control_hspost(control_connection_t *conn,
 
     rend_service_descriptor_free(parsed);
   } else {
-    connection_printf_to_buf(conn, "554 Invalid descriptor\r\n");
+    control_write_endreply(conn, 554, "Invalid descriptor");
   }
 
   tor_free(intro_content);
@@ -1688,7 +1678,7 @@ handle_control_add_onion(control_connection_t *conn,
       rend_service_port_config_t *cfg =
           rend_service_parse_port_config(arg->value, ",", NULL);
       if (!cfg) {
-        connection_printf_to_buf(conn, "512 Invalid VIRTPORT/TARGET\r\n");
+        control_write_endreply(conn, 512, "Invalid VIRTPORT/TARGET");
         goto out;
       }
       smartlist_add(port_cfgs, cfg);
@@ -1697,7 +1687,7 @@ handle_control_add_onion(control_connection_t *conn,
       int ok = 0;
       max_streams = (int)tor_parse_long(arg->value, 10, 0, 65535, &ok, NULL);
       if (!ok) {
-        connection_printf_to_buf(conn, "512 Invalid MaxStreams\r\n");
+        control_write_endreply(conn, 512, "Invalid MaxStreams");
         goto out;
       }
     } else if (!strcasecmp(arg->key, "Flags")) {
@@ -1725,7 +1715,7 @@ handle_control_add_onion(control_connection_t *conn,
 
       smartlist_split_string(flags, arg->value, ",", SPLIT_IGNORE_BLANK, 0);
       if (smartlist_len(flags) < 1) {
-        connection_printf_to_buf(conn, "512 Invalid 'Flags' argument\r\n");
+        control_write_endreply(conn, 512, "Invalid 'Flags' argument");
         bad = 1;
       }
       SMARTLIST_FOREACH_BEGIN(flags, const char *, flag)
@@ -1741,8 +1731,7 @@ handle_control_add_onion(control_connection_t *conn,
         } else if (!strcasecmp(flag, non_anonymous_flag)) {
           non_anonymous = 1;
         } else {
-          connection_printf_to_buf(conn,
+          control_printf_endreply(conn, 512, "Invalid 'Flags' argument: %s",
-                                   "512 Invalid 'Flags' argument: %s\r\n",
                                   escaped(flag));
           bad = 1;
           break;
@@ -1776,8 +1765,7 @@ handle_control_add_onion(control_connection_t *conn,
           }
         } SMARTLIST_FOREACH_END(ac);
         if (bad) {
-          connection_printf_to_buf(conn,
+          control_write_endreply(conn, 512, "Duplicate name in ClientAuth");
-                                   "512 Duplicate name in ClientAuth\r\n");
           rend_authorized_client_free(client);
           goto out;
         }
@@ -1795,19 +1783,19 @@ handle_control_add_onion(control_connection_t *conn,
     }
   }
   if (smartlist_len(port_cfgs) == 0) {
-    connection_printf_to_buf(conn, "512 Missing 'Port' argument\r\n");
+    control_write_endreply(conn, 512, "Missing 'Port' argument");
     goto out;
   } else if (auth_type == REND_NO_AUTH && auth_clients != NULL) {
-    connection_printf_to_buf(conn, "512 No auth type specified\r\n");
+    control_write_endreply(conn, 512, "No auth type specified");
     goto out;
   } else if (auth_type != REND_NO_AUTH && auth_clients == NULL) {
-    connection_printf_to_buf(conn, "512 No auth clients specified\r\n");
+    control_write_endreply(conn, 512, "No auth clients specified");
     goto out;
   } else if ((auth_type == REND_BASIC_AUTH &&
               smartlist_len(auth_clients) > 512) ||
              (auth_type == REND_STEALTH_AUTH &&
               smartlist_len(auth_clients) > 16)) {
-    connection_printf_to_buf(conn, "512 Too many auth clients\r\n");
+    control_write_endreply(conn, 512, "Too many auth clients");
     goto out;
   } else if (non_anonymous != rend_service_non_anonymous_mode_enabled(
                                                               get_options())) {
@@ -1818,8 +1806,8 @@ handle_control_add_onion(control_connection_t *conn,
      * 512 Tor is in non-anonymous hidden service mode
      * (I've deliberately written them out in full here to aid searchability.)
      */
-    connection_printf_to_buf(conn, "512 Tor is in %sanonymous hidden service "
+    control_printf_endreply(conn, 512,
-                             "mode\r\n",
+                            "Tor is in %sanonymous hidden service " "mode",
                             non_anonymous ? "" : "non-");
     goto out;
   }
@@ -1846,7 +1834,7 @@ handle_control_add_onion(control_connection_t *conn,
   /* Hidden service version 3 don't have client authentication support so if
    * ClientAuth was given, send back an error. */
   if (hs_version == HS_VERSION_THREE && auth_clients) {
-    connection_printf_to_buf(conn, "513 ClientAuth not supported\r\n");
+    control_write_endreply(conn, 513, "ClientAuth not supported");
     goto out;
   }
 
@@ -1876,10 +1864,10 @@ handle_control_add_onion(control_connection_t *conn,
     }
 
     tor_assert(service_id);
-    connection_printf_to_buf(conn, "250-ServiceID=%s\r\n", service_id);
+    control_printf_midreply(conn, 250, "ServiceID=%s", service_id);
     if (key_new_alg) {
       tor_assert(key_new_blob);
-      connection_printf_to_buf(conn, "250-PrivateKey=%s:%s\r\n",
+      control_printf_midreply(conn, 250, "PrivateKey=%s:%s",
                               key_new_alg, key_new_blob);
     }
     if (auth_created_clients) {
@@ -1894,24 +1882,24 @@ handle_control_add_onion(control_connection_t *conn,
       });
     }
 
-    connection_printf_to_buf(conn, "250 OK\r\n");
+    send_control_done(conn);
     break;
   }
   case RSAE_BADPRIVKEY:
-    connection_printf_to_buf(conn, "551 Failed to generate onion address\r\n");
+    control_write_endreply(conn, 551, "Failed to generate onion address");
     break;
   case RSAE_ADDREXISTS:
-    connection_printf_to_buf(conn, "550 Onion address collision\r\n");
+    control_write_endreply(conn, 550, "Onion address collision");
     break;
   case RSAE_BADVIRTPORT:
-    connection_printf_to_buf(conn, "512 Invalid VIRTPORT/TARGET\r\n");
+    control_write_endreply(conn, 512, "Invalid VIRTPORT/TARGET");
     break;
   case RSAE_BADAUTH:
-    connection_printf_to_buf(conn, "512 Invalid client authorization\r\n");
+    control_write_endreply(conn, 512, "Invalid client authorization");
     break;
   case RSAE_INTERNAL: /* FALLSTHROUGH */
   default:
-    connection_printf_to_buf(conn, "551 Failed to add Onion Service\r\n");
+    control_write_endreply(conn, 551, "Failed to add Onion Service");
   }
   if (key_new_blob) {
     memwipe(key_new_blob, 0, strlen(key_new_blob));
@@ -2153,7 +2141,7 @@ handle_control_del_onion(control_connection_t *conn,
   } else if (hs_address_is_valid(service_id)) {
     hs_version = HS_VERSION_THREE;
   } else {
-    connection_printf_to_buf(conn, "512 Malformed Onion Service id\r\n");
+    control_write_endreply(conn, 512, "Malformed Onion Service id");
     goto out;
   }
 
@@ -2177,7 +2165,7 @@ handle_control_del_onion(control_connection_t *conn,
     }
   }
   if (onion_services == NULL) {
-    connection_printf_to_buf(conn, "552 Unknown Onion Service id\r\n");
+    control_write_endreply(conn, 552, "Unknown Onion Service id");
   } else {
     int ret = -1;
     switch (hs_version) {
@@ -2229,7 +2217,7 @@ handle_control_obsolete(control_connection_t *conn,
   (void)args;
   char *command = tor_strdup(conn->current_cmd);
   tor_strupper(command);
-  connection_printf_to_buf(conn, "511 %s is obsolete.\r\n", command);
+  control_printf_endreply(conn, 511, "%s is obsolete.", command);
   tor_free(command);
   return 0;
 }
@@ -2362,8 +2350,7 @@ handle_single_control_command(const control_cmd_def_t *def,
                                        cmd_data_len, args,
                                        &err);
   if (!parsed_args) {
-    connection_printf_to_buf(conn,
+    control_printf_endreply(conn, 512, "Bad arguments to %s: %s",
-                             "512 Bad arguments to %s: %s\r\n",
                             conn->current_cmd, err?err:"");
     tor_free(err);
   } else {
@@ -2404,7 +2391,7 @@ handle_control_command(control_connection_t *conn,
     }
   }
 
-  connection_printf_to_buf(conn, "510 Unrecognized command \"%s\"\r\n",
+  control_printf_endreply(conn, 510, "Unrecognized command \"%s\"",
                           conn->current_cmd);
 
   return 0;

src/feature/control/control_events.c

@@ -24,6 +24,7 @@
 #include "feature/control/control.h"
 #include "feature/control/control_events.h"
 #include "feature/control/control_fmt.h"
+#include "feature/control/control_proto.h"
 #include "feature/dircommon/directory.h"
 #include "feature/nodelist/networkstatus.h"
 #include "feature/nodelist/nodelist.h"

src/feature/control/control_fmt.c

@@ -3,7 +3,7 @@
 /* See LICENSE for licensing information */
 
 /**
- * \file control.c
+ * \file control_fmt.c
  * \brief Formatting functions for controller data.
  */
 
@@ -14,6 +14,7 @@
 #include "core/or/circuitlist.h"
 #include "core/or/connection_edge.h"
 #include "feature/control/control_fmt.h"
+#include "feature/control/control_proto.h"
 #include "feature/nodelist/nodelist.h"
 
 #include "core/or/cpath_build_state_st.h"
@@ -23,39 +24,6 @@
 #include "core/or/socks_request_st.h"
 #include "feature/control/control_connection_st.h"
 
-/** Append a NUL-terminated string <b>s</b> to the end of
- * <b>conn</b>-\>outbuf.
- */
-void
-connection_write_str_to_buf(const char *s, control_connection_t *conn)
-{
-  size_t len = strlen(s);
-  connection_buf_add(s, len, TO_CONN(conn));
-}
-
-/** Acts like sprintf, but writes its formatted string to the end of
- * <b>conn</b>-\>outbuf. */
-void
-connection_printf_to_buf(control_connection_t *conn, const char *format, ...)
-{
-  va_list ap;
-  char *buf = NULL;
-  int len;
-
-  va_start(ap,format);
-  len = tor_vasprintf(&buf, format, ap);
-  va_end(ap);
-
-  if (len < 0) {
-    log_err(LD_BUG, "Unable to format string for controller.");
-    tor_assert(0);
-  }
-
-  connection_buf_add(buf, (size_t)len, TO_CONN(conn));
-
-  tor_free(buf);
-}
-
 /** Given an AP connection <b>conn</b> and a <b>len</b>-character buffer
  * <b>buf</b>, determine the address:port combination requested on
  * <b>conn</b>, and write it to <b>buf</b>.  Return 0 on success, -1 on
@@ -197,114 +165,6 @@ circuit_describe_status_for_controller(origin_circuit_t *circ)
   return rv;
 }
 
-/** Given a <b>len</b>-character string in <b>data</b>, made of lines
- * terminated by CRLF, allocate a new string in *<b>out</b>, and copy the
- * contents of <b>data</b> into *<b>out</b>, adding a period before any period
- * that appears at the start of a line, and adding a period-CRLF line at
- * the end. Replace all LF characters sequences with CRLF.  Return the number
- * of bytes in *<b>out</b>.
- */
-size_t
-write_escaped_data(const char *data, size_t len, char **out)
-{
-  tor_assert(len < SIZE_MAX - 9);
-  size_t sz_out = len+8+1;
-  char *outp;
-  const char *start = data, *end;
-  size_t i;
-  int start_of_line;
-  for (i=0; i < len; ++i) {
-    if (data[i] == '\n') {
-      sz_out += 2; /* Maybe add a CR; maybe add a dot. */
-      if (sz_out >= SIZE_T_CEILING) {
-        log_warn(LD_BUG, "Input to write_escaped_data was too long");
-        *out = tor_strdup(".\r\n");
-        return 3;
-      }
-    }
-  }
-  *out = outp = tor_malloc(sz_out);
-  end = data+len;
-  start_of_line = 1;
-  while (data < end) {
-    if (*data == '\n') {
-      if (data > start && data[-1] != '\r')
-        *outp++ = '\r';
-      start_of_line = 1;
-    } else if (*data == '.') {
-      if (start_of_line) {
-        start_of_line = 0;
-        *outp++ = '.';
-      }
-    } else {
-      start_of_line = 0;
-    }
-    *outp++ = *data++;
-  }
-  if (outp < *out+2 || fast_memcmp(outp-2, "\r\n", 2)) {
-    *outp++ = '\r';
-    *outp++ = '\n';
-  }
-  *outp++ = '.';
-  *outp++ = '\r';
-  *outp++ = '\n';
-  *outp = '\0'; /* NUL-terminate just in case. */
-  tor_assert(outp >= *out);
-  tor_assert((size_t)(outp - *out) <= sz_out);
-  return outp - *out;
-}
-
-/** Given a <b>len</b>-character string in <b>data</b>, made of lines
- * terminated by CRLF, allocate a new string in *<b>out</b>, and copy
- * the contents of <b>data</b> into *<b>out</b>, removing any period
- * that appears at the start of a line, and replacing all CRLF sequences
- * with LF.   Return the number of
- * bytes in *<b>out</b>. */
-size_t
-read_escaped_data(const char *data, size_t len, char **out)
-{
-  char *outp;
-  const char *next;
-  const char *end;
-
-  *out = outp = tor_malloc(len+1);
-
-  end = data+len;
-
-  while (data < end) {
-    /* we're at the start of a line. */
-    if (*data == '.')
-      ++data;
-    next = memchr(data, '\n', end-data);
-    if (next) {
-      size_t n_to_copy = next-data;
-      /* Don't copy a CR that precedes this LF. */
-      if (n_to_copy && *(next-1) == '\r')
-        --n_to_copy;
-      memcpy(outp, data, n_to_copy);
-      outp += n_to_copy;
-      data = next+1; /* This will point at the start of the next line,
-                      * or the end of the string, or a period. */
-    } else {
-      memcpy(outp, data, end-data);
-      outp += (end-data);
-      *outp = '\0';
-      return outp - *out;
-    }
-    *outp++ = '\n';
-  }
-
-  *outp = '\0';
-  return outp - *out;
-}
-
-/** Send a "DONE" message down the control connection <b>conn</b>. */
-void
-send_control_done(control_connection_t *conn)
-{
-  connection_write_str_to_buf("250 OK\r\n", conn);
-}
-
 /** 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

@@ -12,21 +12,12 @@
 #ifndef TOR_CONTROL_FMT_H
 #define TOR_CONTROL_FMT_H
 
-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, ...)
-  CHECK_PRINTF(2,3);
-
 int write_stream_target_to_buf(entry_connection_t *conn, char *buf,
                                size_t len);
 void orconn_target_get_name(char *buf, size_t len,
                             or_connection_t *conn);
 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);
-void send_control_done(control_connection_t *conn);
-
 MOCK_DECL(const char *, node_describe_longname_by_id,(const char *id_digest));
 
 #endif /* !defined(TOR_CONTROL_FMT_H) */

src/feature/control/control_getinfo.c

@@ -28,6 +28,7 @@
 #include "feature/control/control_events.h"
 #include "feature/control/control_fmt.h"
 #include "feature/control/control_getinfo.h"
+#include "feature/control/control_proto.h"
 #include "feature/control/fmt_serverstatus.h"
 #include "feature/control/getinfo_geoip.h"
 #include "feature/dircache/dirserv.h"
@@ -1607,7 +1608,7 @@ handle_control_getinfo(control_connection_t *conn,
     if (handle_getinfo_helper(conn, q, &ans, &errmsg) < 0) {
       if (!errmsg)
         errmsg = "Internal error";
-      connection_printf_to_buf(conn, "551 %s\r\n", errmsg);
+      control_write_endreply(conn, 551, errmsg);
       goto done;
     }
     if (!ans) {
@@ -1624,13 +1625,10 @@ handle_control_getinfo(control_connection_t *conn,
   if (smartlist_len(unrecognized)) {
     /* control-spec section 2.3, mid-reply '-' or end of reply ' ' */
     for (i=0; i < smartlist_len(unrecognized)-1; ++i)
-      connection_printf_to_buf(conn,
+      control_write_midreply(conn, 552,
-                               "552-%s\r\n",
                              (char *)smartlist_get(unrecognized, i));
 
-    connection_printf_to_buf(conn,
+    control_write_endreply(conn, 552, (char *)smartlist_get(unrecognized, i));
-                             "552 %s\r\n",
-                             (char *)smartlist_get(unrecognized, i));
     goto done;
   }
 
@@ -1638,19 +1636,13 @@ handle_control_getinfo(control_connection_t *conn,
     char *k = smartlist_get(answers, i);
     char *v = smartlist_get(answers, i+1);
     if (!strchr(v, '\n') && !strchr(v, '\r')) {
-      connection_printf_to_buf(conn, "250-%s=", k);
+      control_printf_midreply(conn, 250, "%s=%s", k, v);
-      connection_write_str_to_buf(v, conn);
-      connection_write_str_to_buf("\r\n", conn);
     } else {
-      char *esc = NULL;
+      control_printf_datareply(conn, 250, "%s=", k);
-      size_t esc_len;
+      control_write_data(conn, v);
-      esc_len = write_escaped_data(v, strlen(v), &esc);
-      connection_printf_to_buf(conn, "250+%s=\r\n", k);
-      connection_buf_add(esc, esc_len, TO_CONN(conn));
-      tor_free(esc);
     }
   }
-  connection_write_str_to_buf("250 OK\r\n", conn);
+  send_control_done(conn);
 
  done:
   SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp));

src/feature/control/control_proto.c

@@ -0,0 +1,276 @@
+/* Copyright (c) 2004-2006, Roger Dingledine, Nick Mathewson.
+ * Copyright (c) 2007-2019, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+/**
+ * \file control_proto.c
+ * \brief Formatting functions for controller data.
+ */
+
+#include "core/or/or.h"
+
+#include "core/mainloop/connection.h"
+#include "core/or/circuitbuild.h"
+#include "core/or/circuitlist.h"
+#include "core/or/connection_edge.h"
+#include "feature/control/control_proto.h"
+#include "feature/nodelist/nodelist.h"
+
+#include "core/or/cpath_build_state_st.h"
+#include "core/or/entry_connection_st.h"
+#include "core/or/or_connection_st.h"
+#include "core/or/origin_circuit_st.h"
+#include "core/or/socks_request_st.h"
+#include "feature/control/control_connection_st.h"
+
+/** Append a NUL-terminated string <b>s</b> to the end of
+ * <b>conn</b>-\>outbuf.
+ */
+void
+connection_write_str_to_buf(const char *s, control_connection_t *conn)
+{
+  size_t len = strlen(s);
+  connection_buf_add(s, len, TO_CONN(conn));
+}
+
+/** Acts like sprintf, but writes its formatted string to the end of
+ * <b>conn</b>-\>outbuf. */
+void
+connection_printf_to_buf(control_connection_t *conn, const char *format, ...)
+{
+  va_list ap;
+  char *buf = NULL;
+  int len;
+
+  va_start(ap,format);
+  len = tor_vasprintf(&buf, format, ap);
+  va_end(ap);
+
+  if (len < 0) {
+    log_err(LD_BUG, "Unable to format string for controller.");
+    tor_assert(0);
+  }
+
+  connection_buf_add(buf, (size_t)len, TO_CONN(conn));
+
+  tor_free(buf);
+}
+
+/** Given a <b>len</b>-character string in <b>data</b>, made of lines
+ * terminated by CRLF, allocate a new string in *<b>out</b>, and copy the
+ * contents of <b>data</b> into *<b>out</b>, adding a period before any period
+ * that appears at the start of a line, and adding a period-CRLF line at
+ * the end. Replace all LF characters sequences with CRLF.  Return the number
+ * of bytes in *<b>out</b>.
+ *
+ * This corresponds to CmdData in control-spec.txt.
+ */
+size_t
+write_escaped_data(const char *data, size_t len, char **out)
+{
+  tor_assert(len < SIZE_MAX - 9);
+  size_t sz_out = len+8+1;
+  char *outp;
+  const char *start = data, *end;
+  size_t i;
+  int start_of_line;
+  for (i=0; i < len; ++i) {
+    if (data[i] == '\n') {
+      sz_out += 2; /* Maybe add a CR; maybe add a dot. */
+      if (sz_out >= SIZE_T_CEILING) {
+        log_warn(LD_BUG, "Input to write_escaped_data was too long");
+        *out = tor_strdup(".\r\n");
+        return 3;
+      }
+    }
+  }
+  *out = outp = tor_malloc(sz_out);
+  end = data+len;
+  start_of_line = 1;
+  while (data < end) {
+    if (*data == '\n') {
+      if (data > start && data[-1] != '\r')
+        *outp++ = '\r';
+      start_of_line = 1;
+    } else if (*data == '.') {
+      if (start_of_line) {
+        start_of_line = 0;
+        *outp++ = '.';
+      }
+    } else {
+      start_of_line = 0;
+    }
+    *outp++ = *data++;
+  }
+  if (outp < *out+2 || fast_memcmp(outp-2, "\r\n", 2)) {
+    *outp++ = '\r';
+    *outp++ = '\n';
+  }
+  *outp++ = '.';
+  *outp++ = '\r';
+  *outp++ = '\n';
+  *outp = '\0'; /* NUL-terminate just in case. */
+  tor_assert(outp >= *out);
+  tor_assert((size_t)(outp - *out) <= sz_out);
+  return outp - *out;
+}
+
+/** Given a <b>len</b>-character string in <b>data</b>, made of lines
+ * terminated by CRLF, allocate a new string in *<b>out</b>, and copy
+ * the contents of <b>data</b> into *<b>out</b>, removing any period
+ * that appears at the start of a line, and replacing all CRLF sequences
+ * with LF.   Return the number of
+ * bytes in *<b>out</b>.
+ *
+ * This corresponds to CmdData in control-spec.txt.
+ */
+size_t
+read_escaped_data(const char *data, size_t len, char **out)
+{
+  char *outp;
+  const char *next;
+  const char *end;
+
+  *out = outp = tor_malloc(len+1);
+
+  end = data+len;
+
+  while (data < end) {
+    /* we're at the start of a line. */
+    if (*data == '.')
+      ++data;
+    next = memchr(data, '\n', end-data);
+    if (next) {
+      size_t n_to_copy = next-data;
+      /* Don't copy a CR that precedes this LF. */
+      if (n_to_copy && *(next-1) == '\r')
+        --n_to_copy;
+      memcpy(outp, data, n_to_copy);
+      outp += n_to_copy;
+      data = next+1; /* This will point at the start of the next line,
+                      * or the end of the string, or a period. */
+    } else {
+      memcpy(outp, data, end-data);
+      outp += (end-data);
+      *outp = '\0';
+      return outp - *out;
+    }
+    *outp++ = '\n';
+  }
+
+  *outp = '\0';
+  return outp - *out;
+}
+
+/** Send a "DONE" message down the control connection <b>conn</b>. */
+void
+send_control_done(control_connection_t *conn)
+{
+  control_write_endreply(conn, 250, "OK");
+}
+
+/** Write a reply to the control channel.
+ *
+ * @param conn control connection
+ * @param code numeric result code
+ * @param c separator character, usually ' ', '-', or '+'
+ * @param s string
+ */
+void
+control_write_reply(control_connection_t *conn, int code, int c, const char *s)
+{
+  connection_printf_to_buf(conn, "%03d%c%s\r\n", code, c, s);
+}
+
+/** Write a formatted reply to the control channel.
+ *
+ * @param conn control connection
+ * @param code numeric result code
+ * @param c separator character, usually ' ', '-', or '+'
+ * @param fmt format string
+ * @param ap va_list from caller
+ */
+void
+control_vprintf_reply(control_connection_t *conn, int code, int c,
+                      const char *fmt, va_list ap)
+{
+  char *buf = NULL;
+  int len;
+
+  len = tor_vasprintf(&buf, fmt, ap);
+  if (len < 0) {
+    log_err(LD_BUG, "Unable to format string for controller.");
+    tor_assert(0);
+  }
+  control_write_reply(conn, code, c, buf);
+  tor_free(buf);
+}
+
+/** Write an EndReplyLine */
+void
+control_write_endreply(control_connection_t *conn, int code, const char *s)
+{
+  control_write_reply(conn, code, ' ', s);
+}
+
+/** Write a formatted EndReplyLine */
+void
+control_printf_endreply(control_connection_t *conn, int code,
+                        const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start(ap, fmt);
+  control_vprintf_reply(conn, code, ' ', fmt, ap);
+  va_end(ap);
+}
+
+/** Write a MidReplyLine */
+void
+control_write_midreply(control_connection_t *conn, int code, const char *s)
+{
+  control_write_reply(conn, code, '-', s);
+}
+
+/** Write a formatted MidReplyLine */
+void
+control_printf_midreply(control_connection_t *conn, int code, const char *fmt,
+                        ...)
+{
+  va_list ap;
+
+  va_start(ap, fmt);
+  control_vprintf_reply(conn, code, '-', fmt, ap);
+  va_end(ap);
+}
+
+/** Write a DataReplyLine */
+void
+control_write_datareply(control_connection_t *conn, int code, const char *s)
+{
+  control_write_reply(conn, code, '+', s);
+}
+
+/** Write a formatted DataReplyLine */
+void
+control_printf_datareply(control_connection_t *conn, int code, const char *fmt,
+                         ...)
+{
+  va_list ap;
+
+  va_start(ap, fmt);
+  control_vprintf_reply(conn, code, '+', fmt, ap);
+  va_end(ap);
+}
+
+/** Write a CmdData */
+void
+control_write_data(control_connection_t *conn, const char *data)
+{
+  char *esc = NULL;
+  size_t esc_len;
+
+  esc_len = write_escaped_data(data, strlen(data), &esc);
+  connection_buf_add(esc, esc_len, TO_CONN(conn));
+  tor_free(esc);
+}

src/feature/control/control_proto.h

@@ -0,0 +1,48 @@
+/* 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_proto.h
+ * \brief Header file for control_proto.c.
+ **/
+
+#ifndef TOR_CONTROL_PROTO_H
+#define TOR_CONTROL_PROTO_H
+
+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, ...)
+  CHECK_PRINTF(2,3);
+
+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);
+void send_control_done(control_connection_t *conn);
+
+void control_write_reply(control_connection_t *conn, int code, int c,
+                         const char *s);
+void control_vprintf_reply(control_connection_t *conn, int code, int c,
+                           const char *fmt, va_list ap)
+  CHECK_PRINTF(4, 0);
+void control_write_endreply(control_connection_t *conn, int code,
+                            const char *s);
+void control_printf_endreply(control_connection_t *conn, int code,
+                             const char *fmt, ...)
+  CHECK_PRINTF(3, 4);
+void control_write_midreply(control_connection_t *conn, int code,
+                            const char *s);
+void control_printf_midreply(control_connection_t *conn, int code,
+                             const char *fmt,
+                             ...)
+  CHECK_PRINTF(3, 4);
+void control_write_datareply(control_connection_t *conn, int code,
+                             const char *s);
+void control_printf_datareply(control_connection_t *conn, int code,
+                              const char *fmt,
+                              ...)
+  CHECK_PRINTF(3, 4);
+void control_write_data(control_connection_t *conn, const char *data);
+
+#endif /* !defined(TOR_CONTROL_PROTO_H) */

src/test/test_util.c

@@ -15,7 +15,7 @@
 #include "lib/buf/buffers.h"
 #include "app/config/config.h"
 #include "feature/control/control.h"
-#include "feature/control/control_fmt.h"
+#include "feature/control/control_proto.h"
 #include "feature/client/transports.h"
 #include "lib/crypt_ops/crypto_format.h"
 #include "lib/crypt_ops/crypto_rand.h"