From 4b22c739fe8b04b05e7bdfdf18ec42ec4bf1c436 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Wed, 3 Jul 2019 15:13:51 -0500 Subject: [PATCH 01/10] clean up kvline_can_encode_lines() Add a check for '=' characters in needs_escape(). This simplifies the logic in kvline_can_encode_lines(). Part of #30984. --- src/lib/encoding/kvline.c | 28 +++++++++++++--------------- src/test/test_config.c | 22 ++++++++++++++++++++++ 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/lib/encoding/kvline.c b/src/lib/encoding/kvline.c index d4a8f510ba..e56b57b4cf 100644 --- a/src/lib/encoding/kvline.c +++ b/src/lib/encoding/kvline.c @@ -29,12 +29,20 @@ #include /** Return true iff we need to quote and escape the string s to encode - * it. */ + * it. + * + * kvline_can_encode_lines() also uses this (with + * as_keyless_val 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,16 @@ 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. */ 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; } @@ -142,9 +143,6 @@ 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)) { diff --git a/src/test/test_config.c b/src/test/test_config.c index a75a862739..6294e21f04 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -6050,6 +6050,28 @@ 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); done: config_free_lines(lines); From 1e8bb79bbe9befd86e018e0d5f6d960e9c789462 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Wed, 3 Jul 2019 16:35:35 -0500 Subject: [PATCH 02/10] add KV_RAW to kvline.c Add the KV_RAW flag to kvline_encode(). This allows generation of output that is compatible with some quirks of the control protocol. Part of #30984. --- src/lib/encoding/kvline.c | 24 +++++++++++++++++------- src/lib/encoding/kvline.h | 1 + src/test/test_config.c | 8 ++++++++ 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/lib/encoding/kvline.c b/src/lib/encoding/kvline.c index e56b57b4cf..f55e3d966f 100644 --- a/src/lib/encoding/kvline.c +++ b/src/lib/encoding/kvline.c @@ -85,8 +85,9 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags) 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 (!keyless && needs_escape(line->key, true)) { @@ -104,7 +105,7 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags) * * If KV_QUOTED is set in flags, 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 flags, then pairs with empty keys are * allowed, and are encoded as 'Value'. Otherwise, such pairs are not @@ -114,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 flags, 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 * @@ -122,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(); @@ -148,7 +155,7 @@ kvline_encode(const config_line_t *line, 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 { @@ -185,12 +192,15 @@ kvline_encode(const config_line_t *line, * If KV_QUOTED_QSTRING is set in flags, 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 6294e21f04..8f705da7e0 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -6072,6 +6072,14 @@ test_config_kvline_parse(void *arg) 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); From 1a68a18093d38f9f5b7ed66c8d42e41a565febe0 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Wed, 3 Jul 2019 23:16:23 -0500 Subject: [PATCH 03/10] reply lines structures Part of #30984. --- src/feature/control/control_proto.c | 157 ++++++++++++++++++++++++++++ src/feature/control/control_proto.h | 72 +++++++++++++ src/test/test_controller.c | 71 +++++++++++++ 3 files changed, 300 insertions(+) diff --git a/src/feature/control/control_proto.c b/src/feature/control/control_proto.c index 5dec87491d..4efe735c72 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 s to the end of * conn-\>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_1kv(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_1kv(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..65c81924fd 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_1kv() 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_1kv(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/test/test_controller.c b/src/test/test_controller.c index d07ec5d0f0..eb68e48476 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_1kv(lines, 250, 0, "A", "B"); + control_reply_add_1kv(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_1kv(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 }; From c744d23c8dce73ded9146661001d910295f9bcbd Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Wed, 3 Jul 2019 23:48:17 -0500 Subject: [PATCH 04/10] simplify getconf by using reply lines In handle_control_getconf(), use the new control reply line abstraction to simplify output generation. Previously, this function explicitly checked for whether it should generate a MidReplyLine or an EndReplyLine. control_write_reply_lines() now abstracts this check. Part of #30984. --- src/feature/control/control_cmd.c | 33 +++++++++---------------------- 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 656ddf5ca1..d4fb17847d 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_1kv(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_1kv(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; } From 2143bae6c40a282f7184460452236192d2168e02 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Thu, 22 Aug 2019 16:58:30 -0500 Subject: [PATCH 05/10] refactor handle_control_protocolinfo Factor out the parts of handle_control_protocolinfo() that assemble the AUTHMETHODS and COOKIEFILE strings. Part of #30984. --- src/feature/control/control_cmd.c | 101 +++++++++++++++++++----------- 1 file changed, 63 insertions(+), 38 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index d4fb17847d..0f279b08d4 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -1242,6 +1242,64 @@ 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; +} + +/** Send the auth methods lines of a PROTOCOLINFO reply. */ +static void +send_authmethods(control_connection_t *conn) +{ + const or_options_t *options = get_options(); + char *methods = get_authmethods(options); + char *esc_cfile = get_esc_cfile(options); + + control_printf_midreply(conn, 250, "AUTH METHODS=%s%s%s", methods, + esc_cfile ? " COOKIEFILE=" : "", + esc_cfile ? 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, @@ -1266,45 +1324,12 @@ 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: + control_write_midreply(conn, 250, "PROTOCOLINFO 1"); + send_authmethods(conn); + control_printf_midreply(conn, 250, "VERSION Tor=%s", escaped(VERSION)); + send_control_done(conn); return 0; } From a08f43ba045e051f16f3ea211be0438c09a906d6 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Thu, 22 Aug 2019 16:58:30 -0500 Subject: [PATCH 06/10] use control reply lines for protocolinfo Simplify handle_control_protocolinfo() by using the new reply line abstraction. Part of #30984. --- src/feature/control/control_cmd.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 0f279b08d4..ce7d1ad2cf 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -1285,17 +1285,19 @@ get_esc_cfile(const or_options_t *options) return esc_cfile; } -/** Send the auth methods lines of a PROTOCOLINFO reply. */ +/** Compose the auth methods line of a PROTOCOLINFO reply. */ static void -send_authmethods(control_connection_t *conn) +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_printf_midreply(conn, 250, "AUTH METHODS=%s%s%s", methods, - esc_cfile ? " COOKIEFILE=" : "", - esc_cfile ? esc_cfile : ""); + 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); } @@ -1307,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; @@ -1326,10 +1329,15 @@ handle_control_protocolinfo(control_connection_t *conn, connection_mark_for_close(TO_CONN(conn)); return 0; } - control_write_midreply(conn, 250, "PROTOCOLINFO 1"); - send_authmethods(conn); - control_printf_midreply(conn, 250, "VERSION Tor=%s", escaped(VERSION)); - send_control_done(conn); + 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; } From 9b196f15638ea490d1d0892cf320c5afc66a1eef Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Thu, 22 Aug 2019 17:54:33 -0500 Subject: [PATCH 07/10] simplify getinfo using reply lines Simplify handle_control_getinfo() by using the new reply lines abstraction. Previously, this function explicitly checked for whether it should generate a MidReplyLine, a DataReplyLine, or an EndReplyLine. control_write_reply_lines() now abstracts this check. Part of #30984. --- src/feature/control/control_getinfo.c | 45 ++++++++++----------------- 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c index 979fa4480d..20dc4d082f 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_1kv(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; } From 69d56625d14ad61db389acd4e0cdde7582f13c56 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Sun, 8 Dec 2019 21:58:18 -0600 Subject: [PATCH 08/10] Doxyfile: skip CHECK_PRINTF() Part of ticket 30984. --- Doxyfile.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 From bfe38878b28a90fb80d9e6311d28dcabd978b1ba Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Sun, 8 Dec 2019 22:20:16 -0600 Subject: [PATCH 09/10] Rename control_reply_add_1kv Part of ticket 30984. --- src/feature/control/control_cmd.c | 6 +++--- src/feature/control/control_getinfo.c | 2 +- src/feature/control/control_proto.c | 6 +++--- src/feature/control/control_proto.h | 6 +++--- src/test/test_controller.c | 6 +++--- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index ce7d1ad2cf..cc4375112f 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -299,13 +299,13 @@ handle_control_getconf(control_connection_t *conn, config_line_t *answer = option_get_assignment(options,q); if (!answer) { const char *name = option_get_canonical_name(q); - control_reply_add_1kv(answers, 250, KV_OMIT_VALS, name, ""); + control_reply_add_one_kv(answers, 250, KV_OMIT_VALS, name, ""); } while (answer) { config_line_t *next; - control_reply_add_1kv(answers, 250, KV_RAW, 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); diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c index 20dc4d082f..cff4a08793 100644 --- a/src/feature/control/control_getinfo.c +++ b/src/feature/control/control_getinfo.c @@ -1653,7 +1653,7 @@ handle_control_getinfo(control_connection_t *conn, "Unrecognized key \"%s\"", q); } } else { - control_reply_add_1kv(answers, 250, KV_RAW, q, ans); + control_reply_add_one_kv(answers, 250, KV_RAW, q, ans); } } SMARTLIST_FOREACH_END(q); diff --git a/src/feature/control/control_proto.c b/src/feature/control/control_proto.c index 4efe735c72..7d04fea6a7 100644 --- a/src/feature/control/control_proto.c +++ b/src/feature/control/control_proto.c @@ -335,8 +335,8 @@ control_write_reply_lines(control_connection_t *conn, smartlist_t *lines) * @param val value */ void -control_reply_add_1kv(smartlist_t *reply, int code, int flags, - const char *key, const char *val) +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)); @@ -374,7 +374,7 @@ 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) { - control_reply_add_1kv(reply, code, KV_OMIT_KEYS|KV_RAW, "", s); + control_reply_add_one_kv(reply, code, KV_OMIT_KEYS|KV_RAW, "", s); } /** Format a new reply line diff --git a/src/feature/control/control_proto.h b/src/feature/control/control_proto.h index 65c81924fd..cf7c000439 100644 --- a/src/feature/control/control_proto.h +++ b/src/feature/control/control_proto.h @@ -29,7 +29,7 @@ * 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_1kv() and + * single string, or control_reply_add_one_kv() and * control_reply_append_kv() for composing a line containing one or * more key-value pairs. * @@ -97,8 +97,8 @@ 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_1kv(smartlist_t *reply, int code, int flags, - const char *key, const char *val); +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); diff --git a/src/test/test_controller.c b/src/test/test_controller.c index eb68e48476..b3023130ae 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -1993,8 +1993,8 @@ test_control_reply(void *arg) UNMOCK(control_write_reply); MOCK(control_write_reply, mock_control_write_reply_list); reply_strs = smartlist_new(); - control_reply_add_1kv(lines, 250, 0, "A", "B"); - control_reply_add_1kv(lines, 250, 0, "C", "D"); + 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"); @@ -2004,7 +2004,7 @@ test_control_reply(void *arg) 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_1kv(lines, 250, KV_OMIT_VALS|KV_RAW, "AUTH", ""); + 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); From 7bd7089988a5745d0829f3ebefddd2f3889c1428 Mon Sep 17 00:00:00 2001 From: Taylor Yu Date: Sun, 8 Dec 2019 22:37:58 -0600 Subject: [PATCH 10/10] changes file for ticket 30984 --- changes/ticket30984 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/ticket30984 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.