r14830@catbus: nickm | 2007-08-29 13:50:10 -0400

Make controllers accept LF as well as CRLF.  Update spec to reflect this.  Remove now-dead code.  Make controller warning about v0 protocol more accurate.


svn:r11299

ChangeLog

@@ -9,6 +9,10 @@ Changes in version 0.2.0.7-alpha - 2007-??-??
       temporarily runs an old version of Tor and then switches back to a new
       one, she doesn't automatically lose her guards.
 
+  o Minor features (controller):
+    - Accept LF instead of CRLF on controller, since some software has a
+      hard time generating real Internet newlines.
+
   o Minor bugfixes:
     - When generating information telling us how to extend to a given
       router, do not try to include the nickname if it is absent.  Fixes

doc/TODO

@@ -144,7 +144,10 @@ N     - Design/implement the "local-status" or something like it, from the
       - Write a proposal; make this part of 105.
     - Audit how much RAM we're using for buffers and cell pools; try to
       trim down a lot.
-    - Accept \n as end of lines in the control protocol in addition to \r\n.
+    o Accept \n as end of lines in the control protocol in addition to \r\n.
+      o Use fetch_from_buf_line_lf in control.c instead of fetch_from_buf_line.
+      o Fix up read escaped_data to accept LF instead of CRLF, and to
+        always translate_newlines (since that's the only way it's called).
     - Base relative control socket paths on datadir.
     - We should ship with a list of stable dir mirrors -- they're not
       trusted like the authorities, but they'll provide more robustness

doc/spec/control-spec.txt

@@ -52,6 +52,10 @@ $Id$
   There are explicitly no limits on line length.  All 8-bit characters are
   permitted unless explicitly disallowed.
 
+  Wherever CRLF is specified to be accepted from the controller, Tor MAY also
+  accept LF.  Tor, however, MUST NOT generate LF instead of CRLF.
+  Controllers SHOULD always send CRLF.
+
 2.2. Commands from controller to Tor
 
     Command = Keyword Arguments CRLF / "+" Keyword Arguments CRLF Data

src/or/buffers.c

@@ -1462,52 +1462,6 @@ find_char_on_buf(buf_t *buf, char *start, size_t len, char c)
   return memchr(buf->mem, c, len_rest);
 }
 
-/** Helper: return a pointer to the first CRLF after cp on <b>buf</b>. Return
- * NULL if no CRLF is found. */
-static char *
-find_crlf_on_buf(buf_t *buf, char *cp)
-{
-  char *next;
-  while (1) {
-    size_t remaining = buf->datalen - _buf_offset(buf,cp);
-    cp = find_char_on_buf(buf, cp, remaining, '\r');
-    if (!cp)
-      return NULL;
-    next = _wrap_ptr(buf, cp+1);
-    if (next == _buf_end(buf))
-      return NULL;
-    if (*next == '\n')
-      return cp;
-    cp = next;
-  }
-}
-
-/** Try to read a single CRLF-terminated line from <b>buf</b>, and write it,
- * NUL-terminated, into the *<b>data_len</b> byte buffer at <b>data_out</b>.
- * Set *<b>data_len</b> to the number of bytes in the line, not counting the
- * terminating NUL.  Return 1 if we read a whole line, return 0 if we don't
- * have a whole line yet, and return -1 if we we need to grow the buffer.
- */
-int
-fetch_from_buf_line(buf_t *buf, char *data_out, size_t *data_len)
-{
-  char *eol;
-  size_t sz;
-  /* Look for a CRLF. */
-  if (!(eol = find_crlf_on_buf(buf, buf->cur))) {
-    return 0;
-  }
-  sz = _buf_offset(buf, eol);
-  if (sz+3 > *data_len) {
-    *data_len = sz+3;
-    return -1;
-  }
-  fetch_from_buf(data_out, sz+2, buf);
-  data_out[sz+2] = '\0';
-  *data_len = sz+2;
-  return 1;
-}
-
 /** Try to read a single LF-terminated line from <b>buf</b>, and write it,
  * NUL-terminated, into the *<b>data_len</b> byte buffer at <b>data_out</b>.
  * Set *<b>data_len</b> to the number of bytes in the line, not counting the
@@ -1516,7 +1470,7 @@ fetch_from_buf_line(buf_t *buf, char *data_out, size_t *data_len)
  *<b>data_len</b>.
  */
 int
