From 91f83cfc2d380aa76fd049d13cf9fb097fecaab6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 29 Aug 2007 19:02:33 +0000 Subject: [PATCH] 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 | 4 +++ doc/TODO | 5 ++- doc/spec/control-spec.txt | 4 +++ src/or/buffers.c | 48 +--------------------------- src/or/connection_edge.c | 2 +- src/or/control.c | 67 ++++++++++++++++++++------------------- src/or/or.h | 7 ++-- src/or/test.c | 2 +- 8 files changed, 52 insertions(+), 87 deletions(-) diff --git a/ChangeLog b/ChangeLog index a0745e2f5..6128daa33 100644 --- a/ChangeLog +++ b/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 diff --git a/doc/TODO b/doc/TODO index d25d8fdfb..2af02f1a8 100644 --- a/doc/TODO +++ b/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 diff --git a/doc/spec/control-spec.txt b/doc/spec/control-spec.txt index fc9d3188d..4d8946b93 100644 --- a/doc/spec/control-spec.txt +++ b/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 diff --git a/src/or/buffers.c b/src/or/buffers.c index 3af8af6ca..3033bd3ad 100644 --- a/src/or/buffers.c +++ b/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 buf. 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 buf, and write it, - * NUL-terminated, into the *data_len byte buffer at data_out. - * Set *data_len 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 buf, and write it, * NUL-terminated, into the *data_len byte buffer at data_out. * Set *data_len 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) *data_len. */ 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; diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index f4431bacf..50b5c48aa 100644 --- a/src/or/connection_edge.c +++ b/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) { diff --git a/src/or/control.c b/src/or/control.c index a6e14b4c8..3056d30b7 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -272,16 +272,14 @@ connection_write_str_to_buf(const char *s, control_connection_t *conn) } /** Given a len-character string in data, made of lines - * terminated by CRLF, allocate a new string in *out, and copy - * the contents of data into *out, adding a period - * before any period that that appears at the start of a line, and - * adding a period-CRLF line at the end. If translate_newlines - * is true, replace all LF characters sequences with CRLF. Return the - * number of bytes in *out. + * terminated by CRLF, allocate a new string in *out, and copy the + * contents of data into *out, adding a period before any period + * that 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 *out. */ /* static */ size_t -write_escaped_data(const char *data, size_t len, int translate_newlines, - char **out) +write_escaped_data(const char *data, size_t len, 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 len-character string in data, made of lines * terminated by CRLF, allocate a new string in *out, and copy * the contents of data into *out, removing any period - * that appears at the start of a line. If translate_newlines - * is true, replace all CRLF sequences with LF. Return the number of + * that appears at the start of a line, and replacing all CRLF sequences + * with LF. Return the number of * bytes in *out. */ /* static */ size_t -read_escaped_data(const char *data, size_t len, int translate_newlines, - char **out) +read_escaped_data(const char *data, size_t len, 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 = tor_memmem(data, end-data, "\r\n", 2); - else - next = tor_memmem(data, end-data, "\r\n.", 3); + next = memchr(data, '\n', end-data); if (next) { - memcpy(outp, data, next-data); - outp += (next-data); - data = next+2; + 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; } - if (translate_newlines) { - *outp++ = '\n'; - } else { - *outp++ = '\r'; - *outp++ = '\n'; - } + *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 " - "and later; use Tor 0.1.2.x or upgrade your controller", + strlcpy(buf+6, "The v0 control protocol is not supported by Tor 0.1.2.17 " + "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); diff --git a/src/or/or.h b/src/or/or.h index aded80613..f5ad821a4 100644 --- a/src/or/or.h +++ b/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, - int translate_newlines, char **out); -size_t read_escaped_data(const char *data, size_t len, - int translate_newlines, char **out); +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); #endif /********************************* cpuworker.c *****************************/ diff --git a/src/or/test.c b/src/or/test.c index a72e2a401..8c1361f63 100644 --- a/src/or/test.c +++ b/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));