April 2019 - tor-commits - lists.torproject.org

[tor/master] Add assertions for correct input to handle_control_command.
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit f3bd0240a6089910f3075a779b32e7aed07338e0 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Apr 1 17:29:46 2019 -0400 Add assertions for correct input to handle_control_command. --- src/feature/control/control_cmd.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 4fcfa7310..56202a7ef 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -2398,6 +2398,10 @@ handle_control_command(control_connection_t *conn, uint32_t cmd_data_len, char *args) { + tor_assert(conn); + tor_assert(args); + tor_assert(args[cmd_data_len] == '\0'); + for (unsigned i = 0; i < N_CONTROL_COMMANDS; ++i) { const control_cmd_def_t *def = &CONTROL_COMMANDS[i]; if (!strcasecmp(conn->incoming_cmd, def->name)) {

1 0

[tor/master] Improve handling of controller commands
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit d1f5957c4ea82d1622233c0dabd1e761df5200d4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Apr 1 17:27:08 2019 -0400 Improve handling of controller commands Use a table-based lookup to find the right command handler. This will serve as the basement for several future improvements, as we improve the API for parsing commands. --- src/feature/control/control_cmd.c | 298 +++++++++++++++++++++++++------------- 1 file changed, 198 insertions(+), 100 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 95cf0d561..4fcfa7310 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -82,7 +82,7 @@ handle_control_resetconf(control_connection_t *conn, uint32_t len, char *body) * reply with a CONFVALUE or an ERROR message */ static int handle_control_getconf(control_connection_t *conn, uint32_t body_len, - const char *body) + char *body) { smartlist_t *questions = smartlist_new(); smartlist_t *answers = smartlist_new(); @@ -2206,110 +2206,208 @@ handle_control_del_onion(control_connection_t *conn, return 0; } +/** + * Called when we get an obsolete command: tell the controller that it is + * obsolete. + */ +static int +handle_control_obsolete(control_connection_t *conn, + uint32_t arg_len, + const char *args) +{ + (void)arg_len; + (void)args; + char *command = tor_strdup(conn->incoming_cmd); + tor_strupper(command); + connection_printf_to_buf(conn, "511 %s is obsolete.\r\n", command); + tor_free(command); + return 0; +} + +/** + * Selects an API to a controller command. See handler_fn_t for the + * possible types. + **/ +typedef enum handler_type_t { + hnd_legacy, + hnd_legacy_mut +} handler_type_t; + +/** + * Union: a function pointer to a handler function for a controller command. + * + * This needs to be a union (rather than just a single pointer) since not + * all controller commands have the same type. + **/ +typedef union handler_fn_t { + /** + * A "legacy" handler takes a command's arguments as a nul-terminated + * string, and their length. It may not change the contents of the + * arguments. If the command is a multiline one, then the arguments may + * extend across multiple lines. + */ + int (*legacy)(control_connection_t *conn, + uint32_t arg_len, + const char *args); + /** + * A "legacy_mut" handler is the same as a "legacy" one, except that it may + * change the contents of the command's arguments -- for example, by + * inserting NULs. It may not deallocate them. + */ + int (*legacy_mut)(control_connection_t *conn, + uint32_t arg_len, + char *args); +} handler_fn_t; + +/** + * Definition for a controller command. + */ +typedef struct control_cmd_def_t { + /** + * The name of the command. If the command is multiline, the name must + * begin with "+". This is not case-sensitive. */ + const char *name; + /** + * Which API to use when calling the handler function. + */ + handler_type_t handler_type; + /** + * A function to execute the command. + */ + handler_fn_t handler; + /** + * Zero or more CMD_FL_* flags, or'd together. + */ + unsigned flags; +} control_cmd_def_t; + +/** + * Indicates that the command's arguments are sensitive, and should be + * memwiped after use. + */ +#define CMD_FL_WIPE (1u<<0) + +/** + * Macro: declare a command with a one-line argument and a given set of + * flags. + **/ +#define ONE_LINE(name, htype, flags) \ + { #name, \ + hnd_ ##htype, \ + { .htype = handle_control_ ##name }, \ + flags \ + } +/** + * Macro: declare a command with a multi-line argument and a given set of + * flags. + **/ +#define MULTLINE(name, htype, flags) \ + { "+"#name, \ + hnd_ ##htype, \ + { .htype = handle_control_ ##name }, \ + flags \ + } +/** + * Macro: declare an obsolete command. (Obsolete commands give a different + * error than non-existent ones.) + **/ +#define OBSOLETE(name) \ + { #name, \ + hnd_legacy, \ + { .legacy = handle_control_obsolete }, \ + 0 \ + } + +/** + * An array defining all the recognized controller commands. + **/ +static const control_cmd_def_t CONTROL_COMMANDS[] = +{ + ONE_LINE(setconf, legacy_mut, 0), + ONE_LINE(resetconf, legacy_mut, 0), + ONE_LINE(getconf, legacy_mut, 0), + MULTLINE(loadconf, legacy, 0), + ONE_LINE(setevents, legacy, 0), + ONE_LINE(authenticate, legacy, 0), + ONE_LINE(saveconf, legacy, 0), + ONE_LINE(signal, legacy, 0), + ONE_LINE(takeownership, legacy, 0), + ONE_LINE(dropownership, legacy, 0), + ONE_LINE(mapaddress, legacy, 0), + ONE_LINE(getinfo, legacy, 0), + ONE_LINE(extendcircuit, legacy, 0), + ONE_LINE(setcircuitpurpose, legacy, 0), + OBSOLETE(setrouterpurpose), + ONE_LINE(attachstream, legacy, 0), + MULTLINE(postdescriptor, legacy, 0), + ONE_LINE(redirectstream, legacy, 0), + ONE_LINE(closestream, legacy, 0), + ONE_LINE(closecircuit, legacy, 0), + ONE_LINE(usefeature, legacy, 0), + ONE_LINE(resolve, legacy, 0), + ONE_LINE(protocolinfo, legacy, 0), + ONE_LINE(authchallenge, legacy, 0), + ONE_LINE(dropguards, legacy, 0), + ONE_LINE(hsfetch, legacy, 0), + MULTLINE(hspost, legacy, 0), + ONE_LINE(add_onion, legacy, CMD_FL_WIPE), + ONE_LINE(del_onion, legacy, CMD_FL_WIPE), +}; + +/** + * The number of entries in CONTROL_COMMANDS. + **/ +static const size_t N_CONTROL_COMMANDS = ARRAY_LENGTH(CONTROL_COMMANDS); + +/** + * Run a single control command, as defined by a control_cmd_def_t, + * with a given set of arguments. + */ +static int +handle_single_control_command(const control_cmd_def_t *def, + control_connection_t *conn, + uint32_t cmd_data_len, + char *args) +{ + int rv = 0; + switch (def->handler_type) { + case hnd_legacy: + if (def->handler.legacy(conn, cmd_data_len, args)) + rv = -1; + break; + case hnd_legacy_mut: + if (def->handler.legacy_mut(conn, cmd_data_len, args)) + rv = -1; + break; + default: + tor_assert_unreached(); + } + + if (def->flags & CMD_FL_WIPE) + memwipe(args, 0, cmd_data_len); + + return rv; +} + +/** + * Run a given controller command, as selected by the incoming_cmd field of + * conn. + */ int handle_control_command(control_connection_t *conn, - uint32_t cmd_data_len, - char *args) + uint32_t cmd_data_len, + char *args) { - /* XXXX Why is this not implemented as a table like the GETINFO - * items are? Even handling the plus signs at the beginnings of - * commands wouldn't be very hard with proper macros. */ - - if (!strcasecmp(conn->incoming_cmd, "SETCONF")) { - if (handle_control_setconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "RESETCONF")) { - if (handle_control_resetconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "GETCONF")) { - if (handle_control_getconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "+LOADCONF")) { - if (handle_control_loadconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SETEVENTS")) { - if (handle_control_setevents(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "AUTHENTICATE")) { - if (handle_control_authenticate(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SAVECONF")) { - if (handle_control_saveconf(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SIGNAL")) { - if (handle_control_signal(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "TAKEOWNERSHIP")) { - if (handle_control_takeownership(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "DROPOWNERSHIP")) { - if (handle_control_dropownership(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "MAPADDRESS")) { - if (handle_control_mapaddress(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "GETINFO")) { - if (handle_control_getinfo(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "EXTENDCIRCUIT")) { - if (handle_control_extendcircuit(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SETCIRCUITPURPOSE")) { - if (handle_control_setcircuitpurpose(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "SETROUTERPURPOSE")) { - connection_write_str_to_buf("511 SETROUTERPURPOSE is obsolete.\r\n", conn); - } else if (!strcasecmp(conn->incoming_cmd, "ATTACHSTREAM")) { - if (handle_control_attachstream(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "+POSTDESCRIPTOR")) { - if (handle_control_postdescriptor(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "REDIRECTSTREAM")) { - if (handle_control_redirectstream(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "CLOSESTREAM")) { - if (handle_control_closestream(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "CLOSECIRCUIT")) { - if (handle_control_closecircuit(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "USEFEATURE")) { - if (handle_control_usefeature(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "RESOLVE")) { - if (handle_control_resolve(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "PROTOCOLINFO")) { - if (handle_control_protocolinfo(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "AUTHCHALLENGE")) { - if (handle_control_authchallenge(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "DROPGUARDS")) { - if (handle_control_dropguards(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "HSFETCH")) { - if (handle_control_hsfetch(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "+HSPOST")) { - if (handle_control_hspost(conn, cmd_data_len, args)) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "ADD_ONION")) { - int ret = handle_control_add_onion(conn, cmd_data_len, args); - memwipe(args, 0, cmd_data_len); /* Scrub the private key. */ - if (ret) - return -1; - } else if (!strcasecmp(conn->incoming_cmd, "DEL_ONION")) { - int ret = handle_control_del_onion(conn, cmd_data_len, args); - memwipe(args, 0, cmd_data_len); /* Scrub the service id/pk. */ - if (ret) - return -1; - } else { - connection_printf_to_buf(conn, "510 Unrecognized command \"%s\"\r\n", - conn->incoming_cmd); + for (unsigned i = 0; i < N_CONTROL_COMMANDS; ++i) { + const control_cmd_def_t *def = &CONTROL_COMMANDS[i]; + if (!strcasecmp(conn->incoming_cmd, def->name)) { + return handle_single_control_command(def, conn, cmd_data_len, args); + } } + connection_printf_to_buf(conn, "510 Unrecognized command \"%s\"\r\n", + conn->incoming_cmd); + return 0; }

1 0

[tor/master] Extract the argument-splitting part of control.c's parser
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit f18b7dc4731bcb853db92a0faaa4ec03d6ef5586 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 2 10:29:07 2019 -0400 Extract the argument-splitting part of control.c's parser This is preliminary work for fixing 29984; no behavior has changed. --- src/feature/control/control.c | 42 ++++++++++++++++++++++++++++-------------- src/feature/control/control.h | 5 +++++ 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/feature/control/control.c b/src/feature/control/control.c index 41e21c0a1..308f1936b 100644 --- a/src/feature/control/control.c +++ b/src/feature/control/control.c @@ -33,6 +33,7 @@ **/ #define CONTROL_MODULE_PRIVATE +#define CONTROL_PRIVATE #include "core/or/or.h" #include "app/config/config.h" @@ -274,6 +275,31 @@ peek_connection_has_http_command(connection_t *conn) return peek_buf_has_http_command(conn->inbuf); } +/** + * Helper: take a nul-terminated command of given length, and find where + * the command starts and the argument begins. Separate them with a NUL, + * and return a pointer to the arguments. + **/ +STATIC char * +control_split_incoming_command(char *incoming_cmd, size_t *data_len) +{ + size_t cmd_len = 0; + while (cmd_len < *data_len + && !TOR_ISSPACE(incoming_cmd[cmd_len])) + ++cmd_len; + + incoming_cmd[cmd_len]='\0'; + char *args = incoming_cmd+cmd_len+1; + tor_assert(*data_len>cmd_len); + *data_len -= (cmd_len+1); /* skip the command and NUL we added after it */ + while (TOR_ISSPACE(*args)) { + ++args; + --*data_len; + } + + return args; +} + static const char CONTROLPORT_IS_NOT_AN_HTTP_PROXY_MSG[] = "HTTP/1.0 501 Tor ControlPort is not an HTTP proxy" "\r\nContent-Type: text/html; charset=iso-8859-1\r\n\r\n" @@ -308,7 +334,6 @@ connection_control_process_inbuf(control_connection_t *conn) { size_t data_len; uint32_t cmd_data_len; - int cmd_len; char *args; tor_assert(conn); @@ -400,22 +425,11 @@ connection_control_process_inbuf(control_connection_t *conn) /* Otherwise, read another line. */ } data_len = conn->incoming_cmd_cur_len; + /* Okay, we now have a command sitting on conn->incoming_cmd. See if we * recognize it. */ - cmd_len = 0; - while ((size_t)cmd_len < data_len - && !TOR_ISSPACE(conn->incoming_cmd[cmd_len])) - ++cmd_len; - - conn->incoming_cmd[cmd_len]='\0'; - args = conn->incoming_cmd+cmd_len+1; - tor_assert(data_len>(size_t)cmd_len); - data_len -= (cmd_len+1); /* skip the command and NUL we added after it */ - while (TOR_ISSPACE(*args)) { - ++args; - --data_len; - } + args = control_split_incoming_command(conn->incoming_cmd, &data_len); /* If the connection is already closing, ignore further commands */ if (TO_CONN(conn)->marked_for_close) { diff --git a/src/feature/control/control.h b/src/feature/control/control.h index 308383793..6fc1c40ca 100644 --- a/src/feature/control/control.h +++ b/src/feature/control/control.h @@ -60,4 +60,9 @@ int get_cached_network_liveness(void); void set_cached_network_liveness(int liveness); #endif /* defined(CONTROL_MODULE_PRIVATE) */ +#ifdef CONTROL_PRIVATE +STATIC char *control_split_incoming_command(char *incoming_cmd, + size_t *data_len); +#endif + #endif /* !defined(TOR_CONTROL_H) */

1 0

[tor/master] Define two more commands as wipe-after-parse.
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit e9ca904dbfc99ebef567d7bb2c6d87819d0d832c Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Apr 1 17:30:46 2019 -0400 Define two more commands as wipe-after-parse. --- src/feature/control/control_cmd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 56202a7ef..930276b10 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -2328,7 +2328,7 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = ONE_LINE(getconf, legacy_mut, 0), MULTLINE(loadconf, legacy, 0), ONE_LINE(setevents, legacy, 0), - ONE_LINE(authenticate, legacy, 0), + ONE_LINE(authenticate, legacy, CMD_FL_WIPE), ONE_LINE(saveconf, legacy, 0), ONE_LINE(signal, legacy, 0), ONE_LINE(takeownership, legacy, 0), @@ -2346,7 +2346,7 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = ONE_LINE(usefeature, legacy, 0), ONE_LINE(resolve, legacy, 0), ONE_LINE(protocolinfo, legacy, 0), - ONE_LINE(authchallenge, legacy, 0), + ONE_LINE(authchallenge, legacy, CMD_FL_WIPE), ONE_LINE(dropguards, legacy, 0), ONE_LINE(hsfetch, legacy, 0), MULTLINE(hspost, legacy, 0),

1 0

[tor/master] Start on a command-parsing tool for controller commands.
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit de70eebc65d40d50f877b0f82df4d05ce670faa5 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 2 08:17:22 2019 -0400 Start on a command-parsing tool for controller commands. There _is_ an underlying logic to these commands, but it isn't wholly uniform, given years of tweaks and changes. Fortunately I think there is a superset that will work. This commit adds a parser for some of the most basic cases -- the ones currently handled by getargs_helper() and some of the object-taking ones. Soon will come initial tests; then I'll start using the parser. After that, I'll expand the parser to handle the other cases that come up in the controller protocol. --- src/core/include.am | 1 + src/feature/control/control_cmd.c | 145 ++++++++++++++++++++++++++++-- src/feature/control/control_cmd.h | 39 ++++++++ src/feature/control/control_cmd_args_st.h | 44 +++++++++ 4 files changed, 221 insertions(+), 8 deletions(-) diff --git a/src/core/include.am b/src/core/include.am index 982460172..b927f17a9 100644 --- a/src/core/include.am +++ b/src/core/include.am @@ -298,6 +298,7 @@ noinst_HEADERS += \ src/feature/control/control.h \ src/feature/control/control_auth.h \ src/feature/control/control_cmd.h \ + src/feature/control/control_cmd_args_st.h \ src/feature/control/control_connection_st.h \ src/feature/control/control_events.h \ src/feature/control/control_fmt.h \ diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 930276b10..aa0bef513 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -45,6 +45,7 @@ #include "core/or/entry_connection_st.h" #include "core/or/origin_circuit_st.h" #include "core/or/socks_request_st.h" +#include "feature/control/control_cmd_args_st.h" #include "feature/control/control_connection_st.h" #include "feature/nodelist/node_st.h" #include "feature/nodelist/routerinfo_st.h" @@ -60,6 +61,87 @@ static int control_setconf_helper(control_connection_t *conn, uint32_t len, * finished authentication and is accepting commands. */ #define STATE_IS_OPEN(s) ((s) == CONTROL_CONN_STATE_OPEN) +/** + * Release all storage held in args + **/ +void +control_cmd_args_free_(control_cmd_args_t *args) +{ + if (! args) + return; + + if (args->args) { + SMARTLIST_FOREACH(args->args, char *, c, tor_free(c)); + smartlist_free(args->args); + } + tor_free(args->object); + + tor_free(args); +} + +/** + * Helper: parse the arguments to a command according to syntax. On + * success, set *error_out to NULL and return a newly allocated + * control_cmd_args_t. On failure, set *error_out to newly allocated + * error string, and return NULL. + **/ +STATIC control_cmd_args_t * +control_cmd_parse_args(const char *command, + const control_cmd_syntax_t *syntax, + size_t body_len, + const char *body, + char **error_out) +{ + *error_out = NULL; + control_cmd_args_t *result = tor_malloc_zero(sizeof(control_cmd_args_t)); + const char *cmdline; + char *cmdline_alloc = NULL; + + result->command = command; + + const char *eol = memchr(body, '\n', body_len); + if (syntax->want_object) { + if (! eol || (eol+1) == body+body_len) { + *error_out = tor_strdup("Empty body"); + goto err; + } + cmdline_alloc = tor_memdup_nulterm(body, eol-body); + cmdline = cmdline_alloc; + ++eol; + result->object_len = read_escaped_data(eol, (body+body_len)-eol, + &result->object); + } else { + if (eol && (eol+1) != body+body_len) { + *error_out = tor_strdup("Unexpected body"); + goto err; + } + cmdline = body; + } + + result->args = smartlist_new(); + smartlist_split_string(result->args, cmdline, " ", + SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + size_t n_args = smartlist_len(result->args); + if (n_args < syntax->min_args) { + tor_asprintf(error_out, "Need at least %u argument(s)", + syntax->min_args); + goto err; + } else if (n_args > syntax->max_args) { + tor_asprintf(error_out, "Cannot accept more than %u argument(s)", + syntax->max_args); + goto err; + } + + tor_assert_nonfatal(*error_out == NULL); + goto done; + err: + tor_assert_nonfatal(*error_out != NULL); + control_cmd_args_free(result); + done: + tor_free(cmdline_alloc); + return result; +} + /** Called when we receive a SETCONF message: parse the body and try * to update our configuration. Reply with a DONE or ERROR message. * Modifies the contents of body.*/ @@ -2230,7 +2312,8 @@ handle_control_obsolete(control_connection_t *conn, **/ typedef enum handler_type_t { hnd_legacy, - hnd_legacy_mut + hnd_legacy_mut, + hnd_parsed, } handler_type_t; /** @@ -2257,6 +2340,13 @@ typedef union handler_fn_t { int (*legacy_mut)(control_connection_t *conn, uint32_t arg_len, char *args); + + /** + * A "parsed" handler expects its arguments in a pre-parsed format, in + * an immutable control_cmd_args_t *object. + **/ + int (*parsed)(control_connection_t *conn, + const control_cmd_args_t *args); } handler_fn_t; /** @@ -2279,6 +2369,10 @@ typedef struct control_cmd_def_t { * Zero or more CMD_FL_* flags, or'd together. */ unsigned flags; + /** + * For parsed command: a syntax description. + */ + const control_cmd_syntax_t *syntax; } control_cmd_def_t; /** @@ -2287,16 +2381,27 @@ typedef struct control_cmd_def_t { */ #define CMD_FL_WIPE (1u<<0) -/** - * Macro: declare a command with a one-line argument and a given set of - * flags. +#define SYNTAX_IGNORE { 0, UINT_MAX, false } + +/** Macro: declare a command with a one-line argument, a given set of flags, + * and a syntax definition. **/ -#define ONE_LINE(name, htype, flags) \ +#define ONE_LINE_(name, htype, flags, syntax) \ { #name, \ hnd_ ##htype, \ { .htype = handle_control_ ##name }, \ - flags \ + flags, \ + syntax, \ } + +/** Macro: declare a parsed command with a one-line argument, a given set of + * flags, and a syntax definition. + **/ +#define ONE_LINE(name, htype, flags) \ + ONE_LINE_(name, htype, flags, NULL) +#define ONE_LINE_PARSED(name, flags, syntax) \ + ONE_LINE_(name, parsed, flags, syntax) + /** * Macro: declare a command with a multi-line argument and a given set of * flags. @@ -2305,7 +2410,8 @@ typedef struct control_cmd_def_t { { "+"#name, \ hnd_ ##htype, \ { .htype = handle_control_ ##name }, \ - flags \ + flags, \ + NULL \ } /** * Macro: declare an obsolete command. (Obsolete commands give a different @@ -2315,7 +2421,8 @@ typedef struct control_cmd_def_t { { #name, \ hnd_legacy, \ { .legacy = handle_control_obsolete }, \ - 0 \ + 0, \ + NULL, \ } /** @@ -2379,6 +2486,28 @@ handle_single_control_command(const control_cmd_def_t *def, if (def->handler.legacy_mut(conn, cmd_data_len, args)) rv = -1; break; + case hnd_parsed: { + control_cmd_args_t *parsed_args; + char *err=NULL; + tor_assert(def->syntax); + parsed_args = control_cmd_parse_args(conn->incoming_cmd, + def->syntax, + cmd_data_len, args, + &err); + if (!parsed_args) { + connection_printf_to_buf(conn, + "512 Bad arguments to %s: %s\r\n", + conn->incoming_cmd, err?err:""); + tor_free(err); + } else { + if (BUG(err)) + tor_free(err); + if (def->handler.parsed(conn, parsed_args)) + rv = 0; + control_cmd_args_free(parsed_args); + } + break; + } default: tor_assert_unreached(); } diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index a417e10da..1070a9edb 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -12,11 +12,19 @@ #ifndef TOR_CONTROL_CMD_H #define TOR_CONTROL_CMD_H +#include "lib/malloc/malloc.h" + int handle_control_command(control_connection_t *conn, uint32_t cmd_data_len, char *args); void control_cmd_free_all(void); +typedef struct control_cmd_args_t control_cmd_args_t; +void control_cmd_args_free_(control_cmd_args_t *args); + +#define control_cmd_args_free(v) \ + FREE_AND_NULL(control_cmd_args_t, control_cmd_args_free_, (v)) + #ifdef CONTROL_CMD_PRIVATE #include "lib/crypt_ops/crypto_ed25519.h" @@ -39,6 +47,37 @@ STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk, STATIC rend_authorized_client_t *add_onion_helper_clientauth(const char *arg, int *created, char **err_msg_out); +/** + * Definition for the syntax of a controller command, as parsed by + * control_cmd_parse_args. + * + * WORK IN PROGRESS: This structure is going to get more complex as this + * branch goes on. + **/ +typedef struct control_cmd_syntax_t { + /** + * Lowest number of positional arguments that this command accepts. + * 0 for "it's okay not to have positional arguments." + **/ + unsigned int min_args; + /** + * Highest number of positional arguments that this command accepts. + * UINT_MAX for no limit. + **/ + unsigned int max_args; + /** + * True iff this command wants to be followed by a multiline object. + **/ + bool want_object; +} control_cmd_syntax_t; + +STATIC control_cmd_args_t *control_cmd_parse_args( + const char *command, + const control_cmd_syntax_t *syntax, + size_t body_len, + const char *body, + char **error_out); + #endif /* defined(CONTROL_CMD_PRIVATE) */ #ifdef CONTROL_MODULE_PRIVATE diff --git a/src/feature/control/control_cmd_args_st.h b/src/feature/control/control_cmd_args_st.h new file mode 100644 index 000000000..9ecc5d750 --- /dev/null +++ b/src/feature/control/control_cmd_args_st.h @@ -0,0 +1,44 @@ +/* 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_cmd_args_st.h + * \brief Definition for control_cmd_args_t + **/ + +#ifndef TOR_CONTROL_CMD_ST_H +#define TOR_CONTROL_CMD_ST_H + +struct smartlist_t; +struct config_line_t; + +/** + * Parsed arguments for a control command. + * + * WORK IN PROGRESS: This structure is going to get more complex as this + * branch goes on. + **/ +struct control_cmd_args_t { + /** + * The command itself, as provided by the controller. Not owned by this + * structure. + **/ + const char *command; + /** + * Positional arguments to the command. + **/ + struct smartlist_t *args; + /** + * Number of bytes in object; 0 if object is not set. + **/ + size_t object_len; + /** + * A multiline object passed with this command. + **/ + char *object; +}; + +#endif /* !defined(TOR_CONTROL_CMD_ST_H) */

1 0

[tor/master] When parsing a multiline controller command, be careful with linebreaks
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit dbfe1a14e44647a4d5f27f8d495f3468208d75dd Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 2 10:41:12 2019 -0400 When parsing a multiline controller command, be careful with linebreaks The first line break in particular was mishandled: it was discarded if no arguments came before it, which made it impossible to distinguish arguments from the first line of the body. To solve this, we need to allocate a copy of the command rather than using NUL to separate it, since we might have "COMMAND\n" as our input. Fixes ticket 29984. --- changes/ticket29984 | 5 ++++ scripts/maint/practracker/exceptions.txt | 2 +- src/core/mainloop/connection.c | 1 + src/feature/control/control.c | 45 ++++++++++++++++++++--------- src/feature/control/control.h | 3 +- src/feature/control/control_cmd.c | 12 ++++---- src/feature/control/control_connection_st.h | 3 +- 7 files changed, 48 insertions(+), 23 deletions(-) diff --git a/changes/ticket29984 b/changes/ticket29984 new file mode 100644 index 000000000..8631dff27 --- /dev/null +++ b/changes/ticket29984 @@ -0,0 +1,5 @@ + o Minor bugfixes (controller protocol): + - Teach the controller parser to correctly distinguish an object + preceded by an argument list from one without. Previously, it + couldn't distinguish an argument list from the first line of a + multiline object. Fixes bug 29984; bugfix on 0.2.3.8-alpha. diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 7d03bf27d..7582395fe 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -56,7 +56,7 @@ problem function-size /src/app/main/ntmain.c:nt_service_install() 125 problem include-count /src/app/main/shutdown.c 52 problem file-size /src/core/mainloop/connection.c 5558 problem include-count /src/core/mainloop/connection.c 61 -problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 184 +problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 185 problem function-size /src/core/mainloop/connection.c:connection_listener_new() 328 problem function-size /src/core/mainloop/connection.c:connection_handle_listener_read() 161 problem function-size /src/core/mainloop/connection.c:connection_connect_sockaddr() 103 diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c index 51c19b4c4..30504e4ed 100644 --- a/src/core/mainloop/connection.c +++ b/src/core/mainloop/connection.c @@ -697,6 +697,7 @@ connection_free_minimal(connection_t *conn) control_connection_t *control_conn = TO_CONTROL_CONN(conn); tor_free(control_conn->safecookie_client_hash); tor_free(control_conn->incoming_cmd); + tor_free(control_conn->current_cmd); if (control_conn->ephemeral_onion_services) { SMARTLIST_FOREACH(control_conn->ephemeral_onion_services, char *, cp, { memwipe(cp, 0, strlen(cp)); diff --git a/src/feature/control/control.c b/src/feature/control/control.c index 308f1936b..23ef83ef9 100644 --- a/src/feature/control/control.c +++ b/src/feature/control/control.c @@ -276,25 +276,38 @@ peek_connection_has_http_command(connection_t *conn) } /** - * Helper: take a nul-terminated command of given length, and find where - * the command starts and the argument begins. Separate them with a NUL, - * and return a pointer to the arguments. + * Helper: take a nul-terminated command of given length, and find where the + * command starts and the arguments begin. Separate them, allocate a new + * string in current_cmd_out for the command, and return a pointer + * to the arguments. **/ STATIC char * -control_split_incoming_command(char *incoming_cmd, size_t *data_len) +control_split_incoming_command(char *incoming_cmd, + size_t *data_len, + char **current_cmd_out) { + const bool is_multiline = *data_len && incoming_cmd[0] == '+'; size_t cmd_len = 0; while (cmd_len < *data_len && !TOR_ISSPACE(incoming_cmd[cmd_len])) ++cmd_len; - incoming_cmd[cmd_len]='\0'; - char *args = incoming_cmd+cmd_len+1; - tor_assert(*data_len>cmd_len); - *data_len -= (cmd_len+1); /* skip the command and NUL we added after it */ - while (TOR_ISSPACE(*args)) { - ++args; - --*data_len; + *current_cmd_out = tor_memdup_nulterm(incoming_cmd, cmd_len); + char *args = incoming_cmd+cmd_len; + tor_assert(*data_len>=cmd_len); + *data_len -= cmd_len; + if (is_multiline) { + // Only match horizontal space: any line after the first is data, + // not arguments. + while ((*args == '\t' || *args == ' ') && *data_len) { + ++args; + --*data_len; + } + } else { + while (TOR_ISSPACE(*args) && *data_len) { + ++args; + --*data_len; + } } return args; @@ -429,7 +442,11 @@ connection_control_process_inbuf(control_connection_t *conn) /* Okay, we now have a command sitting on conn->incoming_cmd. See if we * recognize it. */ - args = control_split_incoming_command(conn->incoming_cmd, &data_len); + tor_free(conn->current_cmd); + args = control_split_incoming_command(conn->incoming_cmd, &data_len, + &conn->current_cmd); + if (BUG(!conn->current_cmd)) + return -1; /* If the connection is already closing, ignore further commands */ if (TO_CONN(conn)->marked_for_close) { @@ -437,14 +454,14 @@ connection_control_process_inbuf(control_connection_t *conn) } /* Otherwise, Quit is always valid. */ - if (!strcasecmp(conn->incoming_cmd, "QUIT")) { + if (!strcasecmp(conn->current_cmd, "QUIT")) { connection_write_str_to_buf("250 closing connection\r\n", conn); connection_mark_and_flush(TO_CONN(conn)); return 0; } if (conn->base_.state == CONTROL_CONN_STATE_NEEDAUTH && - !is_valid_initial_command(conn, conn->incoming_cmd)) { + !is_valid_initial_command(conn, conn->current_cmd)) { connection_write_str_to_buf("514 Authentication required.\r\n", conn); connection_mark_for_close(TO_CONN(conn)); return 0; diff --git a/src/feature/control/control.h b/src/feature/control/control.h index 6fc1c40ca..8d3595d2e 100644 --- a/src/feature/control/control.h +++ b/src/feature/control/control.h @@ -62,7 +62,8 @@ void set_cached_network_liveness(int liveness); #ifdef CONTROL_PRIVATE STATIC char *control_split_incoming_command(char *incoming_cmd, - size_t *data_len); + size_t *data_len, + char **current_cmd_out); #endif #endif /* !defined(TOR_CONTROL_H) */ diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index aa0bef513..53cbf2bd0 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -2299,7 +2299,7 @@ handle_control_obsolete(control_connection_t *conn, { (void)arg_len; (void)args; - char *command = tor_strdup(conn->incoming_cmd); + char *command = tor_strdup(conn->current_cmd); tor_strupper(command); connection_printf_to_buf(conn, "511 %s is obsolete.\r\n", command); tor_free(command); @@ -2490,14 +2490,14 @@ handle_single_control_command(const control_cmd_def_t *def, control_cmd_args_t *parsed_args; char *err=NULL; tor_assert(def->syntax); - parsed_args = control_cmd_parse_args(conn->incoming_cmd, + parsed_args = control_cmd_parse_args(conn->current_cmd, def->syntax, cmd_data_len, args, &err); if (!parsed_args) { connection_printf_to_buf(conn, "512 Bad arguments to %s: %s\r\n", - conn->incoming_cmd, err?err:""); + conn->current_cmd, err?err:""); tor_free(err); } else { if (BUG(err)) @@ -2519,7 +2519,7 @@ handle_single_control_command(const control_cmd_def_t *def, } /** - * Run a given controller command, as selected by the incoming_cmd field of + * Run a given controller command, as selected by the current_cmd field of * conn. */ int @@ -2533,13 +2533,13 @@ handle_control_command(control_connection_t *conn, for (unsigned i = 0; i < N_CONTROL_COMMANDS; ++i) { const control_cmd_def_t *def = &CONTROL_COMMANDS[i]; - if (!strcasecmp(conn->incoming_cmd, def->name)) { + if (!strcasecmp(conn->current_cmd, def->name)) { return handle_single_control_command(def, conn, cmd_data_len, args); } } connection_printf_to_buf(conn, "510 Unrecognized command \"%s\"\r\n", - conn->incoming_cmd); + conn->current_cmd); return 0; } diff --git a/src/feature/control/control_connection_st.h b/src/feature/control/control_connection_st.h index 177a91625..cace6bb36 100644 --- a/src/feature/control/control_connection_st.h +++ b/src/feature/control/control_connection_st.h @@ -40,7 +40,8 @@ struct control_connection_t { /** A control command that we're reading from the inbuf, but which has not * yet arrived completely. */ char *incoming_cmd; + /** The control command that we are currently processing. */ + char *current_cmd; }; #endif -

1 0

[tor/master] kvline: handle empty alues as well as empty keys
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit 73df91bbb55498b05faae16b49ab49545fdffa8f Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 5 15:29:37 2019 -0400 kvline: handle empty alues as well as empty keys The two options are mutually exclusive, since otherwise an entry like "Foo" would be ambiguous. We want to have the ability to treat entries like this as keys, though, since some controller commands interpret them as flags. --- src/lib/encoding/kvline.c | 48 +++++++++++++++++++++++++++++++++++------ src/lib/encoding/kvline.h | 1 + src/test/test_config.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+), 7 deletions(-) diff --git a/src/lib/encoding/kvline.c b/src/lib/encoding/kvline.c index 307adc3f1..806f9d3db 100644 --- a/src/lib/encoding/kvline.c +++ b/src/lib/encoding/kvline.c @@ -54,6 +54,15 @@ line_has_no_key(const config_line_t *line) } /** + * Return true iff the value in line is not set. + **/ +static bool +line_has_no_val(const config_line_t *line) +{ + return line->value == NULL || strlen(line->value) == 0; +} + +/** * Return true iff the all the lines in line can be encoded * using flags. **/ @@ -98,6 +107,10 @@ kvline_can_encode_lines(const config_line_t *line, unsigned flags) * 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 * allowed. + * + * If KV_OMIT_VALS is set in flags, then an empty value is + * encoded as 'Key', not as 'Key=' or 'Key=""'. Mutually exclusive with + * KV_OMIT_KEYS. */ char * kvline_encode(const config_line_t *line, @@ -106,6 +119,9 @@ kvline_encode(const config_line_t *line, if (!kvline_can_encode_lines(line, flags)) return NULL; + tor_assert((flags & (KV_OMIT_KEYS|KV_OMIT_VALS)) != + (KV_OMIT_KEYS|KV_OMIT_VALS)); + smartlist_t *elements = smartlist_new(); for (; line; line = line->next) { @@ -126,7 +142,10 @@ kvline_encode(const config_line_t *line, } } - if (esc) { + if ((flags & KV_OMIT_VALS) && line_has_no_val(line)) { + eq = ""; + v = ""; + } else if (esc) { tmp = esc_for_log(line->value); v = tmp; } else { @@ -155,13 +174,21 @@ kvline_encode(const config_line_t *line, * * If KV_OMIT_KEYS is set in flags, then values without keys are * allowed. Otherwise, such values are not allowed. + * + * If KV_OMIT_VALS is set in flags, then keys without values are + * allowed. Otherwise, such keys are not allowed. Mutually exclusive with + * KV_OMIT_KEYS. */ 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)); + const char *cp = line, *cplast = NULL; - bool omit_keys = (flags & KV_OMIT_KEYS) != 0; - bool quoted = (flags & KV_QUOTED) != 0; + const bool omit_keys = (flags & KV_OMIT_KEYS) != 0; + const bool omit_vals = (flags & KV_OMIT_VALS) != 0; + const bool quoted = (flags & KV_QUOTED) != 0; config_line_t *result = NULL; config_line_t **next_line = &result; @@ -171,27 +198,33 @@ kvline_parse(const char *line, unsigned flags) while (*cp) { key = val = NULL; + /* skip all spaces */ { size_t idx = strspn(cp, " \t\r\v\n"); cp += idx; } if (BUG(cp == cplast)) { - /* If we didn't parse anything, this code is broken. */ + /* If we didn't parse anything since the last loop, this code is + * broken. */ goto err; // LCOV_EXCL_LINE } cplast = cp; if (! *cp) break; /* End of string; we're done. */ - /* Possible formats are K=V, K="V", V, and "V", depending on flags. */ + /* Possible formats are K=V, K="V", K, V, and "V", depending on flags. */ - /* Find the key. */ + /* Find where the key ends */ if (*cp != '\"') { size_t idx = strcspn(cp, " \t\r\v\n="); if (cp[idx] == '=') { key = tor_memdup_nulterm(cp, idx); cp += idx + 1; + } else if (omit_vals) { + key = tor_memdup_nulterm(cp, idx); + cp += idx; + goto commit; } else { if (!omit_keys) goto err; @@ -214,6 +247,7 @@ kvline_parse(const char *line, unsigned flags) cp += idx; } + commit: if (key && strlen(key) == 0) { /* We don't allow empty keys. */ goto err; @@ -221,7 +255,7 @@ kvline_parse(const char *line, unsigned flags) *next_line = tor_malloc_zero(sizeof(config_line_t)); (*next_line)->key = key ? key : tor_strdup(""); - (*next_line)->value = val; + (*next_line)->value = val ? val : tor_strdup(""); next_line = &(*next_line)->next; key = val = NULL; } diff --git a/src/lib/encoding/kvline.h b/src/lib/encoding/kvline.h index 4eed30a22..6740f81d5 100644 --- a/src/lib/encoding/kvline.h +++ b/src/lib/encoding/kvline.h @@ -17,6 +17,7 @@ struct config_line_t; #define KV_QUOTED (1u<<0) #define KV_OMIT_KEYS (1u<<1) +#define KV_OMIT_VALS (1u<<2) 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 72649dd9b..6cfb7b764 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -5886,6 +5886,61 @@ test_config_kvline_parse(void *arg) tt_assert(lines); tt_str_op(lines->key, OP_EQ, "AB"); tt_str_op(lines->value, OP_EQ, ""); + config_free_lines(lines); + + lines = kvline_parse("AB=", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, ""); + config_free_lines(lines); + + lines = kvline_parse(" AB ", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, ""); + config_free_lines(lines); + + lines = kvline_parse("AB", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, ""); + enc = kvline_encode(lines, KV_OMIT_VALS); + tt_str_op(enc, OP_EQ, "AB"); + tor_free(enc); + config_free_lines(lines); + + lines = kvline_parse("AB=CD", KV_OMIT_VALS); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, "CD"); + enc = kvline_encode(lines, KV_OMIT_VALS); + tt_str_op(enc, OP_EQ, "AB=CD"); + tor_free(enc); + config_free_lines(lines); + + lines = kvline_parse("AB=CD DE FGH=I", KV_OMIT_VALS); + 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, "DE"); + tt_str_op(lines->next->value, OP_EQ, ""); + tt_str_op(lines->next->next->key, OP_EQ, "FGH"); + tt_str_op(lines->next->next->value, OP_EQ, "I"); + enc = kvline_encode(lines, KV_OMIT_VALS); + tt_str_op(enc, OP_EQ, "AB=CD DE FGH=I"); + tor_free(enc); + config_free_lines(lines); + + lines = kvline_parse("AB=\"CD E\" DE FGH=\"I\"", KV_OMIT_VALS|KV_QUOTED); + tt_assert(lines); + tt_str_op(lines->key, OP_EQ, "AB"); + tt_str_op(lines->value, OP_EQ, "CD E"); + tt_str_op(lines->next->key, OP_EQ, "DE"); + tt_str_op(lines->next->value, OP_EQ, ""); + tt_str_op(lines->next->next->key, OP_EQ, "FGH"); + 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"); done: config_free_lines(lines);

1 0

[tor/master] Use parsing code for the simpler controller commands.
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit 01b07c548b93bcc58adac612a02c69dcb4b63b28 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 2 19:00:23 2019 -0400 Use parsing code for the simpler controller commands. (This should be all of the command that work nicely with positional arguments only.) Some of these commands should probably treat extra arguments as incorrect, but for now I'm trying to be careful not to break any existing users. --- scripts/maint/practracker/exceptions.txt | 4 +- src/feature/control/control_cmd.c | 239 +++++++++++++++---------------- src/feature/control/control_cmd.h | 44 +++--- src/feature/control/control_getinfo.c | 16 +-- src/feature/control/control_getinfo.h | 8 +- 5 files changed, 154 insertions(+), 157 deletions(-) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 7582395fe..438c1c582 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -54,7 +54,7 @@ problem function-size /src/app/main/main.c:sandbox_init_filter() 291 problem function-size /src/app/main/main.c:run_tor_main_loop() 105 problem function-size /src/app/main/ntmain.c:nt_service_install() 125 problem include-count /src/app/main/shutdown.c 52 -problem file-size /src/core/mainloop/connection.c 5558 +problem file-size /src/core/mainloop/connection.c 5559 problem include-count /src/core/mainloop/connection.c 61 problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 185 problem function-size /src/core/mainloop/connection.c:connection_listener_new() 328 @@ -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 52 +problem include-count /src/feature/control/control_getinfo.c 53 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 diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index 53cbf2bd0..f457e9fa5 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -160,13 +160,17 @@ handle_control_resetconf(control_connection_t *conn, uint32_t len, char *body) return control_setconf_helper(conn, len, body, 1); } +static const control_cmd_syntax_t getconf_syntax = { + .max_args=UINT_MAX +}; + /** Called when we receive a GETCONF message. Parse the request, and * reply with a CONFVALUE or an ERROR message */ static int -handle_control_getconf(control_connection_t *conn, uint32_t body_len, - char *body) +handle_control_getconf(control_connection_t *conn, + const control_cmd_args_t *args) { - smartlist_t *questions = smartlist_new(); + const smartlist_t *questions = args->args; smartlist_t *answers = smartlist_new(); smartlist_t *unrecognized = smartlist_new(); char *msg = NULL; @@ -174,9 +178,6 @@ handle_control_getconf(control_connection_t *conn, uint32_t body_len, const or_options_t *options = get_options(); int i, len; - (void) body_len; /* body is NUL-terminated; so we can ignore len. */ - smartlist_split_string(questions, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(questions, const char *, q) { if (!option_is_recognized(q)) { smartlist_add(unrecognized, (char*) q); @@ -221,8 +222,6 @@ handle_control_getconf(control_connection_t *conn, uint32_t body_len, SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp)); smartlist_free(answers); - SMARTLIST_FOREACH(questions, char *, cp, tor_free(cp)); - smartlist_free(questions); smartlist_free(unrecognized); tor_free(msg); @@ -230,17 +229,21 @@ handle_control_getconf(control_connection_t *conn, uint32_t body_len, return 0; } +static const control_cmd_syntax_t loadconf_syntax = { + .want_object = true +}; + /** Called when we get a +LOADCONF message. */ static int -handle_control_loadconf(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_loadconf(control_connection_t *conn, + const control_cmd_args_t *args) { setopt_err_t retval; char *errstring = NULL; const char *msg = NULL; - (void) len; - retval = options_init_from_string(NULL, body, CMD_RUN_TOR, NULL, &errstring); + retval = options_init_from_string(NULL, args->object, + CMD_RUN_TOR, NULL, &errstring); if (retval != SETOPT_OK) log_warn(LD_CONTROL, @@ -276,20 +279,20 @@ handle_control_loadconf(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t setevents_syntax = { + .max_args = UINT_MAX +}; + /** Called when we get a SETEVENTS message: update conn->event_mask, * and reply with DONE or ERROR. */ static int -handle_control_setevents(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_setevents(control_connection_t *conn, + const control_cmd_args_t *args) { int event_code; event_mask_t event_mask = 0; - smartlist_t *events = smartlist_new(); - - (void) len; + const smartlist_t *events = args->args; - smartlist_split_string(events, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(events, const char *, ev) { if (!strcasecmp(ev, "EXTENDED") || @@ -311,16 +314,12 @@ handle_control_setevents(control_connection_t *conn, uint32_t len, if (event_code == -1) { connection_printf_to_buf(conn, "552 Unrecognized event \"%s\"\r\n", ev); - SMARTLIST_FOREACH(events, char *, e, tor_free(e)); - smartlist_free(events); return 0; } } event_mask |= (((event_mask_t)1) << event_code); } SMARTLIST_FOREACH_END(ev); - SMARTLIST_FOREACH(events, char *, e, tor_free(e)); - smartlist_free(events); conn->event_mask = event_mask; @@ -348,23 +347,23 @@ handle_control_saveconf(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t signal_syntax = { + .min_args = 1, + .max_args = 1, +}; + /** Called when we get a SIGNAL command. React to the provided signal, and * report success or failure. (If the signal results in a shutdown, success * may not be reported.) */ static int -handle_control_signal(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_signal(control_connection_t *conn, + const control_cmd_args_t *args) { int sig = -1; int i; - int n = 0; - char *s; - - (void) len; - while (body[n] && ! TOR_ISSPACE(body[n])) - ++n; - s = tor_strndup(body, n); + tor_assert(smartlist_len(args->args) == 1); + const char *s = smartlist_get(args->args, 0); for (i = 0; signal_table[i].signal_name != NULL; ++i) { if (!strcasecmp(s, signal_table[i].signal_name)) { @@ -376,7 +375,6 @@ handle_control_signal(control_connection_t *conn, uint32_t len, if (sig < 0) connection_printf_to_buf(conn, "552 Unrecognized signal code \"%s\"\r\n", s); - tor_free(s); if (sig < 0) return 0; @@ -390,15 +388,18 @@ handle_control_signal(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t takeownership_syntax = { + .max_args = UINT_MAX, // This should probably become zero. XXXXX +}; + /** Called when we get a TAKEOWNERSHIP command. Mark this connection * as an owning connection, so that we will exit if the connection * closes. */ static int -handle_control_takeownership(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_takeownership(control_connection_t *conn, + const control_cmd_args_t *args) { - (void)len; - (void)body; + (void)args; conn->is_owning_control_connection = 1; @@ -410,15 +411,18 @@ handle_control_takeownership(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t dropownership_syntax = { + .max_args = UINT_MAX, // This should probably become zero. XXXXX +}; + /** Called when we get a DROPOWNERSHIP command. Mark this connection * as a non-owning connection, so that we will not exit if the connection * closes. */ static int -handle_control_dropownership(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_dropownership(control_connection_t *conn, + const control_cmd_args_t *args) { - (void)len; - (void)body; + (void)args; conn->is_owning_control_connection = 0; @@ -1099,21 +1103,21 @@ handle_control_postdescriptor(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t redirectstream_syntax = { + .min_args = 2, + .max_args = UINT_MAX, // XXX should be 3. +}; + /** Called when we receive a REDIRECTSTERAM command. Try to change the target * address of the named AP stream, and report success or failure. */ static int -handle_control_redirectstream(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_redirectstream(control_connection_t *conn, + const control_cmd_args_t *cmd_args) { entry_connection_t *ap_conn = NULL; char *new_addr = NULL; uint16_t new_port = 0; - smartlist_t *args; - (void) len; - - args = getargs_helper("REDIRECTSTREAM", conn, body, 2, -1); - if (!args) - return 0; + const smartlist_t *args = cmd_args->args; if (!(ap_conn = get_stream(smartlist_get(args, 0))) || !ap_conn->socks_request) { @@ -1133,8 +1137,6 @@ handle_control_redirectstream(control_connection_t *conn, uint32_t len, } } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); if (!new_addr) return 0; @@ -1147,23 +1149,26 @@ handle_control_redirectstream(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t closestream_syntax = { + .min_args = 2, + .max_args = UINT_MAX, /* XXXX This is the original behavior, but + * maybe we should change the spec. */ +}; + /** Called when we get a CLOSESTREAM command; try to close the named stream * and report success or failure. */ static int -handle_control_closestream(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_closestream(control_connection_t *conn, + const control_cmd_args_t *cmd_args) { entry_connection_t *ap_conn=NULL; uint8_t reason=0; - smartlist_t *args; int ok; - (void) len; + const smartlist_t *args = cmd_args->args; - args = getargs_helper("CLOSESTREAM", conn, body, 2, -1); - if (!args) - return 0; + tor_assert(smartlist_len(args) >= 2); - else if (!(ap_conn = get_stream(smartlist_get(args, 0)))) + if (!(ap_conn = get_stream(smartlist_get(args, 0)))) connection_printf_to_buf(conn, "552 Unknown stream \"%s\"\r\n", (char*)smartlist_get(args, 0)); else { @@ -1175,8 +1180,6 @@ handle_control_closestream(control_connection_t *conn, uint32_t len, ap_conn = NULL; } } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); if (!ap_conn) return 0; @@ -1269,19 +1272,20 @@ handle_control_resolve(control_connection_t *conn, uint32_t len, return 0; } +static const control_cmd_syntax_t protocolinfo_syntax = { + .max_args = UINT_MAX +}; + /** Called when we get a PROTOCOLINFO command: send back a reply. */ static int -handle_control_protocolinfo(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_protocolinfo(control_connection_t *conn, + const control_cmd_args_t *cmd_args) { const char *bad_arg = NULL; - smartlist_t *args; - (void)len; + const smartlist_t *args = cmd_args->args; conn->have_sent_protocolinfo = 1; - args = smartlist_new(); - smartlist_split_string(args, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + SMARTLIST_FOREACH(args, const char *, arg, { int ok; tor_parse_long(arg, 10, 0, LONG_MAX, &ok, NULL); @@ -1337,24 +1341,21 @@ handle_control_protocolinfo(control_connection_t *conn, uint32_t len, tor_free(esc_cfile); } done: - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); return 0; } +static const control_cmd_syntax_t usefeature_syntax = { + .max_args = UINT_MAX +}; + /** Called when we get a USEFEATURE command: parse the feature list, and * set up the control_connection's options properly. */ static int handle_control_usefeature(control_connection_t *conn, - uint32_t len, - const char *body) + const control_cmd_args_t *cmd_args) { - smartlist_t *args; + const smartlist_t *args = cmd_args->args; int bad = 0; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ - args = smartlist_new(); - smartlist_split_string(args, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(args, const char *, arg) { if (!strcasecmp(arg, "VERBOSE_NAMES")) ; @@ -1372,22 +1373,19 @@ handle_control_usefeature(control_connection_t *conn, send_control_done(conn); } - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); return 0; } +static const control_cmd_syntax_t dropguards_syntax = { + .max_args = 0, +}; + /** Implementation for the DROPGUARDS command. */ static int handle_control_dropguards(control_connection_t *conn, - uint32_t len, - const char *body) + const control_cmd_args_t *args) { - smartlist_t *args; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ - args = smartlist_new(); - smartlist_split_string(args, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + (void) args; /* We don't take arguments. */ static int have_warned = 0; if (! have_warned) { @@ -1397,15 +1395,9 @@ handle_control_dropguards(control_connection_t *conn, have_warned = 1; } - if (smartlist_len(args)) { - connection_printf_to_buf(conn, "512 Too many arguments to DROPGUARDS\r\n"); - } else { - remove_all_entry_guards(); - send_control_done(conn); - } + remove_all_entry_guards(); + send_control_done(conn); - SMARTLIST_FOREACH(args, char *, cp, tor_free(cp)); - smartlist_free(args); return 0; } @@ -2202,19 +2194,19 @@ add_onion_helper_clientauth(const char *arg, int *created, char **err_msg) return client; } +static const control_cmd_syntax_t del_onion_syntax = { + .min_args = 1, .max_args = 1, +}; + /** Called when we get a DEL_ONION command; parse the body, and remove * the existing ephemeral Onion Service. */ static int handle_control_del_onion(control_connection_t *conn, - uint32_t len, - const char *body) + const control_cmd_args_t *cmd_args) { int hs_version = 0; - smartlist_t *args; - (void) len; /* body is nul-terminated; it's safe to ignore the length */ - args = getargs_helper("DEL_ONION", conn, body, 1, 1); - if (!args) - return 0; + smartlist_t *args = cmd_args->args; + tor_assert(smartlist_len(args) == 1); const char *service_id = smartlist_get(args, 0); if (rend_valid_v2_service_id(service_id)) { @@ -2280,11 +2272,6 @@ handle_control_del_onion(control_connection_t *conn, } out: - SMARTLIST_FOREACH(args, char *, cp, { - memwipe(cp, 0, strlen(cp)); - tor_free(cp); - }); - smartlist_free(args); return 0; } @@ -2399,20 +2386,26 @@ typedef struct control_cmd_def_t { **/ #define ONE_LINE(name, htype, flags) \ ONE_LINE_(name, htype, flags, NULL) -#define ONE_LINE_PARSED(name, flags, syntax) \ - ONE_LINE_(name, parsed, flags, syntax) +#define ONE_LINE_PARSED(name, flags) \ + ONE_LINE_(name, parsed, flags, &name ##_syntax) /** * Macro: declare a command with a multi-line argument and a given set of * flags. **/ -#define MULTLINE(name, htype, flags) \ +#define MULTLINE_(name, htype, flags, syntax) \ { "+"#name, \ hnd_ ##htype, \ { .htype = handle_control_ ##name }, \ flags, \ - NULL \ + syntax \ } + +#define MULTLINE(name, htype, flags) \ + MULTLINE_(name, htype, flags, NULL) +#define MULTLINE_PARSED(name, flags) \ + MULTLINE_(name, parsed, flags, &name##_syntax) + /** * Macro: declare an obsolete command. (Obsolete commands give a different * error than non-existent ones.) @@ -2432,33 +2425,33 @@ static const control_cmd_def_t CONTROL_COMMANDS[] = { ONE_LINE(setconf, legacy_mut, 0), ONE_LINE(resetconf, legacy_mut, 0), - ONE_LINE(getconf, legacy_mut, 0), - MULTLINE(loadconf, legacy, 0), - ONE_LINE(setevents, legacy, 0), + ONE_LINE_PARSED(getconf, 0), + MULTLINE_PARSED(loadconf, 0), + ONE_LINE_PARSED(setevents, 0), ONE_LINE(authenticate, legacy, CMD_FL_WIPE), ONE_LINE(saveconf, legacy, 0), - ONE_LINE(signal, legacy, 0), - ONE_LINE(takeownership, legacy, 0), - ONE_LINE(dropownership, legacy, 0), + ONE_LINE_PARSED(signal, 0), + ONE_LINE_PARSED(takeownership, 0), + ONE_LINE_PARSED(dropownership, 0), ONE_LINE(mapaddress, legacy, 0), - ONE_LINE(getinfo, legacy, 0), + ONE_LINE_PARSED(getinfo, 0), ONE_LINE(extendcircuit, legacy, 0), ONE_LINE(setcircuitpurpose, legacy, 0), OBSOLETE(setrouterpurpose), ONE_LINE(attachstream, legacy, 0), MULTLINE(postdescriptor, legacy, 0), - ONE_LINE(redirectstream, legacy, 0), - ONE_LINE(closestream, legacy, 0), + ONE_LINE_PARSED(redirectstream, 0), + ONE_LINE_PARSED(closestream, 0), ONE_LINE(closecircuit, legacy, 0), - ONE_LINE(usefeature, legacy, 0), + ONE_LINE_PARSED(usefeature, 0), ONE_LINE(resolve, legacy, 0), - ONE_LINE(protocolinfo, legacy, 0), + ONE_LINE_PARSED(protocolinfo, 0), ONE_LINE(authchallenge, legacy, CMD_FL_WIPE), - ONE_LINE(dropguards, legacy, 0), + ONE_LINE_PARSED(dropguards, 0), ONE_LINE(hsfetch, legacy, 0), MULTLINE(hspost, legacy, 0), ONE_LINE(add_onion, legacy, CMD_FL_WIPE), - ONE_LINE(del_onion, legacy, CMD_FL_WIPE), + ONE_LINE_PARSED(del_onion, CMD_FL_WIPE), }; /** diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index 1070a9edb..801bf4370 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -25,28 +25,6 @@ void control_cmd_args_free_(control_cmd_args_t *args); #define control_cmd_args_free(v) \ FREE_AND_NULL(control_cmd_args_t, control_cmd_args_free_, (v)) -#ifdef CONTROL_CMD_PRIVATE -#include "lib/crypt_ops/crypto_ed25519.h" - -/* ADD_ONION secret key to create an ephemeral service. The command supports - * multiple versions so this union stores the key and passes it to the HS - * subsystem depending on the requested version. */ -typedef union add_onion_secret_key_t { - /* Hidden service v2 secret key. */ - crypto_pk_t *v2; - /* Hidden service v3 secret key. */ - ed25519_secret_key_t *v3; -} add_onion_secret_key_t; - -STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk, - const char **key_new_alg_out, - char **key_new_blob_out, - add_onion_secret_key_t *decoded_key, - int *hs_version, char **err_msg_out); - -STATIC rend_authorized_client_t *add_onion_helper_clientauth(const char *arg, - int *created, char **err_msg_out); - /** * Definition for the syntax of a controller command, as parsed by * control_cmd_parse_args. @@ -71,6 +49,28 @@ typedef struct control_cmd_syntax_t { bool want_object; } control_cmd_syntax_t; +#ifdef CONTROL_CMD_PRIVATE +#include "lib/crypt_ops/crypto_ed25519.h" + +/* ADD_ONION secret key to create an ephemeral service. The command supports + * multiple versions so this union stores the key and passes it to the HS + * subsystem depending on the requested version. */ +typedef union add_onion_secret_key_t { + /* Hidden service v2 secret key. */ + crypto_pk_t *v2; + /* Hidden service v3 secret key. */ + ed25519_secret_key_t *v3; +} add_onion_secret_key_t; + +STATIC int add_onion_helper_keyarg(const char *arg, int discard_pk, + const char **key_new_alg_out, + char **key_new_blob_out, + add_onion_secret_key_t *decoded_key, + int *hs_version, char **err_msg_out); + +STATIC rend_authorized_client_t *add_onion_helper_clientauth(const char *arg, + int *created, char **err_msg_out); + STATIC control_cmd_args_t *control_cmd_parse_args( const char *command, const control_cmd_syntax_t *syntax, diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c index a7a85f2fd..5c6a0d4aa 100644 --- a/src/feature/control/control_getinfo.c +++ b/src/feature/control/control_getinfo.c @@ -55,6 +55,7 @@ #include "core/or/origin_circuit_st.h" #include "core/or/socks_request_st.h" #include "feature/control/control_connection_st.h" +#include "feature/control/control_cmd_args_st.h" #include "feature/dircache/cached_dir_st.h" #include "feature/nodelist/extrainfo_st.h" #include "feature/nodelist/microdesc_st.h" @@ -1584,21 +1585,22 @@ handle_getinfo_helper(control_connection_t *control_conn, return 0; /* unrecognized */ } +const control_cmd_syntax_t getinfo_syntax = { + .max_args = UINT_MAX, +}; + /** Called when we receive a GETINFO command. Try to fetch all requested * information, and reply with information or error message. */ int -handle_control_getinfo(control_connection_t *conn, uint32_t len, - const char *body) +handle_control_getinfo(control_connection_t *conn, + const control_cmd_args_t *args) { - smartlist_t *questions = smartlist_new(); + const smartlist_t *questions = args->args; smartlist_t *answers = smartlist_new(); smartlist_t *unrecognized = smartlist_new(); char *ans = NULL; int i; - (void) len; /* body is NUL-terminated, so it's safe to ignore the length. */ - smartlist_split_string(questions, body, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); SMARTLIST_FOREACH_BEGIN(questions, const char *, q) { const char *errmsg = NULL; @@ -1653,8 +1655,6 @@ handle_control_getinfo(control_connection_t *conn, uint32_t len, done: SMARTLIST_FOREACH(answers, char *, cp, tor_free(cp)); smartlist_free(answers); - SMARTLIST_FOREACH(questions, char *, cp, tor_free(cp)); - smartlist_free(questions); SMARTLIST_FOREACH(unrecognized, char *, cp, tor_free(cp)); smartlist_free(unrecognized); diff --git a/src/feature/control/control_getinfo.h b/src/feature/control/control_getinfo.h index d5a2feb3e..2d56586f6 100644 --- a/src/feature/control/control_getinfo.h +++ b/src/feature/control/control_getinfo.h @@ -12,8 +12,12 @@ #ifndef TOR_CONTROL_GETINFO_H #define TOR_CONTROL_GETINFO_H -int handle_control_getinfo(control_connection_t *conn, uint32_t len, - const char *body); +struct control_cmd_syntax_t; +struct control_cmd_args_t; +extern const struct control_cmd_syntax_t getinfo_syntax; + +int handle_control_getinfo(control_connection_t *conn, + const struct control_cmd_args_t *args); #ifdef CONTROL_GETINFO_PRIVATE STATIC int getinfo_helper_onions(

1 0

[tor/master] Allow kvlines in control commands.
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit 0841a69357d73353905f8012f455ec6128201131 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 5 15:29:56 2019 -0400 Allow kvlines in control commands. --- src/feature/control/control_cmd.c | 46 +++++++++++++++++++++++++++++-- src/feature/control/control_cmd.h | 17 ++++++++++++ src/feature/control/control_cmd_args_st.h | 4 +++ src/test/test_controller.c | 24 ++++++++++++++++ 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/src/feature/control/control_cmd.c b/src/feature/control/control_cmd.c index f457e9fa5..727950a93 100644 --- a/src/feature/control/control_cmd.c +++ b/src/feature/control/control_cmd.c @@ -40,6 +40,7 @@ #include "lib/crypt_ops/crypto_rand.h" #include "lib/crypt_ops/crypto_util.h" #include "lib/encoding/confline.h" +#include "lib/encoding/kvline.h" #include "core/or/cpath_build_state_st.h" #include "core/or/entry_connection_st.h" @@ -74,12 +75,28 @@ control_cmd_args_free_(control_cmd_args_t *args) SMARTLIST_FOREACH(args->args, char *, c, tor_free(c)); smartlist_free(args->args); } + config_free_lines(args->kwargs); tor_free(args->object); tor_free(args); } /** + * Return true iff any element of the NULL-terminated array matches + * kwd. Case-insensitive. + **/ +static bool +string_array_contains_keyword(const char **array, const char *kwd) +{ + for (unsigned i = 0; array[i]; ++i) { + if (! strcasecmp(array[i], kwd)) + return true; + } + return false; +} + + +/** * Helper: parse the arguments to a command according to syntax. On * success, set *error_out to NULL and return a newly allocated * control_cmd_args_t. On failure, set *error_out to newly allocated @@ -96,6 +113,7 @@ control_cmd_parse_args(const char *command, control_cmd_args_t *result = tor_malloc_zero(sizeof(control_cmd_args_t)); const char *cmdline; char *cmdline_alloc = NULL; + tor_assert(syntax->max_args < INT_MAX || syntax->max_args == UINT_MAX); result->command = command; @@ -120,18 +138,42 @@ control_cmd_parse_args(const char *command, result->args = smartlist_new(); smartlist_split_string(result->args, cmdline, " ", - SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, + (int)(syntax->max_args+1)); size_t n_args = smartlist_len(result->args); if (n_args < syntax->min_args) { tor_asprintf(error_out, "Need at least %u argument(s)", syntax->min_args); goto err; - } else if (n_args > syntax->max_args) { + } else if (n_args > syntax->max_args && ! syntax->accept_keywords) { tor_asprintf(error_out, "Cannot accept more than %u argument(s)", syntax->max_args); goto err; } + if (n_args > syntax->max_args) { + tor_assert(n_args == syntax->max_args + 1); + tor_assert(syntax->accept_keywords); + char *remainder = smartlist_pop_last(result->args); + result->kwargs = kvline_parse(remainder, syntax->kvline_flags); + tor_free(remainder); + if (result->kwargs == NULL) { + tor_asprintf(error_out, "Cannot parse keyword argument(s)"); + goto err; + } + if (syntax->allowed_keywords) { + /* Check for unpermitted arguments */ + const config_line_t *line; + for (line = result->kwargs; line; line = line->next) { + if (! string_array_contains_keyword(syntax->allowed_keywords, + line->key)) { + tor_asprintf(error_out, "Unrecognized keyword argument %s", + escaped(line->key)); + } + } + } + } + tor_assert_nonfatal(*error_out == NULL); goto done; err: diff --git a/src/feature/control/control_cmd.h b/src/feature/control/control_cmd.h index 801bf4370..6f35c74de 100644 --- a/src/feature/control/control_cmd.h +++ b/src/feature/control/control_cmd.h @@ -44,6 +44,23 @@ typedef struct control_cmd_syntax_t { **/ unsigned int max_args; /** + * If true, we should parse options after the positional arguments + * as a set of unordered flags and key=value arguments. + * + * Requires that max_args is not UINT_MAX. + **/ + bool accept_keywords; + /** + * If accept_keywords is true, then only the keywords listed in this + * (NULL-terminated) array are valid keywords for this command. + **/ + const char **allowed_keywords; + /** + * If accept_keywords is true, this option is passed to kvline_parse() as + * its flags. + **/ + unsigned kvline_flags; + /** * True iff this command wants to be followed by a multiline object. **/ bool want_object; diff --git a/src/feature/control/control_cmd_args_st.h b/src/feature/control/control_cmd_args_st.h index 9ecc5d750..06e2a183b 100644 --- a/src/feature/control/control_cmd_args_st.h +++ b/src/feature/control/control_cmd_args_st.h @@ -32,6 +32,10 @@ struct control_cmd_args_t { **/ struct smartlist_t *args; /** + * Keyword arguments to the command. + **/ + struct config_line_t *kwargs; + /** * Number of bytes in object; 0 if object is not set. **/ size_t object_len; diff --git a/src/test/test_controller.c b/src/test/test_controller.c index ca853367a..dc286dacc 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -18,6 +18,8 @@ #include "test/test.h" #include "test/test_helpers.h" #include "lib/net/resolve.h" +#include "lib/encoding/confline.h" +#include "lib/encoding/kvline.h" #include "feature/control/control_connection_st.h" #include "feature/control/control_cmd_args_st.h" @@ -58,6 +60,16 @@ control_cmd_dump_args(const control_cmd_args_t *result) buf_add_string(buf, ", obj="); buf_add_string(buf, escaped(result->object)); } + if (result->kwargs) { + buf_add_string(buf, ", { "); + const config_line_t *line; + for (line = result->kwargs; line; line = line->next) { + const bool last = (line->next == NULL); + buf_add_printf(buf, "%s=%s%s ", line->key, escaped(line->value), + last ? "" : ","); + } + buf_add_string(buf, "}"); + } buf_add_string(buf, " }"); char *encoded = buf_extract(buf, NULL); @@ -152,6 +164,17 @@ static const control_cmd_syntax_t no_args_one_obj_syntax = { static const parse_test_params_t parse_no_args_one_obj_params = TESTPARAMS( no_args_one_obj_syntax, no_args_one_obj_tests ); +static const parser_testcase_t no_args_kwargs_tests[] = { + OK("", "{ args=[] }"), +}; +static const control_cmd_syntax_t no_args_kwargs_syntax = { + .min_args=0, .max_args=0, + .accept_keywords=true, + .kvline_flags=KV_OMIT_VALS +}; +static const parse_test_params_t parse_no_args_kwargs_params = + TESTPARAMS( no_args_kwargs_syntax, no_args_kwargs_tests ); + static void test_add_onion_helper_keyarg_v3(void *arg) { @@ -1752,6 +1775,7 @@ test_getinfo_md_all(void *arg) struct testcase_t controller_tests[] = { PARSER_TEST(one_to_three), PARSER_TEST(no_args_one_obj), + PARSER_TEST(no_args_kwargs), { "add_onion_helper_keyarg_v2", test_add_onion_helper_keyarg_v2, 0, NULL, NULL }, { "add_onion_helper_keyarg_v3", test_add_onion_helper_keyarg_v3, 0,

1 0

[tor/master] Unit tests for current control-command parser logic
by dgoulet＠torproject.org 30 Apr '19

30 Apr '19

commit cbd1a7e0534d6f5e693c65c8a8b239e35e6f3b2c Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 2 11:08:22 2019 -0400 Unit tests for current control-command parser logic --- src/test/test_controller.c | 134 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/src/test/test_controller.c b/src/test/test_controller.c index f3af6d2ec..ca853367a 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -20,10 +20,138 @@ #include "lib/net/resolve.h" #include "feature/control/control_connection_st.h" +#include "feature/control/control_cmd_args_st.h" #include "feature/dirclient/download_status_st.h" #include "feature/nodelist/microdesc_st.h" #include "feature/nodelist/node_st.h" +typedef struct { + const char *input; + const char *expected_parse; + const char *expected_error; +} parser_testcase_t; + +typedef struct { + const control_cmd_syntax_t *syntax; + size_t n_testcases; + const parser_testcase_t *testcases; +} parse_test_params_t; + +static char * +control_cmd_dump_args(const control_cmd_args_t *result) +{ + buf_t *buf = buf_new(); + buf_add_string(buf, "{ args=["); + if (result->args) { + if (smartlist_len(result->args)) { + buf_add_string(buf, " "); + } + SMARTLIST_FOREACH_BEGIN(result->args, const char *, s) { + const bool last = (s_sl_idx == smartlist_len(result->args)-1); + buf_add_printf(buf, "%s%s ", + escaped(s), + last ? "" : ","); + } SMARTLIST_FOREACH_END(s); + } + buf_add_string(buf, "]"); + if (result->object) { + buf_add_string(buf, ", obj="); + buf_add_string(buf, escaped(result->object)); + } + buf_add_string(buf, " }"); + + char *encoded = buf_extract(buf, NULL); + buf_free(buf); + return encoded; +} + +static void +test_controller_parse_cmd(void *arg) +{ + const parse_test_params_t *params = arg; + control_cmd_args_t *result = NULL; + char *error = NULL; + char *encoded = NULL; + + for (size_t i = 0; i < params->n_testcases; ++i) { + const parser_testcase_t *t = &params->testcases[i]; + result = control_cmd_parse_args("EXAMPLE", + params->syntax, + strlen(t->input), + t->input, + &error); + // A valid test should expect exactly one parse or error. + tt_int_op((t->expected_parse == NULL), OP_NE, + (t->expected_error == NULL)); + // We get a result or an error, not both. + tt_int_op((result == NULL), OP_EQ, (error != NULL)); + // We got the one we expected. + tt_int_op((result == NULL), OP_EQ, (t->expected_parse == NULL)); + + if (result) { + encoded = control_cmd_dump_args(result); + tt_str_op(encoded, OP_EQ, t->expected_parse); + } else { + tt_str_op(error, OP_EQ, t->expected_error); + } + + tor_free(error); + tor_free(encoded); + control_cmd_args_free(result); + } + + done: + tor_free(error); + tor_free(encoded); + control_cmd_args_free(result); +} + +#define OK(inp, out) \ + { inp "\r\n", out, NULL } +#define ERR(inp, err) \ + { inp "\r\n", NULL, err } + +#define TESTPARAMS(syntax, array) \ + { &syntax, \ + ARRAY_LENGTH(array), \ + array } + +static const parser_testcase_t one_to_three_tests[] = { + ERR("", "Need at least 1 argument(s)"), + ERR(" \t", "Need at least 1 argument(s)"), + OK("hello", "{ args=[ \"hello\" ] }"), + OK("hello world", "{ args=[ \"hello\", \"world\" ] }"), + OK("hello world", "{ args=[ \"hello\", \"world\" ] }"), + OK(" hello world", "{ args=[ \"hello\", \"world\" ] }"), + OK(" hello world ", "{ args=[ \"hello\", \"world\" ] }"), + OK("hello there world", "{ args=[ \"hello\", \"there\", \"world\" ] }"), + ERR("why hello there world", "Cannot accept more than 3 argument(s)"), + ERR("hello\r\nworld.\r\n.", "Unexpected body"), +}; + +static const control_cmd_syntax_t one_to_three_syntax = { + .min_args=1, .max_args=3 +}; + +static const parse_test_params_t parse_one_to_three_params = + TESTPARAMS( one_to_three_syntax, one_to_three_tests ); + +static const parser_testcase_t no_args_one_obj_tests[] = { + ERR("Hi there!\r\n.", "Cannot accept more than 0 argument(s)"), + ERR("", "Empty body"), + OK("\r\n", "{ args=[], obj=\"\\n\" }"), + OK("\r\nHello world\r\n", "{ args=[], obj=\"Hello world\\n\\n\" }"), + OK("\r\nHello\r\nworld\r\n", "{ args=[], obj=\"Hello\\nworld\\n\\n\" }"), + OK("\r\nHello\r\n..\r\nworld\r\n", + "{ args=[], obj=\"Hello\\n.\\nworld\\n\\n\" }"), +}; +static const control_cmd_syntax_t no_args_one_obj_syntax = { + .min_args=0, .max_args=0, + .want_object=true, +}; +static const parse_test_params_t parse_no_args_one_obj_params = + TESTPARAMS( no_args_one_obj_syntax, no_args_one_obj_tests ); + static void test_add_onion_helper_keyarg_v3(void *arg) { @@ -1617,7 +1745,13 @@ test_getinfo_md_all(void *arg) return; } +#define PARSER_TEST(type) \ + { "parse/" #type, test_controller_parse_cmd, 0, &passthrough_setup, \ + (void*)&parse_ ## type ## _params } + struct testcase_t controller_tests[] = { + PARSER_TEST(one_to_three), + PARSER_TEST(no_args_one_obj), { "add_onion_helper_keyarg_v2", test_add_onion_helper_keyarg_v2, 0, NULL, NULL }, { "add_onion_helper_keyarg_v3", test_add_onion_helper_keyarg_v3, 0,

1 0