-fetch_from_buf_line_lf(buf_t *buf, char *data_out, size_t *data_len)
+fetch_from_buf_line(buf_t *buf, char *data_out, size_t *data_len)
 {
   char *cp;
   size_t sz;

src/or/connection_edge.c

@@ -1725,7 +1725,7 @@ connection_ap_process_natd(edge_connection_t *conn)
 
   /* look for LF-terminated "[DEST ip_addr port]"
    * where ip_addr is a dotted-quad and port is in string form */
-  err = fetch_from_buf_line_lf(conn->_base.inbuf, tmp_buf, &tlen);
+  err = fetch_from_buf_line(conn->_base.inbuf, tmp_buf, &tlen);
   if (err == 0)
     return 0;
   if (err < 0) {

src/or/control.c

@@ -272,16 +272,14 @@ connection_write_str_to_buf(const char *s, control_connection_t *conn)
 }
 
 /** 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
+ * terminated by CRLF, allocate a new string in *<b>out</b>, and copy the
- * the contents of <b>data</b> into *<b>out</b>, adding a period
+ * contents of <b>data</b> into *<b>out</b>, adding a period before any period
- * before any period that that appears at the start of a line, and
+ * that that appears at the start of a line, and adding a period-CRLF line at
- * adding a period-CRLF line at the end. If <b>translate_newlines</b>
+ * the end. Replace all LF characters sequences with CRLF.  Return the number
- * is true, replace all LF characters sequences with CRLF.  Return the
+ * of bytes in *<b>out</b>.
- * number of bytes in *<b>out</b>.
  */
 /* static */ size_t
-write_escaped_data(const char *data, size_t len, int translate_newlines,
+write_escaped_data(const char *data, size_t len, char **out)
-                   char **out)
 {
   size_t sz_out = len+8;
   char *outp;
@@ -297,7 +295,7 @@ write_escaped_data(const char *data, size_t len, int translate_newlines,
   start_of_line = 1;
   while (data < end) {
     if (*data == '\n') {
-      if (translate_newlines && data > start && data[-1] != '\r')
+      if (data > start && data[-1] != '\r')
         *outp++ = '\r';
       start_of_line = 1;
     } else if (*data == '.') {
@@ -325,12 +323,11 @@ write_escaped_data(const char *data, size_t len, int translate_newlines,
 /** 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.  If <b>translate_newlines</b>
+ * that appears at the start of a line, and replacing all CRLF sequences
- * is true, replace all CRLF sequences with LF.  Return the number of
+ * with LF.   Return the number of
  * bytes in *<b>out</b>. */
 /* static */ size_t
-read_escaped_data(const char *data, size_t len, int translate_newlines,
+read_escaped_data(const char *data, size_t len, char **out)
-                  char **out)
 {
   char *outp;
   const char *next;
@@ -341,28 +338,26 @@ read_escaped_data(const char *data, size_t len, int translate_newlines,
   end = data+len;
 
   while (data < end) {
+    /* we're at the start of a line. */
     if (*data == '.')
       ++data;
-    if (translate_newlines)
+    next = memchr(data, '\n', end-data);
-      next = tor_memmem(data, end-data, "\r\n", 2);
-    else
-      next = tor_memmem(data, end-data, "\r\n.", 3);
     if (next) {
-      memcpy(outp, data, next-data);
+      size_t n_to_copy = next-data;
-      outp += (next-data);
+      /* Don't copy a CR that precedes this LF. */
-      data = next+2;
+      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;
     }
-    if (translate_newlines) {
+    *outp++ = '\n';
-      *outp++ = '\n';
-    } else {
-      *outp++ = '\r';
-      *outp++ = '\n';
-    }
   }
 
   *outp = '\0';
@@ -1818,7 +1813,7 @@ handle_control_getinfo(control_connection_t *conn, uint32_t len,
     } else {
       char *esc = NULL;
       size_t esc_len;
-      esc_len = write_escaped_data(v, strlen(v), 1, &esc);
+      esc_len = write_escaped_data(v, strlen(v), &esc);
       connection_printf_to_buf(conn, "250+%s=\r\n", k);
       connection_write_to_buf(esc, esc_len, TO_CONN(conn));
       tor_free(esc);
@@ -2002,6 +1997,8 @@ static int
 handle_control_setpurpose(control_connection_t *conn, int for_circuits,
                           uint32_t len, const char *body)
 {
+  /* XXXX020 this should maybe be two functions; almost no code is acutally
+     shared. */
   origin_circuit_t *circ = NULL;
   routerinfo_t *ri = NULL;
   uint8_t new_purpose;
@@ -2170,7 +2167,7 @@ handle_control_postdescriptor(control_connection_t *conn, uint32_t len,
   }
   SMARTLIST_FOREACH(args, char *, arg, tor_free(arg));
   smartlist_free(args);
-  read_escaped_data(cp, len-(cp-body), 1, &desc);
+  read_escaped_data(cp, len-(cp-body), &desc);
 
   switch (router_load_single_router(desc, purpose, &msg)) {
   case -1:
@@ -2530,8 +2527,8 @@ connection_control_process_inbuf(control_connection_t *conn)
     char buf[128];
     set_uint16(buf+2, htons(0x0000)); /* type == error */
     set_uint16(buf+4, htons(0x0001)); /* code == internal error */
-    strlcpy(buf+6, "The v0 control protocol is not supported by Tor 0.2.0.x "
+    strlcpy(buf+6, "The v0 control protocol is not supported by Tor 0.1.2.17 "
-            "and later; use Tor 0.1.2.x or upgrade your controller",
+            "and later; upgrade your controller.",
             sizeof(buf)-6);
     body_len = 2+strlen(buf+6)+2; /* code, msg, nul. */
     set_uint16(buf+0, htons(body_len));
@@ -2571,11 +2568,17 @@ connection_control_process_inbuf(control_connection_t *conn)
     if (last_idx == 0 && *conn->incoming_cmd != '+')
       /* One line command, didn't start with '+'. */
       break;
+    /* XXXX this code duplication is kind of dumb. */
     if (last_idx+3 == conn->incoming_cmd_cur_len &&
         !memcmp(conn->incoming_cmd + last_idx, ".\r\n", 3)) {
       /* Just appended ".\r\n"; we're done. Remove it. */
       conn->incoming_cmd_cur_len -= 3;
       break;
+    } else if (last_idx+2 == conn->incoming_cmd_cur_len &&
+               !memcmp(conn->incoming_cmd + last_idx, ".\n", 2)) {
+      /* Just appended ".\n"; we're done. Remove it. */
+      conn->incoming_cmd_cur_len -= 2;
+      break;
     }
     /* Otherwise, read another line. */
   }
@@ -3309,7 +3312,7 @@ control_event_or_authdir_new_descriptor(const char *action,
                msg ? msg : "");
 
   /* Escape the server descriptor properly */
-  esclen = write_escaped_data(desc, desclen, 1, &esc);
+  esclen = write_escaped_data(desc, desclen, &esc);
 
   totallen = strlen(firstline) + esclen + 1;
   buf = tor_malloc(totallen);
@@ -3345,7 +3348,7 @@ control_event_networkstatus_changed(smartlist_t *statuses)
     });
 
   s = smartlist_join_strings(strs, "", 0, NULL);
-  write_escaped_data(s, strlen(s), 1, &esc);
+  write_escaped_data(s, strlen(s), &esc);
   SMARTLIST_FOREACH(strs, char *, cp, tor_free(cp));
   smartlist_free(strs);
   tor_free(s);

src/or/or.h

@@ -2241,7 +2241,6 @@ int fetch_from_buf_http(buf_t *buf,
 int fetch_from_buf_socks(buf_t *buf, socks_request_t *req,
                          int log_sockstype, int safe_socks);
 int fetch_from_buf_line(buf_t *buf, char *data_out, size_t *data_len);
-int fetch_from_buf_line_lf(buf_t *buf, char *data_out, size_t *data_len);
 
 int peek_buf_has_control0_command(buf_t *buf);
 
@@ -2720,10 +2719,8 @@ void enable_control_logging(void);
 
 #ifdef CONTROL_PRIVATE
 /* Used only by control.c and test.c */
-size_t write_escaped_data(const char *data, size_t len,
+size_t write_escaped_data(const char *data, size_t len, char **out);
-                          int translate_newlines, char **out);
+size_t read_escaped_data(const char *data, size_t len, char **out);
-size_t read_escaped_data(const char *data, size_t len,
-                         int translate_newlines,  char **out);
 #endif
 
 /********************************* cpuworker.c *****************************/

src/or/test.c

@@ -2000,7 +2000,7 @@ test_util_control_formats(void)
     "..This is a test\r\nof the emergency \nbroadcast\r\n..system.\r\nZ.\r\n";
   size_t sz;
 
-  sz = read_escaped_data(inp, strlen(inp), 1, &out);
+  sz = read_escaped_data(inp, strlen(inp), &out);
   test_streq(out,
              ".This is a test\nof the emergency \nbroadcast\n.system.\nZ.\n");
   test_eq(sz, strlen(out));